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
refactor ValidatingVisitor to use factory pattern, error if strict is set and defaults file does not exist (DAT-15920) #5814
Conversation
@filipelautert @suryaaki @wwillard7800 @abrackx I would appreciate some opinions on the breaking changes introduced in this PR. Are they problematic? If so, should I see if I can reduce the number of breaking changes (at the cost of making the new interface have a not ideal class name). |
liquibase-standard/src/main/java/liquibase/changelog/visitor/StandardValidatingVisitor.java
Fixed
Show fixed
Hide fixed
liquibase-standard/src/main/java/liquibase/changelog/visitor/StandardValidatingVisitor.java
Fixed
Show fixed
Hide fixed
liquibase-standard/src/main/java/liquibase/changelog/visitor/StandardValidatingVisitor.java
Fixed
Show fixed
Hide fixed
liquibase-standard/src/main/java/liquibase/changelog/visitor/StandardValidatingVisitor.java
Fixed
Show fixed
Hide fixed
liquibase-standard/src/main/java/liquibase/changelog/visitor/ValidatingVisitor.java
Fixed
Show fixed
Hide fixed
liquibase-standard/src/main/java/liquibase/changelog/visitor/StandardValidatingVisitor.java
Fixed
Show fixed
Hide fixed
liquibase-standard/src/main/java/liquibase/changelog/visitor/StandardValidatingVisitor.java
Fixed
Show fixed
Hide fixed
liquibase-standard/src/main/java/liquibase/changelog/visitor/StandardValidatingVisitor.java
Fixed
Show fixed
Hide fixed
liquibase-standard/src/main/java/liquibase/changelog/visitor/StandardValidatingVisitor.java
Fixed
Show fixed
Hide fixed
liquibase-standard/src/main/java/liquibase/changelog/visitor/StandardValidatingVisitor.java
Fixed
Show fixed
Hide fixed
@StevenMassaro I searched in our extensions and seems no one overrides or uses it. It makes sense as ValidatingVisitor is at the core of liquibase. Perhaps adding javadoc to the new interface ValidatingVisitor providing directions on how to use it would be enough? |
I agree with @filipelautert . I think if we provide instructions on how to get their customized |
liquibase-standard/src/main/java/liquibase/changelog/visitor/StandardValidatingVisitor.java
Fixed
Show fixed
Hide fixed
liquibase-standard/src/main/java/liquibase/changelog/visitor/StandardValidatingVisitor.java
Fixed
Show fixed
Hide fixed
Signed-off-by: Steven Massaro <steven.massaro.web@gmail.com>
FOSSA Snippets Detection 🤖
|
FOSSA Test 🧪
|
…different method" This reverts commit aadb1b7.
This approach is not ideal. Closing in favor of a new approach. |
FOSSA Test 🧪
|
FOSSA Snippets Detection 🤖
|
FOSSA Test 🧪
|
FOSSA Snippets Detection 🤖
|
FOSSA Test 🧪
|
FOSSA Snippets Detection 🤖
|
FOSSA Test 🧪
|
@filipelautert Can you rereview this? I changed the implementation to not be a breaking change. This should improve things considerably. |
/** | ||
* Other implementations of this class might optionally provide additional validations to do in this method. | ||
*/ | ||
protected void additionalValidations(ChangeSet changeSet, Database database, boolean shouldValidate, boolean ran) { |
Check notice
Code scanning / CodeQL
Useless parameter Note
/** | ||
* Other implementations of this class might optionally provide additional validations to do in this method. | ||
*/ | ||
protected void additionalValidations(ChangeSet changeSet, Database database, boolean shouldValidate, boolean ran) { |
Check notice
Code scanning / CodeQL
Useless parameter Note
/** | ||
* Other implementations of this class might optionally provide additional validations to do in this method. | ||
*/ | ||
protected void additionalValidations(ChangeSet changeSet, Database database, boolean shouldValidate, boolean ran) { |
Check notice
Code scanning / CodeQL
Useless parameter Note
/** | ||
* Other implementations of this class might optionally provide additional validations to do in this method. | ||
*/ | ||
protected void additionalValidations(ChangeSet changeSet, Database database, boolean shouldValidate, boolean ran) { |
Check notice
Code scanning / CodeQL
Useless parameter Note
/** | ||
* An interface for generating validating visitors, which are used to validate changesets. | ||
*/ | ||
public interface ValidatingVisitorGenerator extends Plugin { |
Check notice
Code scanning / CodeQL
Constant interface anti-pattern Note
Plugin
Can you change the pr description to reflect these new changes? |
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.
Well, that's an even better approach!
FOSSA Snippets Detection 🤖
|
FOSSA Test 🧪
|
Impact
Description
Refactor the
ValidatingVisitor
into the factory pattern to support pro usages.Things to be aware of
Things to worry about
Additional Context