Skip to content

fix(#4218): avoid duplicate injection on fields #5224

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

Open
wants to merge 2 commits into
base: 2.x
Choose a base branch
from

Conversation

giulong
Copy link
Contributor

@giulong giulong commented Jul 13, 2025

After a quite deep analysis on #4218, I ended up implementing what was also suggested in this comment.

This way, when there are both creator properties and fields with the same injection, we mark the annotated field to be ignored when creating the corresponding injectable.
We ignore fields instead of constructor properties considering that constructor injection happens before field injection, and users may need the injected value in the constructor to do any additional logic on their side.

This seems to solve #2678 as well.

Waiting for feedbacks and improvements

…sponding injected constructor parameter is available
@@ -816,6 +816,9 @@ protected void addInjectables(DeserializationContext ctxt,

for (Map.Entry<Object, AnnotatedMember> entry : raw.entrySet()) {
AnnotatedMember m = entry.getValue();
if (m.isIgnoreInjection()) {
Copy link
Contributor Author

@giulong giulong Jul 13, 2025

Choose a reason for hiding this comment

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

We avoid creating a redundant ValueInjector for fields whose injection was already performed via constructor properties.

*
* @since 2.20
*/
protected boolean _ignoreInjection;
Copy link
Member

@cowtowncoder cowtowncoder Jul 15, 2025

Choose a reason for hiding this comment

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

AnnotatedMembers cannot have per-call state (one instance created per AnnotatedClass; concurrent readValue() calls would have conflict).

So this cannot be done.

Ignore above, I misread code.

@cowtowncoder
Copy link
Member

I like the concept, although would prefer not adding configuring state in AnnotatedMember, if that could be avoid. I'll have to think about this when I have time.
But I think we can work with this, thank you @giulong !

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.

2 participants