[DAT-16377] Verify if supports method is implemented in a change to prevent unexpected behaviors #5903
Conversation
…it's not part of the default liquibase changes
| //we only verify supports method if this is not part of the default liquibase changes | ||
| for (Change plugin : plugins) { | ||
| String packageName = plugin.getClass().getPackage().getName(); | ||
| if (packageName.startsWith("liquibase.change") || packageName.startsWith("com.datical.liquibase.ext")) { |
There was a problem hiding this comment.
Should I keep pro package here or should we verify it too?
There was a problem hiding this comment.
I think that is okay to keep it out.
There was a problem hiding this comment.
In fact I think you should remove it since it's an extension.
This reverts commit 3b47101.
liquibase-cli/src/test/groovy/liquibase/integration/commandline/LiquibaseCommandLineTest.groovy
Outdated
Show resolved
Hide resolved
| //we only verify supports method if this is not part of the default liquibase changes | ||
| for (Change plugin : plugins) { | ||
| String packageName = plugin.getClass().getPackage().getName(); | ||
| if (packageName.startsWith("liquibase.change") || packageName.startsWith("com.datical.liquibase.ext")) { |
There was a problem hiding this comment.
In fact I think you should remove it since it's an extension.
| Controls the level of validation performed on the | ||
| supports method of Change classes. Options are | ||
| OFF, WARN, FAIL. | ||
| DEFAULT: WARN |
There was a problem hiding this comment.
Should the default be FAIL so that we notice if we violate this rule when we run tests in all of the extensions? If we just warn, we probably won't ever actually notice until runtime, at which point it will be too late.
There was a problem hiding this comment.
We could change to FAIL, but this could also break existing extensions. I think we should start with WARN for 1-2 releases and then switch to FAIL - I can create a dat ticket for that.
There was a problem hiding this comment.
Please do and put it in a future sprint.
This reverts commit 5b89de9.
| if (packageName.startsWith("liquibase.change")) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Do we even want this? Thinking more about this, don't we want all changes to be verified? If so, then I think this method just introduces another way for an extension to accidentally override the supports method and us not notice.
liquibase-standard/src/main/java/liquibase/change/ChangeFactory.java
Outdated
Show resolved
Hide resolved
…y.java typo Co-authored-by: Steven Massaro <[email protected]>
Impact
Description
Things to be aware of