Skip to content
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

HHH-18977 proposal for @DefaultSchema #4654

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

gavinking
Copy link
Member

@gavinking gavinking commented Jan 17, 2022

This PR introduces an @DefaultSchema annotation, allowing setting the schema at the package or outer class level.

Thoughts?

See https://hibernate.atlassian.net/browse/HHH-18977

@hibernate hibernate deleted a comment from hibernate-github-bot bot Jan 17, 2022
@beikov
Copy link
Member

beikov commented Jan 17, 2022

Dunno, I always thought that hard-coding a schema name is bad practice and one should rather use a variable that is substituted.

@gavinking
Copy link
Member Author

gavinking commented Jan 17, 2022

@beikov it sounds like you're thinking about a different problem, though I can only speculate what it might be.

The issue this proposal seeks to solve is that there are currently only two levels of granularity for specifying a schema:

  • global to the SessionFactory, or
  • in a particular @Table annotation.

The global setting strikes me as pretty useless, since you can usually handle that with a JDBC parameter. (But perhaps that's not true for all databases?)

The table-level granularity is almost never what I want.

Instead, the sort of thing I often have is, say, 5 entities as inner classes of a test class, and sometimes I get table name collisions with inner classes used by other tests. So it would be nice to be able to easily give each test class its own schema.

Similarly, I would sometimes like to be able to assign all entities in a package to one schema.

I don't see how this as "hardcoding" any more than specifying a table or column name is "hardcoding". It's using a schema to disambiguate name collisions, just like we use packages for this in Java.

@beikov
Copy link
Member

beikov commented Jan 17, 2022

Well, I was treating a schema more like a "tenant namespace" in the past, but I understand that it can also be used for grouping of logically connected tables, so that's fine.

@gavinking
Copy link
Member Author

Yep, using schemas for tenant segregation is all good, but a completely different usecase.

@gavinking
Copy link
Member Author

Note that the reason that the tests are failing here is that this PR depends on a change to commons-annotations: hibernate/hibernate-commons-annotations#18

@Sanne
Copy link
Member

Sanne commented Jan 18, 2022

Instead, the sort of thing I often have is, say, 5 entities as inner classes of a test class, and sometimes I get table name collisions with inner classes used by other tests. So it would be nice to be able to easily give each test class its own schema.

Similarly, I would sometimes like to be able to assign all entities in a package to one schema.

These points strike a chord with me, definitely +1 for such benefits with testing

@Sanne
Copy link
Member

Sanne commented Jan 18, 2022

BTW, somehow related: we occasionally talk about the need to eventually be able to run the testsuite in parallel. Today we run paralle jobs for different database dialects, but it might be useful to run also tests from the same DB Dialect in sub-groups.

Having a declarative way expressing which schema a certain test will use should be useful to get into that sort of direction.

@sebersole
Copy link
Member

Having a declarative way expressing which schema a certain test will use should be useful to get into that sort of direction.

I think we want to be careful here. Part of the reason (a large part) the tests take so long is because they are constantly creating and dropping schemas. Simply segmenting packages or other groups of tests to create and drop even more schemas is only going to make that aspect worse.

Would the supposed performance benefit of being able to run the tests in parallel outweigh this aspect? Not sure.

@sebersole
Copy link
Member

sebersole commented Jan 18, 2022

Not to mention, for tests I think it might be better to have this automated in some fashion rather than relying on specific annotations like this. Think about something like the standard domain models we use in the test suite. Those won't be able to effectively use these dialect override annotations. To be able to properly parallelize tests that use these standard domain models we'd really need some kind of injected way to specify the schema and/or catalog rather than relying on the domain model to include the alternate schemas.

This discussion is kind of beyond the topic of this pull request, so I'll keep it brief. My intention with those standard domain models was always to pre-export them for the entire test suite rather than recreating them for every test that uses them. Which I think will help the performance of the test suite overall. I think the tests that use those standard domain models should just be forced to run in sequence. Others that use one-off models can certainly benefit from segmenting them into different schemas and/or catalogs

@gavinking
Copy link
Member Author

FTR, I was more thinking of Hibernate Reactive's test suite, which is not quite as "extreme" as core.

@Sanne
Copy link
Member

Sanne commented Jan 18, 2022

Hi Steve, yes good points.

A unified schema would certainly be effective, but to get there it might still be useful to be able to logically group those tests belonging to such schema - ofc this could be done in a number of ways, but this one feature could be useful to get there.

Also I'm confident that while many tests could use such an approach, we'll always have also a significant number of tests which will need alternative strategies - schema isolation would help for some (and yet not all) of these.

@Sanne
Copy link
Member

Sanne commented Jan 18, 2022

ah, only saw your second comment after posting - sorry that might be confusing.

I agree with your ideas, that might be even better. Still, this shouldn't hurt to get there.

to allow setting the schema at the package or outer class level
@hibernate hibernate deleted a comment from hibernate-github-bot bot Jan 21, 2022
@gavinking
Copy link
Member Author

So, there's a problem with this: there's no good way for the previous-generation id generators to get access to the @DefaultSchema of the entities they belong to.

There might be in the new infrastructure that's currently incubating, but that's not been implemented yet.

So I guess I need to put this on ice for now.

@gavinking gavinking marked this pull request as draft December 20, 2024 17:00
@gavinking gavinking changed the title proposal for @DefaultSchema HHH-18977 proposal for @DefaultSchema Dec 20, 2024
@hibernate-github-bot
Copy link

hibernate-github-bot bot commented Dec 20, 2024

Thanks for your pull request!

This pull request does not follow the contribution rules. Could you have a look?

❌ All commit messages should start with a JIRA issue key matching pattern HHH-\d+
    ↳ Offending commits: [3f73c18, 4bd0ce4]

› This message was automatically generated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants