-
Notifications
You must be signed in to change notification settings - Fork 351
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
Issue 3753 - CDI2 doesn't work with BeanValidation #4219
Conversation
Caused by SuplierBeanBridge returning the same bean ID for all bindings. Updated getBeanClass to return the contract class, making the bean ID to relate to the actual type. Signed-off-by: Pappy <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I confirm this solves #4208.
Unfortunately I found another issue with injection by @context in InjectingConstraintValidatorFactory. :( I'll try to figure out what is going on in cdi2-se. |
Signed-off-by: Răzvan STĂNESCU <[email protected]>
This PR seems to break HelloWorldTest.testHello |
@pa314159 Can you please fix the failing test? |
Signed-off-by: pappy <[email protected]>
@mkarg, @jansupol, pushing that test was on purpose, it's should be fixed now. I have no clue why @context injection doesn't work in InjectingConstraintValidatorFactory - @Inject works, but that's normal. I ended up by instantianting InjectingConstraintValidatorFactory directly, I don't see any reason to be a CDI bean. |
@jansupol How to proceed? |
There is a couple of issues here:
For 1 - 3 @senivam was anxious to look at that in more detail. Nevertheless, I would think {1} and {2,3} would be better to have a separate issue (PRs) for them. For 4, I am not sure a new example would be better. Maybe not. For 5, hopefully, it will be done. |
Please open a PR that clearly describes what it solely is good for and that only covers #4208 but not any other bug. We need this rather soon as this bug and only this bug is planned for inclusion in 2.29.1. Plese open a PR that clearly describes what it solely is good for and that only covers the actually fixed parts of #3753 but not any other bug as this bug is not planned for inclusion in 2.29.1. ...and so. Just to enable Team Jersey to understand what each single fix is good for and to pick each bug fix separately. I know this looks like nitpickery as we certainly could simply cherry-pick the commits separately. But first, this is not how the contribution process works - Team Jersey (and I assume most EE4J teams) picks complete PRs only - and second this could end up in your authorship information getting lost by rewriting commits (e. g. if I would separate your PRs, my name could be shown instead of yours). So we hope you understand why we need this clear process. To speed things up, I could open a PR and cherry-pick the urgent commits, just tell me if you like to go with that. Thanks a lot! :-) |
OK, I agree of the need of traceability and awareness :) So this is what I did: I created PR #4236, where getBeanClass is implemented in SupplierBeanBridge and I related it to issue #4208. There is only one commit that is relevant for both #3753 and #4236. In this case, although it solves the ILA, it doesn't solve the whole BV validation. You will need to decide what to do with #3753, because the title and the description won't match anymore after merging changes from #4236. |
@pa314159 This is no problem. You can open a separate PR right now with both commits as a sequence, but keep it in the draft state (i. e. switch the PR-creation-button's mode from PR to draft), which effectively prevents the merge. Once #4236 is merged, you simply rebase on |
Caused by SuplierBeanBridge returning the same bean ID for all bindings.
Updated getBeanClass to return the contract class, making the bean ID
to relate to the actual type.
Signed-off-by: pa314159 [email protected]