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

Enlist repository parameterized types for reflection #6326

Merged

Conversation

loicmathieu
Copy link
Contributor

Fixes #6324

Enlist for relfection types parameters from interfaces and super classes.
This fixes the issue with the repository implementation where the repository type parameter (it should be the MongoDB persisted class) is not registered for reflection so it can fails if not direct usage is made that GraalVM can be aware of.

I didn't climb up the hierarchy of interfaces and super classes so it's still possible to have relfection issue for complex type hierarchies. I don't now if it worth it to implement it.

gsmet
gsmet previously requested changes Dec 23, 2019
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Can you fix the commit message? relfection. Thanks.

@gsmet gsmet changed the title Enlist repository parameterized types for relfection Enlist repository parameterized types for reflection Dec 23, 2019
@loicmathieu loicmathieu force-pushed the fix/mongodb-panache-reflective-class branch from 279dbc6 to de19993 Compare December 23, 2019 15:23
@loicmathieu
Copy link
Contributor Author

@gsmet commit message updated

gsmet
gsmet previously requested changes Dec 23, 2019
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

We need to take into account hierarchy of superclasses. It happens and dealing with the superclass only is clearly not sufficient.

Or did I miss something?

@machi1990
Copy link
Member

We need to take into account hierarchy of superclasses. It happens and dealing with the superclass only is clearly not sufficient.

Or did I miss something?

+1 with @gsmet here

@loicmathieu
Copy link
Contributor Author

@gsmet hierarchy of super classes where the direct super classes is not the parameterized one seems to be not very common but I can handle it.
But this will need to wait for a few days ...

@loicmathieu loicmathieu force-pushed the fix/mongodb-panache-reflective-class branch from de19993 to bde979a Compare December 27, 2019 10:09
@loicmathieu
Copy link
Contributor Author

@gsmet @machi1990 I climb up the hierarchy of super types in the PR.

@gsmet gsmet dismissed their stale review January 3, 2020 17:11

Comments addressed.

@gsmet
Copy link
Member

gsmet commented Jan 3, 2020

I will let @FroMage check that one and decide of its fate :).

@FroMage
Copy link
Member

FroMage commented Jan 6, 2020

I don't understand this PR. The test you added is for when an entity type is never registered via reflection. This can only happen for Mongo, right? And only because you don't have an equivalent to @Entity that ORM uses to register those types. So for Mongo they become "unknown" unless they extend MongoPanacheEntity.

So, perhaps you should have a @MongoEntity annotation?

Alternately, what you want to do is use JandexUtil.resolveTypeParameters(DotName.createSimple(userRepoSubclassName), DotName.createSimple(MongoPanacheRepositoryBase.class.getName()), index) and get the right type parameter that points to the entity.

@loicmathieu
Copy link
Contributor Author

@FroMage yes, when you define "mongodb entities" it's not mandatory to add the @MongoEntity annotation that only exist to define the database name (if not comming from application.properties) and the collection name (by default the name of the class).

We can ask to always add the annotation, but as it's not enforced inside the framework, situations will always exist where a user will not add it ...

I will refactory with JandexUtil.resolveTypeParameters().

@FroMage
Copy link
Member

FroMage commented Jan 6, 2020

We can ask to always add the annotation, but as it's not enforced inside the framework, situations will always exist where a user will not add it ...

Well, you can enforce it ;)

It would be more consistent with ORM, but on the other hand this appears to be the first use-case for requiring it, and we can solve it by scanning type params, so probably we can keep that strategy for now and keep an eye out for new use-cases that would require that annotation.

@loicmathieu
Copy link
Contributor Author

@FroMage updated with the usage of JandexUtil.resolveTypeParameters()

Copy link
Member

@FroMage FroMage left a comment

Choose a reason for hiding this comment

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

Look @geoand we're all using this method now :)

@geoand
Copy link
Contributor

geoand commented Jan 6, 2020

Excellent, you all know who to direct your rage at if things don't work :P

@loicmathieu
Copy link
Contributor Author

@gsmet ready to be merged :)

@loicmathieu
Copy link
Contributor Author

@geoand
Copy link
Contributor

geoand commented Jan 15, 2020

Let's merge

@geoand geoand merged commit bf7bbf6 into quarkusio:master Jan 15, 2020
@geoand geoand added this to the 1.2.0 milestone Jan 15, 2020
@loicmathieu loicmathieu deleted the fix/mongodb-panache-reflective-class branch January 15, 2020 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MongoDB with Panache didn't enlist for reflection Repository parameterized type
5 participants