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

More convenient injection into Java records #832

Open
hantsy opened this issue Dec 25, 2024 · 12 comments
Open

More convenient injection into Java records #832

hantsy opened this issue Dec 25, 2024 · 12 comments
Milestone

Comments

@hantsy
Copy link
Contributor

hantsy commented Dec 25, 2024

CDI @Singleton bean and @Decorator do not generate proxy at runtime, it should allow it to be declared as a record type. And inject dependent beans via constructor injection.

@manovotn
Copy link
Contributor

CDI @singleton bean and @decorator do not generate proxy at runtime

Decorated beans are implemented through subclassing, just like intercepted beans, so this would not really work.

@Singleton alone should work just fine or is there any issue with it?

@hantsy
Copy link
Contributor Author

hantsy commented Dec 26, 2024

When using record type, I must add a cocanial constructor and an extra @Inject to inject the dependent beans.

  1. @Inject can not be applied on the record class level
  2. If possible make all record fields injectable by default without a @inject

@manovotn
Copy link
Contributor

@Inject can not be applied on the record class level

This seems doable, just a few thoughts:

  • Records can also have multiple c-tors so we'd need to check that
  • We do not allow this kind of declaration for any other bean and allowing it just here means creating a special case
    • We could allow this for all beans - so long as they have a single c-tor so CDI can be deterministic; throw error otherwise
  • To specify a special behavior for records, the spec needs to target JDK 14+ (which it doesn't for 4.1 but surely will for 5,x)

If possible make all record fields injectable by default without a @Inject

Not sure what you mean here. The injection into records would need to be through constructors only - all other parts are by definition final and meant to be immutable.

@manovotn manovotn added this to the CDI 5.0 milestone Dec 27, 2024
@manovotn manovotn changed the title Record type support More convenient injection into Java records Dec 27, 2024
@hantsy
Copy link
Contributor Author

hantsy commented Dec 27, 2024

Not sure what you mean here. The injection into records would need to be through constructors only - all other parts are by definition final and meant to be immutable.

What I said here is that when a record class is annotated with @Singletone, all fields in the default constructor are injected automatically without an explicit @Inject.

@manovotn
Copy link
Contributor

I see, that's a little different from what I meant in my previous comment and would require us to special case records for sure.

@Ladicek
Copy link
Contributor

Ladicek commented Jan 2, 2025

When using record type, I must add a cocanial constructor and an extra @Inject to inject the dependent beans.

  1. @Inject can not be applied on the record class level
  2. If possible make all record fields injectable by default without a @inject

This is Java the language decision. They simply don't provide a way to annotate the constructor without actually declaring it. I don't see why CDI / AtInject should try to fix that "problem".

@manovotn
Copy link
Contributor

manovotn commented Jan 2, 2025

This is Java the language decision. They simply don't provide a way to annotate the constructor without actually declaring it. I don't see why CDI / AtInject should try to fix that "problem".

I think the only reason is just convenience.

Special casing records is IMO not worth it.
However, I was thinking we could change the spec to state that for any [discovered] class based bean with a single declared constructor, such c-tor is automatically treated as if it was annotated with @Inject. This would solve the issue presented here with records but would also work for any other bean.
In case said bean has multiple c-tors (or needs to have them for proxyability), we are back to current behavior and explicit @Inject annotation.

Now, I might have missed something but so far I can't think of anything preventing us from doing this.
Thoughts?

@ljnelson
Copy link

ljnelson commented Jan 2, 2025

Historically with a discovery mode of "all" this could cause a lot of beans to exist. I (very dimly) seem to recall in the earliest days of the specification people deciding this was a bad idea. Maybe that's no longer a relevant situation/case; I don't know.

@manovotn
Copy link
Contributor

manovotn commented Jan 2, 2025

Historically with a discovery mode of "all" this could cause a lot of beans to exist. I (very dimly) seem to recall in the earliest days of the specification people deciding this was a bad idea. Maybe that's no longer a relevant situation/case; I don't know.

Hm, that's a fair point, thanks for mentioning that.
It would indeed pick up more classes as bean candidates 🤔

@starksm64
Copy link
Contributor

But with the new default mode of needing a bean defining annotation, it would only pick up classes that someone was trying to use a bean. To date no class with a non-default ctor could in fact be a bean, so the impact of this should tend to zero.

@manovotn
Copy link
Contributor

manovotn commented Jan 3, 2025

But with the new default mode of needing a bean defining annotation, it would only pick up classes that someone was trying to use a bean. To date no class with a non-default ctor could in fact be a bean, so the impact of this should tend to zero.

The all discovery mode still exists and is perfectly valid in CDI Full.
So this change would affect existing applications using that and would be detrimental to all discovery mode in general as it would lower its usability - you cannot really turn that off.

I guess we could further tweak the suggestion in my previous comment and state that this only works for class-based beans that have a bean defining annotation on them?

@Ladicek
Copy link
Contributor

Ladicek commented Jan 3, 2025

Seriously, is

@Dependent
public record Foo(Bar bar) {
    @Inject
    Foo {
    }
}

so much worse than

@Dependent
public record Foo(Bar bar) {
}

?

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

No branches or pull requests

5 participants