Skip to content

fix: retrieve DependentResource based on name instead of class #1058

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

Merged
merged 20 commits into from
Mar 27, 2022

Conversation

metacosm
Copy link
Collaborator

This causes issues if the class is proxied for any reason (e.g. because
it's a CDI bean).

@metacosm metacosm self-assigned this Mar 21, 2022
@metacosm metacosm requested a review from csviri March 21, 2022 20:55
@scrocquesel
Copy link
Contributor

scrocquesel commented Mar 21, 2022

Could the name be provided by the DependentResourceSpec and set during createAndConfigureFrom ?

  • This would allow to have two dependents of the same type with different name. Related to your question DependentResource CDI awareness #1038 (comment).
  • This would allow to have the real static class name from annotation in logs instead of I guess the proxy one return by getClass.getCanonicalName ?

@scrocquesel
Copy link
Contributor

Could the name be provided by the DependentResourceSpec and set during createAndConfigureFrom ?

  • This would allow to have two dependents of the same type with different name. Related to your question DependentResource CDI awareness #1038 (comment).
  • This would allow to have the real static class name from annotation in logs instead of I guess the proxy one return by getClass.getCanonicalName ?

And name could be latter override by configuration on the annotation if needed

@csviri
Copy link
Collaborator

csviri commented Mar 22, 2022

Looks good so far, my question is also that, the name should be mandatory in the annotation? Or how do you see that?

@sclorng
Copy link

sclorng commented Mar 22, 2022

Looks good so far, my question is also that, the name should be mandatory in the annotation? Or how do you see that?

If not provided, then it should take the type canonical name and raise an error if there is a dup

@csviri
Copy link
Collaborator

csviri commented Mar 22, 2022

Looks good so far, my question is also that, the name should be mandatory in the annotation? Or how do you see that?

If not provided, then it should take the type canonical name and raise an error if there is a dup

Yep, probably a sensible default makes sense in this case.

@metacosm
Copy link
Collaborator Author

Could the name be provided by the DependentResourceSpec and set during createAndConfigureFrom ?

Yes, that's the plan indeed.

  • This would allow to have the real static class name from annotation in logs instead of I guess the proxy one return by getClass.getCanonicalName ?

I think so, yes.

@metacosm
Copy link
Collaborator Author

Looks good so far, my question is also that, the name should be mandatory in the annotation? Or how do you see that?

If not provided, then it should take the type canonical name and raise an error if there is a dup

Yes, that is the plan but this PR is not finished yet. Great to see some early feedback that validates what I was thinking, though! 😄

@metacosm metacosm force-pushed the more-cdi-dependents branch 2 times, most recently from 75c1a4b to eb4e720 Compare March 22, 2022 12:09
@metacosm metacosm marked this pull request as ready for review March 23, 2022 10:04
@metacosm metacosm force-pushed the more-cdi-dependents branch 2 times, most recently from 01015f0 to cc9bae2 Compare March 23, 2022 15:12
@metacosm metacosm changed the base branch from next to main March 23, 2022 15:13
@metacosm metacosm force-pushed the more-cdi-dependents branch from cc9bae2 to 12b9e64 Compare March 23, 2022 15:28
@metacosm metacosm changed the base branch from main to next March 23, 2022 16:11
@metacosm metacosm changed the base branch from next to main March 23, 2022 16:12
@metacosm metacosm changed the base branch from main to next March 23, 2022 17:41
@metacosm metacosm changed the base branch from next to main March 23, 2022 17:41
@metacosm metacosm force-pushed the more-cdi-dependents branch 2 times, most recently from bb073e7 to f1149fa Compare March 23, 2022 21:53
@metacosm metacosm marked this pull request as draft March 23, 2022 22:29
@metacosm
Copy link
Collaborator Author

Still having issues with this, so switching back to draft mode as I uncovered issues that need to be fixed with the event source registration. Basically, the dependent resource name should be propagated to the associated event source if it exists. The MySQL sample is also behaving weirdly…

@metacosm metacosm force-pushed the more-cdi-dependents branch from 22ccb38 to 14c2f56 Compare March 24, 2022 13:01
@metacosm metacosm marked this pull request as ready for review March 24, 2022 15:45
@metacosm metacosm force-pushed the more-cdi-dependents branch from 7f82f54 to f4be7ef Compare March 25, 2022 08:54
*
* @return the resource type associated with this DependentResource
*/
Class<R> resourceType();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why we need this at this level?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To make things explicit so that we don't need to rely on a default implementation that will not work in all situations.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean are we sure that this will be needed in all cases?
I mean I can imaging a dependent resource implementation that does not need resource type.

I thought it is something that is required for quarkus.

I guess we can then delete ResourceTypeAware.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I'm conflicted on this… but the problem is that I haven't had time to spend completely focused on the extension so I'm not 100% sure about it.

@metacosm
Copy link
Collaborator Author

Can we merge this?

@metacosm metacosm force-pushed the more-cdi-dependents branch from ca0f38e to 5cefd44 Compare March 26, 2022 23:26
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

56.8% 56.8% Coverage
0.0% 0.0% Duplication

Copy link
Collaborator

@csviri csviri left a comment

Choose a reason for hiding this comment

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

sure, LGTM!

@metacosm metacosm merged commit 9d36fb3 into main Mar 27, 2022
@metacosm metacosm deleted the more-cdi-dependents branch March 27, 2022 16:37
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.

4 participants