-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Fix CNFE in native mode for Flyway + MSSQL #21615
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I think this may also going to happen with other DBs supported by Flyway Teams edition (where the |
Ah ah, funny, I was going to add the same comment. |
@gsmet great minds think alike 😆 |
Looking at the sources it seems that SQLServerDatabaseType is the only one that uses it, so we're good |
I think this is due to the new changes made for the 8.0.5 upgrade. I /think/ I registered all of them in the patch I had locally. I had something like that to mimic what was done before (we included all the implementations before the upgrade to 8.0.5): + @BuildStep
+ void registerDatabaseTypes(CombinedIndexBuildItem combinedIndex,
+ BuildProducer<ReflectiveClassBuildItem> reflectiveClasses) {
+ for (ClassInfo databaseType : combinedIndex.getIndex().getAllKnownImplementors(DATABASE_TYPE)) {
+ if (ORACLE_DATABASE_TYPE.equals(databaseType.name())) {
+ try {
+ // Only add the OracleDatabaseType if the driver exists
+ Class.forName("oracle.jdbc.OracleConnection");
+ } catch (ClassNotFoundException e) {
+ continue;
+ }
+ } else if (POSTGRESQL_DATABASE_TYPE.equals(databaseType.name())) {
+ try {
+ // Only add the PostgreSQLDatabaseType if the driver exists
+ Class.forName("org.postgresql.Driver");
+ } catch (ClassNotFoundException e) {
+ continue;
+ }
+ }
+
+ System.out.println("Registering for reflection: " + databaseType.name().toString());
+
+ reflectiveClasses.produce(new ReflectiveClassBuildItem(true, true, databaseType.name().toString()));
+ }
+ } Basically, this was a replacement for what was done before here: https://github.com/quarkusio/quarkus/blob/2.5/extensions/flyway/runtime/src/main/java/io/quarkus/flyway/runtime/graal/DatabaseTypeRegisterSubstitutions.java#L27 (this class was dropped from Flyway to use a ServiceLoader based approach). |
👍🏼 |
Now maybe it's not needed, just saying what I did locally before @gastaldi did the work on his side for the 8.0.5 upgrade. |
@gsmet I am not sure this fix needs to be backported. This change happens in 8.0.5 only, and that was not marked to be backported (see #21526) These are the 8.0.3 sources: https://github.com/flyway/flyway/blob/flyway-8.0.3/flyway-core/src/main/java/org/flywaydb/core/internal/database/sqlserver/SQLServerDatabaseType.java#L92 |
Actually I think an easier solution is to create a |
Done in #21617 |
Superseded by #21617 |
Fixes: #21611