Implement multi-schema support #727
Conversation
philipp-kleber-avelios
requested a review
from filipelautert
as a code owner
philipp-kleber-avelios
temporarily deployed
to
external
GitHub Actions
Inactive
src/main/java/liquibase/ext/hibernate/database/HibernateDatabase.java
Outdated
Show resolved
Hide resolved
filipelautert
force-pushed
the
multi-schema-support
branch
from
7effe3b to
0385016
Compare
filipelautert
temporarily deployed
to
external
GitHub Actions
Inactive
There was a problem hiding this comment.
Approved.
Besides the minor comment I posted this looks good to me. Thank you, @philipp-kleber-avelios for contributing with this PR.
Daniel.
philipp-kleber-avelios
temporarily deployed
to
external
GitHub Actions
Inactive
src/main/java/liquibase/ext/hibernate/snapshot/SequenceSnapshotGenerator.java
Outdated
Show resolved
Hide resolved
philipp-kleber-avelios
had a problem deploying
to
external
GitHub Actions
Failure
filipelautert
force-pushed
the
multi-schema-support
branch
from
21227fc to
6d8764b
Compare
There was a problem hiding this comment.
Thanks for the improvement @philipp-kleber-avelios !
@tati-qalified if you need any ideas on how to test it please let me know.
There was a problem hiding this comment.
Thank you @philipp-kleber-avelios for your contribution!
I was wondering if the entities for the test could be narrowed down, I don't think it's necessary to create 13 classes just for one test case.
| @@ -34,6 +34,10 @@ protected void addTo(DatabaseObject foundObject, DatabaseSnapshot snapshot) thro | |||
| Schema schema = (Schema) foundObject; | |||
| HibernateDatabase database = (HibernateDatabase) snapshot.getDatabase(); | |||
| for (org.hibernate.boot.model.relational.Namespace namespace : database.getMetadata().getDatabase().getNamespaces()) { | |||
| if (!schemaMatchesNamespace(schema, namespace)) { | |||
| continue; | |||
There was a problem hiding this comment.
@philipp-kleber-avelios in this piece of code it's checking if the schema doesn't match the namespace, but then it does nothing either way. Did you mean to add something else inside the if block? If not, it should be removed
| Table table = new Table().setName(hibernateTable.getName()); | ||
| Scope.getCurrentScope().getLog(getClass()).info("Found table " + table.getName()); | ||
| if (!schemaMatchesTable(example, hibernateTable)) { | ||
| Scope.getCurrentScope().getLog(getClass()).info("Skipping table " + hibernateTable.getName() + " for schema " + example.getSchema().getName() + ", because it is part of another one."); |
There was a problem hiding this comment.
The last part because it is part of another one isn't too clear, I'd change it to because it belongs to a different schema.
These changes are an initial experiment to resolve issue #248, by allowing multiple schemas during the snapshot.
I added a new test
HibernateMultiSchemaTest.java, which currently only checks the exit code of such a multi-schemadiffChangelogcommand.If somebody could review the code and maybe give their input if the changes are sufficient or if additional ones are required, that would be great, thanks!.