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

Improve wording of managed bean requirements WRT non-static public fields #660

Closed
manovotn opened this issue Mar 9, 2023 · 2 comments · Fixed by #698
Closed

Improve wording of managed bean requirements WRT non-static public fields #660

manovotn opened this issue Mar 9, 2023 · 2 comments · Fixed by #698
Milestone

Comments

@manovotn
Copy link
Contributor

manovotn commented Mar 9, 2023

The specification text in question can be seen here:

If a managed bean has a non-static public field, it must have scope @Dependent. If a managed bean with a non-static public field declares any scope other than @Dependent, the container automatically detects the problem and treats it as a definition error.

From my understanding, this is for prevention of a user error where you could try to access a field directly on a proxy instance. And since (client) proxies are stateless, the value of this field if undefined.

This makes sense but it is incomplete for it only allows these fields on @Dependent beans but @Singleton would be perfectly fine as well. I suggest we change the wording to only forbid these fields on normal scoped beans, i.e. all beans that require client proxy.

As a side note, this check only forbids public fields but you can theoretically encounter the same issue with protected or pack private fields. However forbidding those is too restrictive, I just wanted to mention it because I am not totally clear on why we do this check in the first place 🤷

Cc @mkouba @Ladicek

@manovotn manovotn added this to the CDI 4.1/5.0 milestone Mar 9, 2023
@manovotn manovotn changed the title Improve wording of managed bean requirements WRT non-static public methods Improve wording of managed bean requirements WRT non-static public fields Mar 9, 2023
@Ladicek
Copy link
Contributor

Ladicek commented Mar 9, 2023

FTR, the fact that public fields on @Singleton beans lead to a definition error actually prevents passing the AtInject TCK. So the current wording is overly restrictive. I believe we should either ban public fields only on normal scoped beans, as @manovotn suggests, or at the very least allow them on @Singleton beans as well (in addition to @Dependent).

@mkouba
Copy link
Contributor

mkouba commented Mar 9, 2023

I suggest we change the wording to only forbid these fields on normal scoped beans, i.e. all beans that require client proxy.

👍

As a side note, this check only forbids public fields but you can theoretically encounter the same issue with protected or pack private fields. However forbidding those is too restrictive, I just wanted to mention it because I am not totally clear on why we do this check in the first place

I agree that this would be too restrictive...

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 a pull request may close this issue.

3 participants