chore: Add flyway repair for the migration #37103
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughA new configuration class, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
2 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/FlywayRepairConfig.java (1)
1-34: Consider adding logging for migration operations.Adding debug/info level logging would help track migration progress and troubleshooting.
Add logging statements:
+import org.slf4j.Logger; +import org.slf4j.LoggerFactory; @Configuration public class FlywayRepairConfig { + private static final Logger logger = LoggerFactory.getLogger(FlywayRepairConfig.class); @Bean public Flyway flyway() { + logger.info("Initializing Flyway migration..."); // ... existing code ... + logger.debug("Executing Flyway repair..."); flyway.repair(); + logger.debug("Executing Flyway migrate..."); flyway.migrate(); + logger.info("Flyway migration completed successfully");
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/FlywayRepairConfig.java (1 hunks)
- app/server/appsmith-server/src/main/resources/application.properties (1 hunks)
🔇 Additional comments (5)
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/FlywayRepairConfig.java (3)
1-8: LGTM! Dependencies look good.All necessary Spring and Flyway imports are present.
9-18: LGTM! Clean constructor injection.Good use of constructor injection with @Autowired annotation.
30-31: Verify concurrent migration safety.The sequential repair() and migrate() calls could potentially cause issues in a multi-instance deployment.
Let's check if there are any distributed locking mechanisms in place:
✅ Verification successful
The previous search was too narrow. Let's broaden our search to understand how Flyway is configured and if there are any migration-related classes that might handle concurrency. Flyway has built-in locking mechanisms, but we need to verify how they're configured in this codebase.
Let's examine the complete Flyway configuration and any Spring configurations that might handle concurrent startup scenarios. We also need to check if there are any custom migration implementations that might affect the concurrent execution.
Flyway's built-in locking mechanism handles concurrent migrations safely
The current implementation is safe for concurrent execution because:
- Flyway provides built-in distributed locking at the database level using its schema history table
- The configuration uses default Flyway settings which include these safety mechanisms
- The repair() operation is also protected by Flyway's internal locking
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for distributed lock implementations rg -l "DistributedLock|@Lock|@Synchronized"Length of output: 45
Script:
#!/bin/bash # Let's search for Flyway configuration and locking mechanisms more broadly rg -i "flyway.*lock|lock.*flyway" -A 5 # Also check for any custom Flyway configuration rg -i "flywayconfig|flyway\.configure" -A 5 # Look for any database migration related classes fd -e java -e xml -H migrationLength of output: 2550
Script:
#!/bin/bash # Let's check the Flyway configuration class in detail cat app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/FlywayRepairConfig.java # Also check for any Spring configuration that might handle concurrent startup rg -i "@configuration.*flyway|bean.*flyway" -A 5 # Check if there are any custom migration implementations cat app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/AppsmithJavaMigration.javaLength of output: 3057
app/server/appsmith-server/src/main/resources/application.properties (2)
17-18: LGTM: Environment-based SQL debugging configuration.The change to use
APPSMITH_SHOW_SQLenvironment variable provides better flexibility for different environments.
Line range hint
1-150: Verify Flyway configuration migration.The removal of Flyway properties (
spring.flyway.enabled,locations,baseline-on-migrate,baseline-version) aligns with the PR objective of programmatic configuration. However, ensure these settings are properly configured in the newFlywayRepairConfigclass.Let's verify the new configuration:
✅ Verification successful
Flyway configuration successfully migrated to Java-based configuration
The Flyway properties have been properly migrated from
application.propertiesto theFlywayRepairConfigclass with all essential configurations:
- Baseline version and migrate settings preserved
- Migration files location correctly set to
classpath:com/appsmith/server/migrations- Database connection properly configured via
DataSourceProperties- Migration files (V001-V007) present in the correct location
The programmatic configuration is complete and correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify Flyway configuration in FlywayRepairConfig class # Test: Search for Flyway configuration settings rg -A 10 "class FlywayRepairConfig" # Test: Check for potential Flyway property references rg "spring.flyway"Length of output: 1447
Script:
#!/bin/bash # Let's check the complete Flyway configuration implementation cat app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/FlywayRepairConfig.java # Also check for any Flyway migration files fd -t f -e sql "V[0-9].*\.sql$" # Check for any other Flyway related configurations rg -g "*.java" -A 5 "Flyway|@FlywayConfiguration"Length of output: 5216
...ver/appsmith-server/src/main/java/com/appsmith/server/configurations/FlywayRepairConfig.java
Outdated
Show resolved
Hide resolved
abhvsn
reviewed
Oct 25, 2024
There was a problem hiding this comment.
@AnaghHegde now this is a bean how do we guarantee that flyway migration runs before the other beans are created?
|
@abhvsn Thanks for highlighting this. This is fixed by using the ApplicationRunner. |
Failed server tests
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (8)
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/ThemesConfig.java (2)
Line range hint
91-92: Remove duplicate repository callThe
themeRepository.getSystemThemeByName()is called twice consecutively.- themeRepository.getSystemThemeByName(theme.getName()); Theme savedTheme = themeRepository.getSystemThemeByName(theme.getName()).orElse(null);
Line range hint
117-122: Consider using anyMatch() instead of filter().findFirst().isPresent()The current stream operation chain can be simplified.
- boolean isThemePermissionPresent = permissions.stream() - .filter(p -> p.getAclPermission().equals(READ_THEMES) - && p.getDocumentId().equals(finalSavedTheme.getId())) - .findFirst() - .isPresent(); + boolean isThemePermissionPresent = permissions.stream() + .anyMatch(p -> p.getAclPermission().equals(READ_THEMES) + && p.getDocumentId().equals(finalSavedTheme.getId()));app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/FlywayRepairConfig.java (2)
15-15: @Autowired annotation on the constructor is unnecessarySince the class has only one constructor, the
@Autowiredannotation can be omitted for brevity.
34-34: Address the TODO commentEnsure to remove the migration runner once the
pgandreleasebranches are merged, as indicated by the TODO.Would you like me to open a GitHub issue to track this task?
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/UserConfig.java (4)
Line range hint
73-73: Handle Optional.empty() before callingget()to preventNoSuchElementExceptionYou're calling
get()oninstanceAdminConfigurationOptionalwithout ensuring it's present. If it's empty, this will throw aNoSuchElementException. Since you've logged an error when it's empty, you should return or handle the empty case appropriately.Apply this diff to fix the issue:
if (instanceAdminConfigurationOptional.isEmpty()) { log.error("Instance configuration not found. Cannot create super users."); + return; } Config instanceAdminConfiguration = instanceAdminConfigurationOptional.get();
Line range hint
83-83: Handle Optional.empty() before callingget()to preventNoSuchElementExceptionYou're calling
get()oninstanceAdminPGOptionalwithout ensuring it's present. If it's empty, this will throw aNoSuchElementException. Since you've logged an error when it's empty, you should return or handle the empty case appropriately.Apply this diff to fix the issue:
if (instanceAdminPGOptional.isEmpty()) { log.error("Instance admin permission group not found. Cannot create super users."); + return; } PermissionGroup instanceAdminPG = instanceAdminPGOptional.get();
Line range hint
89-89: Handle Optional.empty() before callingget()to preventNoSuchElementExceptionYou're calling
get()ontenantOptionalwithout ensuring it's present. If it's empty, this will throw aNoSuchElementException. Since you've logged an error when it's empty, you should return or handle the empty case appropriately.Apply this diff to fix the issue:
if (tenantOptional.isEmpty()) { log.error("Default tenant not found. Cannot create super users."); + return; } Tenant tenant = tenantOptional.get();
Line range hint
100-114: Simplify user retrieval and creation logic usingorElseGetYou can simplify the user retrieval and creation logic by using
orElseGeton the Optional, making the code more concise and readable.Apply this diff to refactor the code:
Set<String> userIds = adminEmails.stream() .map(String::trim) .map(String::toLowerCase) .map(email -> { - User user = null; - Optional<User> userOptional = userRepository.findByEmail(email); - if (userOptional.isPresent()) { - user = userOptional.get(); - } - - if (user == null) { + User user = userRepository.findByEmail(email).orElseGet(() -> { log.info("Creating super user with username {}", email); return updateSuperUserHelper.createNewUser( email, tenant, instanceAdminPG, userRepository, permissionGroupRepository, policySolution, policyGenerator); - } + }); return user.getId(); }) .collect(Collectors.toSet());
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/FlywayRepairConfig.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/TenantConfig.java (3 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/ThemesConfig.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/UserConfig.java (3 hunks)
- app/server/appsmith-server/src/main/resources/application.properties (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-server/src/main/resources/application.properties
🔇 Additional comments (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/TenantConfig.java (1)
10-10: LGTM: Good choice switching to ApplicationReadyEventThe change from ApplicationStartedEvent to ApplicationReadyEvent is appropriate as it ensures tenant cleanup happens after all beans are fully initialized, which is crucial when working with database migrations.
Also applies to: 20-21
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/ThemesConfig.java (1)
36-36: LGTM: Good choice using ApplicationListenerThe change from @bean method to ApplicationListener is a better approach for initialization that depends on application readiness.
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/FlywayRepairConfig.java (2)
22-30: Externalize Flyway configuration propertiesThe Flyway configuration values are hardcoded. Consider moving
locations,baselineOnMigrate, andbaselineVersiontoapplication.propertiesfor flexibility and ease of maintenance.
38-39: Add error handling around Flyway operationsThere is no error handling for
flyway.repair()andflyway.migrate(). Including try-catch blocks will help manage potential exceptions during migration.
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/TenantConfig.java
Show resolved
Hide resolved
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/ThemesConfig.java
Outdated
Show resolved
Hide resolved
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/ThemesConfig.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/ThemesConfig.java (2)
114-115: Consider using method reference for predicate.The lambda expression can be simplified using method reference.
- .anyMatch(p -> p.getAclPermission().equals(READ_THEMES) - && p.getDocumentId().equals(finalSavedTheme.getId())); + .anyMatch(p -> READ_THEMES.equals(p.getAclPermission()) + && finalSavedTheme.getId().equals(p.getDocumentId()));
Race conditions possible during theme initialization
The current implementation in
ThemesConfighas potential race conditions during concurrent application startups:
- Double check on
getSystemThemeByNamefollowed by save/update operations is not atomic- Multiple instances could simultaneously attempt to create/update the same theme
- No transactional boundaries around the theme and permission group updates
Recommendations:
- Use MongoDB's atomic
findAndModifyoperation instead of separate find and save- Add
@Transactionalto ensure atomic updates across theme and permission group changes- Consider adding version field to themes for optimistic locking
🔗 Analysis chain
Line range hint
41-124: Verify database operations for potential race conditions.The theme creation/update logic involves multiple database operations. We should ensure there are no race conditions during concurrent application startups.
Let me search for transaction management and synchronization in the relevant server code, particularly around theme-related operations. I'm also checking the theme repository implementation to understand how the database operations are handled.
I need to verify if we're using Spring's reactive MongoDB, which would handle concurrency differently than traditional MongoDB operations. This will help determine if the current implementation is safe for concurrent operations.
I need to verify the MongoDB configuration and repository pattern being used to understand if the operations are atomic and safe for concurrent access.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any synchronized blocks or transaction management in similar config classes rg -t java "synchronized|@Transactional" --glob "server/configurations/*.java"Length of output: 229
Script:
#!/bin/bash # Search for any synchronized blocks or transaction management in the server directory rg -t java "synchronized|@Transactional" app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/ # Search for theme-related repository implementations ast-grep --pattern 'class $_ implements ThemeRepository' # Search for any transaction management in theme-related classes rg -t java "@Transactional" -A 2 -B 2 app/server/appsmith-server/src/main/java/com/appsmith/server/ | grep -C 2 -i "theme"Length of output: 2550
Script:
#!/bin/bash # Check if ThemeRepository uses Spring's reactive MongoDB ast-grep --pattern 'interface $_ extends ReactiveMongoRepository<Theme, String>' # Look for the actual implementation of getSystemThemeByName rg -t java "getSystemThemeByName" -A 5 app/server/appsmith-server/src/main/java/com/appsmith/server/ # Check if we're using reactive MongoDB operations rg -t java "extends ReactiveMongoTemplate|extends ReactiveMongoOperations" app/server/appsmith-server/src/main/java/com/appsmith/server/Length of output: 11221
Script:
#!/bin/bash # Check the base repository class being used rg -t java "class CustomThemeRepositoryCEImpl extends" -A 2 app/server/appsmith-server/src/main/java/com/appsmith/server/ # Look for MongoDB repository configuration rg -t java "@EnableMongoRepositories|@Configuration.*Mongo" app/server/appsmith-server/src/main/java/com/appsmith/server/ # Check if we're using MongoDB's atomic operations rg -t java "findAndModify|@Query.*value\s*=.*\$set" app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/Length of output: 1356
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/ThemesConfig.java (3 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/configurations/SeedMongoData.java (2 hunks)
🔇 Additional comments (5)
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/ThemesConfig.java (3)
36-36: LGTM! Good transition to event-driven initialization.The change to implement ApplicationListener is a better approach for initialization timing control.
41-52: Skip comment about method extraction.A previous review already suggested extracting the resource loading logic to a separate method.
56-61: LGTM! Improved error handling.The error handling has been enhanced with a more descriptive error message for JSON processing exceptions.
app/server/appsmith-server/src/test/java/com/appsmith/server/configurations/SeedMongoData.java (2)
18-19: LGTM: Import changes align with the new ApplicationListener implementation.
Line range hint
40-48: Verify test stability with the new ApplicationListener timing.The change from ApplicationRunner to ApplicationListener affects when the test data is seeded. While this aligns with other configuration changes, we should verify that tests aren't impacted by this timing change.
Consider documenting the initialization order dependencies in a README or configuration guide, as this change is part of a broader initialization timing update across multiple configuration classes.
abhvsn
reviewed
Oct 28, 2024
| @@ -17,7 +17,7 @@ | |||
| @Configuration | |||
| @RequiredArgsConstructor | |||
| @Slf4j | |||
| public class TenantConfig implements ApplicationListener<ApplicationStartedEvent> { | |||
| public class TenantConfig implements ApplicationListener<ApplicationReadyEvent> { | |||
There was a problem hiding this comment.
I think this was a deliberate choice to avoid any conflicts to fix the tenant object before the application ready event. Any reasone we are changing this?
There was a problem hiding this comment.
This is due to the migration issue. This would otherwise throw error that table does not exists.
| @@ -31,20 +33,32 @@ | |||
| @Configuration | |||
| @Slf4j | |||
| @RequiredArgsConstructor | |||
| public class ThemesConfig { | |||
| public class ThemesConfig implements ApplicationListener<ApplicationReadyEvent> { | |||
There was a problem hiding this comment.
Let's keep all the repetitive migration on ApplicationStartedEvent so that we will be sure on before the InstanceConfig all the migrations are completed.
There was a problem hiding this comment.
This is due to the migration issue. This would otherwise throw error that table does not exists.
| // ApplicationRunner bean to run migrations at startup | ||
| // TODO - Remove this once the pg and release branch is merged | ||
| @Bean | ||
| public ApplicationRunner flywayMigrationRunner(Flyway flyway) { |
There was a problem hiding this comment.
Can you provide some insights when the flyway bean will be instantiated on the spring lifecycle events and at what point we are expecting the flyway will execute the migrations?
There was a problem hiding this comment.
@abhvsn This is done during ApplicationStartedEvent and flyway executes the migration and then the repetable migrations are executed.
There was a problem hiding this comment.
@AnaghHegde I don't think this will work as expected when we add a new field to the entity class as the structure of the table will be affected and repository beans will start complaining. Can we make sure the flyway migration is executed even before the beans are executed just like what it's present today?
There was a problem hiding this comment.
@abhvsn How does this cause the repository beans will start complaining
There was a problem hiding this comment.
@AnaghHegde IMO this will be the case when we update the entity object which getting stored in the DB.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/FlywayRepairConfig.java (1)
35-36: Enhance logging for better observabilityThe current logging only shows total migration time. Consider adding more detailed logging:
- MigrateResult result = flyway.migrate(); - log.info(" Migration completed, total time " + result.getTotalMigrationTime()); + MigrateResult result = flyway.migrate(); + log.info("Migration completed - Success: {}, Migrations executed: {}, Time taken: {}ms", + result.success, + result.migrationsExecuted, + result.totalMigrationTime);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/FlywayRepairConfig.java(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/configurations/FlywayTestConfig.java(1 hunks)app/server/appsmith-server/src/test/resources/application-test.properties(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/server/appsmith-server/src/test/resources/application-test.properties
🔇 Additional comments (4)
app/server/appsmith-server/src/test/java/com/appsmith/server/configurations/FlywayTestConfig.java (3)
1-8: LGTM! Clean and well-organized imports.
9-10: LGTM! Appropriate use of @TestConfiguration.
13-13: Verify @primary bean override behavior.
Ensure that overriding other ApplicationRunner beans with @primary doesn't affect critical startup sequences.
✅ Verification successful
@primary annotation is safe in this test configuration
The @primary ApplicationRunner bean in FlywayTestConfig is specifically scoped to tests (@TestConfiguration) and doesn't interfere with the main Flyway configuration, which uses ApplicationListener instead of ApplicationRunner. The test bean's purpose is to ensure proper database state during tests.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other ApplicationRunner beans that might be affected
ast-grep --pattern 'class $_ {
$$$
@Bean
$$$
ApplicationRunner $_($$$) {
$$$
}
$$$
}'
Length of output: 103
Script:
#!/bin/bash
# Let's try a different approach to find ApplicationRunner beans
# Search for ApplicationRunner implementations and Bean annotations
rg -U "ApplicationRunner.*implements|@Bean.*ApplicationRunner" -A 5
# Also search for Primary annotations near ApplicationRunner
rg -U "@Primary.*ApplicationRunner|ApplicationRunner.*@Primary" -A 5
# Search for files containing both Bean and ApplicationRunner
rg -l "@Bean" | xargs rg -l "ApplicationRunner"
Length of output: 284
Script:
#!/bin/bash
# Let's examine the content of the FlywayTestConfig file
cat app/server/appsmith-server/src/test/java/com/appsmith/server/configurations/FlywayTestConfig.java
# Also search for any other Flyway-related configuration files
rg -l "Flyway" --type java
Length of output: 879
Script:
#!/bin/bash
# Let's check the content of the main Flyway configuration
cat app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/FlywayRepairConfig.java
# Search for any test classes that might be affected by this configuration
rg -l "FlywayTestConfig" --type java
Length of output: 1735
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/FlywayRepairConfig.java (1)
16-21: LGTM!
Clean implementation with proper dependency injection.
Comment on lines
+12
to
+19
| @Bean | ||
| @Primary | ||
| public ApplicationRunner testFlywayMigrationRunner(Flyway flyway) { | ||
| return args -> { | ||
| flyway.repair(); | ||
| flyway.migrate(); | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for Flyway operations.
The Flyway repair and migrate operations can throw FlywayException. Consider adding appropriate error handling:
@Bean
@Primary
public ApplicationRunner testFlywayMigrationRunner(Flyway flyway) {
return args -> {
- flyway.repair();
- flyway.migrate();
+ try {
+ flyway.repair();
+ flyway.migrate();
+ } catch (FlywayException e) {
+ throw new RuntimeException("Failed to execute Flyway migrations", e);
+ }
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @Bean | |
| @Primary | |
| public ApplicationRunner testFlywayMigrationRunner(Flyway flyway) { | |
| return args -> { | |
| flyway.repair(); | |
| flyway.migrate(); | |
| }; | |
| } | |
| @Bean | |
| @Primary | |
| public ApplicationRunner testFlywayMigrationRunner(Flyway flyway) { | |
| return args -> { | |
| try { | |
| flyway.repair(); | |
| flyway.migrate(); | |
| } catch (FlywayException e) { | |
| throw new RuntimeException("Failed to execute Flyway migrations", e); | |
| } | |
| }; | |
| } |
Comment on lines
+8
to
+14
| import org.springframework.boot.context.event.ApplicationStartingEvent; | ||
| import org.springframework.context.ApplicationListener; | ||
| import org.springframework.context.annotation.Configuration; | ||
|
|
||
| @Configuration | ||
| @Slf4j | ||
| public class FlywayRepairConfig implements ApplicationListener<ApplicationStartingEvent> { |
There was a problem hiding this comment.
Consider using a later lifecycle event for Flyway execution
Using ApplicationStartingEvent is too early in the Spring lifecycle and could cause initialization issues. Consider using ApplicationStartedEvent, CommandLineRunner, or ApplicationRunner instead to ensure proper initialization order.
-import org.springframework.boot.context.event.ApplicationStartingEvent;
+import org.springframework.boot.context.event.ApplicationStartedEvent;
import org.springframework.context.ApplicationListener;
import org.springframework.context.annotation.Configuration;
@Configuration
@Slf4j
-public class FlywayRepairConfig implements ApplicationListener<ApplicationStartingEvent> {
+public class FlywayRepairConfig implements ApplicationListener<ApplicationStartedEvent> {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import org.springframework.boot.context.event.ApplicationStartingEvent; | |
| import org.springframework.context.ApplicationListener; | |
| import org.springframework.context.annotation.Configuration; | |
| @Configuration | |
| @Slf4j | |
| public class FlywayRepairConfig implements ApplicationListener<ApplicationStartingEvent> { | |
| import org.springframework.boot.context.event.ApplicationStartedEvent; | |
| import org.springframework.context.ApplicationListener; | |
| import org.springframework.context.annotation.Configuration; | |
| @Configuration | |
| @Slf4j | |
| public class FlywayRepairConfig implements ApplicationListener<ApplicationStartedEvent> { |
| .baselineOnMigrate(true) | ||
| .baselineVersion("0") | ||
| .load(); | ||
| flyway.repair(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make repair() execution conditional
Since repair() is needed specifically for Spotless-related checksum changes, consider making it conditional to avoid unnecessary execution during every startup.
+ boolean needsRepair = Boolean.getBoolean("flyway.repair");
+ if (needsRepair) {
flyway.repair();
+ log.info("Flyway repair completed");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| flyway.repair(); | |
| boolean needsRepair = Boolean.getBoolean("flyway.repair"); | |
| if (needsRepair) { | |
| flyway.repair(); | |
| log.info("Flyway repair completed"); | |
| } |
Failed server tests
|
abhvsn
requested changes
Nov 5, 2024
There was a problem hiding this comment.
@AnaghHegde let's speed up this, migration script is at risk because of this change.
|
Blocking: #37043 |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
This PR has been closed because of inactivity. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The reason for doing this is that we want to introduce the spotless on the SQL files. Post this, the existing migrations checksum will change and the validation fails because of this. Hence introducing the
flyway.repair()We have considered the other alternatives like
ignoreMigration(not available in OSS), change baseline version(not working out ex ecpected), make flyway to ignore the validation on few files by setting checksum asNULLIn this setup, calling migrate() explicitly after repair() ensures that all pending migrations are applied immediately after any necessary repairs to the schema history. This approach is beneficial because:
Order of Execution: Spring Boot’s default Flyway configuration may automatically call migrate() at application startup if spring.flyway.enabled=true, but this will not account for any issues that repair() might have resolved.
Ensures Continuity: Calling repair() fixes schema history inconsistencies like checksum mismatches, and calling migrate() right afterward ensures that migrations proceed smoothly with any errors addressed first. This can prevent unexpected issues in production environments.
Creating the Flyway Instance: In the configuration, creating the Flyway instance allows you to customize the setup before Spring Boot runs any migrations. Specifically:
Custom Configuration: By creating the Flyway instance yourself, you can set properties directly on it (like baselineVersion, locations, dataSource, etc.) based on what’s defined in application.properties. This setup also gives flexibility to add conditional logic, if needed, for different environments.
Control over Repair and Migration: By defining your own Flyway bean, you gain explicit control over when repair() and migrate() are called and in what order, which can be important if you need to fix schema inconsistencies before applying any migrations.
Automation
/ok-to-test tags=""
🔍 Cypress test results
Warning
Tests have not run on the HEAD c23d8ef yet
Mon, 04 Nov 2024 09:41:52 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
FlywayRepairConfig.UserConfig,ThemesConfig, andSeedMongoData.Bug Fixes
Chores