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-14469 - Support schema-tooling on sub-sets of the relational mode… #3763

Closed
wants to merge 5 commits into from

Conversation

sebersole
Copy link
Member

HHH-14469 - Support schema-tooling on sub-sets of the relational model known to Hibernate
HHH-14470 - Support "hiding" domain references from sources other than ORM

…l known to Hibernate

- Initial commit adding support for tagging the various `Contributable` references (PersistentClass, Exportable)
@sebersole
Copy link
Member Author

This is still a draft. It is the first dev phase implementing the "tagging" of various Contributable objects based on who contributed them (orm, envers, search, etc). I added handling for orm and envers mainly because bootstrapping for those 2 are part of this repo.

@koentsje Wanted you to take a look at the changes to these various Contributable implementation constructors (PersistentClass, Table, etc) to make sure they work for your tooling work.

@Sanne You had brought up the search aspect so wanted to get your take on whether this works for the needs from the search perspective.

Next steps are:

  1. allow define schema tooling different for each contributor (HHH-14469)
  2. by default hide entities defined by contributors other than orm for queries and metadata lookup (HHH-14470)

@sebersole
Copy link
Member Author

@Naros Hoping to get your thoughts on the envers aspects to this. They are pretty trivial, limited to MetadataBuildingProcess when we process enver's AdditionalJaxbMappingProvider

Copy link
Member

@koentsje koentsje left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proof will be in eating the pudding but AFAICS there is nothing insurmountable from the tools perspective

…l known to Hibernate

- @BootstrapServiceRegistry
- Support for filtering at schema-tooling level
@sebersole
Copy link
Member Author

After these last 2 committs. the only piece left is the ability (if we want) to "hide" entities for the purpose of querying and looking up metadata (HHH-14470)

@sebersole sebersole marked this pull request as ready for review March 3, 2021 20:11
@beikov
Copy link
Contributor

beikov commented Mar 4, 2021

There is a compile error currently in case you didn't see it:

/hibernate-orm/hibernate-envers/src/main/java/org/hibernate/envers/enhanced/OrderedSequenceStructure.java:61: error: method does not override or implement a method from a supertype

  @Override
  ^

…l known to Hibernate

- @BootstrapServiceRegistry
- Support for filtering at schema-tooling level
@sebersole
Copy link
Member Author

Fixed the compile error in envers. Thanks @christianbauer

Anyone think hiding entities (query/metamodel) from user is still a good idea? @Sanne you were the one who suggested this part - still have feelings about it?

@beikov
Copy link
Contributor

beikov commented Mar 4, 2021

I personally think that hiding such entities is a bad idea, but since this behavior configurable, I guess it's fine.

@sebersole
Copy link
Member Author

I personally think that hiding such entities is a bad idea, but since this behavior configurable, I guess it's fine.

Not sure what you mean by configurable. There are 2 related features here:

  • limited schema-tooling
  • hiding of entities

Limited schema-tooling is configurable in the same sense that schema-tooling itself is configurable, so there is nothing to see there.

Hiding of entities though would be a completely new concept. We could design to make it configurable, but at the moment nothing like this exists and so is not configurable.

The use case that Sanne brought up was some hibernate-search specific entities. The idea being that at the moment any user can query those search entities and that it might be nice to hide those by default. The user would have to explicitly DoSomething to make them available (we had discussed hints on the Jira, but there are other possible ways)

@sebersole
Copy link
Member Author

The jobs complain about checkstyle violation, but I cannot reproduce that locally 🤷

@beikov
Copy link
Contributor

beikov commented Mar 4, 2021

Well, we already have hibernate.ejb.metamodel.population which can be used to hide envers entities from the JPA metamodel. I guess what you are talking about would be an extension to that?

@beikov
Copy link
Contributor

beikov commented Mar 4, 2021

You can download the report and view it locally. Here is the error:

/home/runner/work/hibernate-orm/hibernate-orm/hibernate-testing/src/main/java/org/hibernate/testing/orm/junit/BootstrapServiceRegistry.java

Error Description Line
Javadoc comment at column 17 has parse error. Missed HTML close tag 'T'. Sometimes it means that close tag missed for one of previous tags. 33
Javadoc comment at column 27 has parse error. Details: no viable alternative at input '<S extends T' while parsing HTML_ELEMENT 37

@sebersole
Copy link
Member Author

Well, we already have hibernate.ejb.metamodel.population which can be used to hide envers entities from the JPA metamodel. I guess what you are talking about would be an extension to that?

No. That setting simply says how to deal with the JPA "static metamodel" (aka, the Entity_ stuff) - how should we handle populating that. It has nothing to do with whether we expose that entity for queries or metadata look up.

You can download the report and view it locally.

Failures you cannot reproduce locally are not useful

@Sanne
Copy link
Member

Sanne commented Mar 4, 2021

Hi, sorry I missed previous notifications.

Yes I do still think that being able to "hide" technical entities from e.g. user queries would be useful. Interestingly I believe @yrodiere and @fax4ever are right now exploring that feature in Search that could benefit from this.

I also wonder if it could be useful for the hypotethical new 2LC design, but TBH wasn't able to think much more about it so it's all a bit vague still.

@beikov
Copy link
Contributor

beikov commented Mar 4, 2021

No. That setting simply says how to deal with the JPA "static metamodel" (aka, the Entity_ stuff) - how should we handle populating that. It has nothing to do with whether we expose that entity for queries or metadata look up.

I believe you mean the similarly named yet different property hibernate.jpa.static_metamodel.population. The one I mentioned configures, if the JPA Metamodel reports Envers entities as javax.persistence.metamodel.EntityType.

Failures you cannot reproduce locally are not useful

That's odd, the GH actions job just runs ./gradlew check. Maybe you are running just compile?

@sebersole
Copy link
Member Author

Failures you cannot reproduce locally are not useful

Nevermind. I missed that it is in hibernate-testing. I was only checking core and envers. Man I really dislike this Javadoc limitation

Copy link
Member

@dreab8 dreab8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looks great to me 👍

@sebersole
Copy link
Member Author

I believe you mean the similarly named yet different property hibernate.jpa.static_metamodel.population. The one I mentioned configures, if the JPA Metamodel reports Envers entities as javax.persistence.metamodel.EntityType.

I assume you mean hibernate.ejb.metamodel.population=ignoreUnsupported?

That does not work. First it does not do even what you think it does. I can look up the dynamic entity from the JPA model no problem. I have a test that shows this

And regardless, even if it did do what you say... it would still be available from the mapping model

…l known to Hibernate

- @BootstrapServiceRegistry
- Support for filtering at schema-tooling level
@beikov
Copy link
Contributor

beikov commented Mar 4, 2021

Hmm, so ignoreUnsupported, which is the default, skips adding EntityType instances to the JPA Metamodel which have no bindable java type like e.g. Envers entities. I see that the setting does a lot more than that, although just for the JPA metamodel. You are right though that the model is still accessible through the Hibernate native Metamodel which I assume is what is desired to not happen except for special internal cases.

@sebersole
Copy link
Member Author

Nope, I'm saying that it is also available from the JPA model. From the test, this line fails

The subsequent assertion wrt the mapping model would fail also

@beikov
Copy link
Contributor

beikov commented Mar 4, 2021

Indeed, this seems to have changed on wip/6.0. I was looking at the state on master.

@sebersole
Copy link
Member Author

Indeed, this seems to have changed on wip/6.0. I was looking at the state on master.

Not to mention it would still miss things like envers' DefaultRevisionEntity object which is a real class

@yrodiere
Copy link
Member

yrodiere commented Mar 4, 2021

Yes I do still think that being able to "hide" technical entities from e.g. user queries would be useful. Interestingly I believe @yrodiere and @fax4ever are right now exploring that feature in Search that could benefit from this.

Yes, Fabio is working on that. Basically we need to declare an entity used internally by Hibernate Search to publish and consume events. That entity is of no concern to users, so we'd rather hide it to avoid impacting other integrations that process metadata (JPAModelgen, ...). I suppose not making the entity available in the metamodel API would be nice, too. We will need queries, though, so if they are not allowed by default, we would need a way to override that in Hibernate Search

@sebersole
Copy link
Member Author

Yes I do still think that being able to "hide" technical entities from e.g. user queries would be useful. Interestingly I believe @yrodiere and @fax4ever are right now exploring that feature in Search that could benefit from this.

Yes, Fabio is working on that. Basically we need to declare an entity used internally by Hibernate Search to publish and consume events. That entity is of no concern to users, so we'd rather hide it to avoid impacting other integrations that process metadata (JPAModelgen, ...). I suppose not making the entity available in the metamodel API would be nice, too. We will need queries, though, so if they are not allowed by default, we would need a way to override that in Hibernate Search

The associated Jira discusses that part using hints

@sebersole
Copy link
Member Author

Before we keep growing this discussion, let's start a Discussion :)

#3787

@sebersole
Copy link
Member Author

I'm going to close this PR and integrate its changes upstream. The work here is limited to HHH-14469. The continuing discussion is specific to HHH-14470 so no need to hold this up

@sebersole
Copy link
Member Author

Applied upstream

@sebersole sebersole closed this Mar 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants