Skip to content
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

Introduce MigrationContext to be prepared for JDBC migration refactor #152

Merged
merged 2 commits into from
Dec 21, 2023

Conversation

rPraml
Copy link
Contributor

@rPraml rPraml commented Dec 11, 2023

Hello @rbygrave ,

I tried to extract the planned MigrationContext from #151, so that the PR does not become too big.

It would be good, if I get feedback, so this gets merged soon. Then I'll continue with #151. (probably next year)

This PR should not introcude breaking changes.

Roland

@rPraml
Copy link
Contributor Author

rPraml commented Dec 11, 2023

FYI: We do not use AutoRunner, as we have a multi-tenant setup.
Our current migration code looks like this:

private Database database;

void initDatabase() { // will be invoked for each tenant (in multiple threads)
    // every JDBC migration inherits from AbstractJdbcMigration class and stores current DB in a threadLocal
    AbstractJdbcMigration.setDb(database); 
    try {
        MigrationConfig migrationConfig = new MigrationConfig();
        // ... set up config
        // I want to pass my own context here, so that I can remove the threadLocal and
        // have a
        new MigrationRunner(migrationConfig).run(database.dataSource()); 
    } finally {
        AbstractJdbcMigration.setDb(database);
    }
}

Layout of the AbstractJdbcMigration:

public abstract class AbstractJdbcMigration implements JdbcMigration {
	private static final ThreadLocal<Database> DB = new ThreadLocal<>();
	public static void setDb(final Database db) {
		if (db == null) {
			DB.remove();
		} else {
			DB.set(db);
		}
	}
	public void migrate(final Connection connection) {

		SpiEbeanServer defaultServer = (SpiEbeanServer) DB.get();
		assert defaultServer != null;
		TransactionManager transactionManager = (TransactionManager) defaultServer.transactionManager();
		SpiTransaction txn = transactionManager.wrapExternalConnection(connection);
		transactionManager.externalBeginTransaction(txn, TxScope.notSupported());
		try {
			migrate(defaultServer);
			txn.flush();
		} finally {
			transactionManager.externalRemoveTransaction();
		}
	}
	protected abstract void migrate(Database db);
}

Example migration.

public class V1_2_3_MigrateSomeJson extends AbstractJdbcMigration {
        @Data
        static class MyDto {
                private UUID id;
                private String someJson;
        }
	protected void migrate(final Database db) {
		List<MyDto> lst = db.findDto(MyDto.class, "select id, some_json from my_table");
                for (MyDto dto : lst) {
                      db.sqlUpdate("update my_table set some_json = :json where id = :id)
                           .setParameter("json", convertJson(dto.getSomeJson))
                           .setParameter("id", dto.getId())
                           .update()
                }
	}
       String convertJson(String oldJson) {
                // convert the JSON with Jackson ObjectMapper
       }
}

This migrates the JSON content of an existing field.
having access to the db gives a lot of advantages:

  • good wrapping of all data types (especially the UUIDs often needs conversions from byte-array etc)
  • ability to use a lot of ebean features. Like dto and update queries
  • cleaner code (no need to iterate of raw JDBC result sets etc, error handling etc.)

*
* @author Roland Praml, FOCONIS AG
*/
public class DefaultMigrationContext implements MigrationContext {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great, if ebean-migration can have a dependency to have access to "Database". What do you think about this idea?

Otherwise, I plan to imlement a EbeanMigrationContext extends DefaultMigrationContext, that has a Database getDb() method in our application and cast to this context in our JdbcMigrations

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have access to "Database".

Yes, I would expect it along with a ebean Transaction - so with Database and Transaction then Jdbc Migrations can use all the features of ebean etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that I don't expect this DefaultMigrationContext to be public.

Copy link
Member

@rbygrave rbygrave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work !!

*
* @author Roland Praml, FOCONIS AG
*/
public class DefaultMigrationContext implements MigrationContext {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have access to "Database".

Yes, I would expect it along with a ebean Transaction - so with Database and Transaction then Jdbc Migrations can use all the features of ebean etc.

*
* @author Roland Praml, FOCONIS AG
*/
public class DefaultMigrationContext implements MigrationContext {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that I don't expect this DefaultMigrationContext to be public.

@rbygrave rbygrave merged commit 3203ba4 into ebean-orm:master Dec 21, 2023
1 check passed
@rbygrave rbygrave added this to the 13.12.0 milestone Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants