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
There was a problem hiding this comment.
@philipp-kleber-avelios Why getConnectionCatalogName() and getConnectionSchemaName() are being locked here to constants?
There was a problem hiding this comment.
For getConnectionCatalogName(), the behavior is the same as before just without one less indirection (it used to call getDefaultCatalogName() internally).
For getConnectionSchemaName(), the behavior is slightly different from before:
It used to call getDefaultSchemaName() internally, but we can't do this anymore: The new getDefaultSchemaName() implementation calls super.getDefaultSchemaName() and this then calls getConnectionSchemaName(), so we'd have a call-cycle.
| @@ -34,6 +34,12 @@ 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()) { | |||
| boolean namespaceMatchesSchema = (namespace.getName().getSchema() != null && namespace.getName().getSchema().matches(foundObject.getName())) | |||
There was a problem hiding this comment.
This check is duplicated in SequenceSnapshotGenerator and TableSnapshotGenerator. Could you extract it to a generic method in HibernateSnapshotGenerator , or even move the check there so it's not repeated? As I believe it would be necessary to IndexSnapshotGenerator and other SnapshotGenerators too.. or am I overreacting?
There was a problem hiding this comment.
Sure, I can extract the check. I don't think we need to move the check though, since from my understanding, DatabaseObjects other than Schema, Table and Sequence are added only from those three corresponding snapshot-generators.
So I think if we have the check in those three generators, we don't need the check in the other ones, since the database objects that get to them always already have the correct schema.
I might be wrong though, I don't fully understand the snapshotting mechanism.
philipp-kleber-avelios
requested a deployment
to
external
GitHub Actions
Waiting
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!.