New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow users to use their own implementation of FileMigrationLoader. R… #107

Merged
merged 1 commit into from Dec 15, 2017

Conversation

Projects
None yet
3 participants
@harawata
Member

harawata commented Dec 12, 2017

…elated to #93 #102 .

To use a custom FileMigrationLoader, you need to ...

  1. create an implementation of FileMigrationLoaderFactory [1].
  2. create a JAR including a file /META-INF/services/org.apache.ibatis.migration.FileMigrationLoaderFactory that contains fully-qualified name of the custom implementation (Migrations looks for a custom implementation via Java Service Provider Interface).
  3. drop the JAR file in $MIGRATIONS_HOME/lib directory.

[1] Note that the signature of FileMigrationLoaderFactory#create() is still under discussion and could be changed before final release.

Please see these examples (not tested thoroughly!).

@h3adache ,
I'm so sorry to keep bothering you, but could you please see if this change is acceptable or not when you have time?
Thanks in advance!

@chb0github

If we take this a level higher it could be really powerful. This would allow a fully codeable migration.

Properties variables = environment().getVariables();
MigrationLoader migrationLoader = null;
for (FileMigrationLoaderFactory factory : ServiceLoader.load(FileMigrationLoaderFactory.class)) {
if (migrationLoader != null) {

This comment has been minimized.

@chb0github

chb0github Dec 13, 2017

Contributor

I like this idea. How about taking it one step higher to MigrationLoaderFactory so that it's no longer file based? Basically, you can abstract the Migration and get an iterator of them.

This comment has been minimized.

@h3adache

h3adache Dec 13, 2017

Member

Yea I agree, we have a MigrationLoader so a MigrationLoader factory makes a lot of sense. The FileMigrationLoader is just an implementation.
It might be more useful to just give it the path(),environment() for flexibility.

This comment has been minimized.

@harawata

harawata Dec 13, 2017

Member

Thank you guys for the quick feedback!

How about taking it one step higher to MigrationLoaderFactory so that it's no longer file based?

I thought about that.
The return type of FileMigrationLoaderFactory#create() is MigrationLoader, so by design, it is possible to return a custom MigrationLoader that loads migrations from some other source (i.e. it does not have to be a subclass of FileMigrationLoader).

But this is BaseCommand which is the base class for the command line API and the current command line workflow cannot be separated from files completely (e.g. migrate new).
That is why I named it FileMigrationLoaderFactory.

It might be more useful to just give it the path(),environment() for flexibility.

It was my first implementation and I'm still not that sure which is better, so if you think that is better, I'll change. :)

This comment has been minimized.

@h3adache

h3adache Dec 13, 2017

Member

Yea it's tied to Files atm but we also have for example JavaMigrationLoader that is useful for runtime migrations.

This comment has been minimized.

@chb0github

chb0github Dec 14, 2017

Contributor

I saw that. But

JavaMigrationLoader has a method getMigrations() which returns a List<Change> (counter conventional, btw).

When you look at Change you see

public class Change implements Comparable<Change>, Cloneable {

  private BigDecimal id;
  private String description;
  private String appliedTimestamp;
  private String filename;
}

Notice the part where it says fileName? It's still anchored to the file system.

Side question: Why is the id BigDecimal? Do you expect a version like 3.1415926?

This comment has been minimized.

@h3adache

h3adache Dec 14, 2017

Member

Not sure what you mean @chb0github, both have getMigrations which returns a list of changes.

Even if we change the signature of create() method as @h3adache suggested, it still is coupled with file system and FileMigrationLoaderFactory describes the purpose of this class more accurately, IMO.

That is true. I was just thinking about it from a Factory standpoint. The factory author can create the loader any way they like so I just assumed they would have the proper classLoader/packageNames to create the JavaMigrationLoader

This comment has been minimized.

@h3adache

h3adache Dec 15, 2017

Member

@harawata if you have no more changes let's pull this in. Don't worry about the Factory idea for now. We can deal with it when we have to like you said

This comment has been minimized.

@harawata

harawata Dec 15, 2017

Member

@h3adache ,
Okay!
Regarding the arguments of create(), I re-checked SelectedPaths and Environment and couldn't think of a good use case for other properties.
Let's merge this as it is and see what users say.

I'll add documentation and tests as soon as possible.

Thank you both for insightful comments!

This comment has been minimized.

@h3adache

h3adache Dec 15, 2017

Member

I use it to create a JarFileMigrationLoader. :)
ATM our FileMigrationLoader doesn't read jar bundled migrations properly.
I create a JarFileMigrationLoader to handle that for me but I read the properties from the bundled environment file in the jar as well.

This comment has been minimized.

@harawata
@h3adache

This comment has been minimized.

Member

h3adache commented Dec 13, 2017

This looks great @harawata!

@harawata harawata merged commit ad97fd5 into mybatis:master Dec 15, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@harawata harawata self-assigned this Dec 15, 2017

@harawata harawata added this to the 3.3.2 milestone Dec 15, 2017

@h3adache

This comment has been minimized.

Member

h3adache commented Dec 17, 2017

Thanks @harawata!

@chb0github

This comment has been minimized.

Contributor

chb0github commented Dec 21, 2017

When will this be released?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment