-
-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
validate the db name for all three moudles #4496
validate the db name for all three moudles #4496
Conversation
CLA Assistant Lite bot: I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request |
apollo-common/src/main/java/com/ctrip/framework/apollo/common/utils/ConfigValidator.java
Outdated
Show resolved
Hide resolved
apollo-common/src/main/java/com/ctrip/framework/apollo/common/utils/ConfigValidator.java
Outdated
Show resolved
Hide resolved
apollo-common/src/main/java/com/ctrip/framework/apollo/common/utils/ConfigValidator.java
Outdated
Show resolved
Hide resolved
apollo-common/src/main/java/com/ctrip/framework/apollo/common/utils/ConfigValidator.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
private void checkDataBaseName() { | ||
String checkFlag = applicationContext.getEnvironment().getProperty("config.check.databasename"); |
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.
Shall we change the property name to apollo.check.database-name
?
And is it better to use the ConditionalOnProperty
mechanism? e.g.
@ConditionalOnProperty(value = {"apollo.check.database-name"}, havingValue = "true", matchIfMissing = true)
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.
OK, I will revise it in this way.
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.
I want to add logic to check the database name before creating the data source.
If use the @ConditionalOnProperty
to deal with it, there will be a problem. There is a dependency in admin service,
adminServiceAuthenticationFilter
-> bizConfig
-> bizDBPropertySource
-> serverConfigRepository
-> serverConfig
-> entityManager
-> dataSourceScriptDatabaseInitializer
-> dataSource
, when the servlet is initialized, the filter will be initialized first,this will cause the data source to initialize, so unable to add name check before data source initialization. it seems meaningless if the name check is added after the data source is initialized,or is it the wrong way I handled it?
What's the purpose of this PR
validate the db name for all three moudles
Which issue(s) this PR fixes:
Fixes #4230
Brief changelog
If the database names of admin service and config service are missing 'config', or if the database name of portal service is missing 'portal', the startup will fail.
Follow this checklist to help us incorporate your contribution quickly and easily:
mvn clean test
to make sure this pull request doesn't break anything.CHANGES
log.