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

Qute: access to beans without name in qute templates #41932

Closed
maxandersen opened this issue Jul 16, 2024 · 38 comments
Closed

Qute: access to beans without name in qute templates #41932

maxandersen opened this issue Jul 16, 2024 · 38 comments
Labels
area/qute The template engine kind/enhancement New feature or request

Comments

@maxandersen
Copy link
Member

Description

when using qute web templates or even plain qute templates default beans without a name is not accessible.

i.e. in java code I can do @Inject UserInfo user; but not possible in Qute.

You can do tricks like:

@TemplateExtension
public class Globals {
    public static UserInfo user() {
        return Arc.container().instance(UserInfo.class);
    }
}

but after zulip discussion: https://quarkusio.zulipchat.com/#narrow/stream/187038-dev/topic/qute.20web.20.2F.20how.20to.20.40named.20default.20beans.3F/near/451771103 suggestion on supporting things like the following:

{@io.quarkus.oidc.UserInfo user}

if user not available as parameter inject it.

OR explicit inject in header

{@inject io.quarkus.oidc.UserInfo user}

OR enhance {cdi:} to support

{cdi:UserInfo.name}

to look at build time provided names/types.

Implementation ideas

No response

@maxandersen maxandersen added the kind/enhancement New feature or request label Jul 16, 2024
Copy link

quarkus-bot bot commented Jul 16, 2024

You added a link to a Zulip discussion, please make sure the description of the issue is comprehensive and doesn't require accessing Zulip

This message is automatically generated by a bot.

@quarkus-bot quarkus-bot bot added the area/qute The template engine label Jul 16, 2024
Copy link

quarkus-bot bot commented Jul 16, 2024

/cc @mkouba (qute)

@mkouba
Copy link
Contributor

mkouba commented Jul 26, 2024

So if I understand it correctly it's not about access to named beans but about access to beans with no name. I will change the title of the issue. Feel free to update the title if needed.

@mkouba mkouba changed the title access to default named beans in qute typesafe templates Qute: access to beans without name in qute templates Jul 26, 2024
@FroMage
Copy link
Member

FroMage commented Jul 29, 2024

I dislike the {@io.quarkus.oidc.UserInfo user} and {@inject io.quarkus.oidc.UserInfo user} syntax because unless you're going to re-use that bean, making a variable for it is a lot of ceremony, especially if you need to go up to the start of the template to add it there and come back.

I propose to expand the cdi: namespace resolver so that the following work:

  • {cdi:UserInfo} if there is a @Named("UserInfo")
  • {cdi:UserInfo} if there is a single bean of (unqualified) type UserInfo
  • {cdi:io.quarkus.oidc.UserInfo} if there is a matching FQN bean of that type

@mkouba
Copy link
Contributor

mkouba commented Jul 29, 2024

  • {cdi:io.quarkus.oidc.UserInfo} if there is a matching FQN bean of that type

👎 I think that it's a misuse of the syntax. Qute ref guide is pretty clear that the dot notation is used to access properties or call (virtual) methods.

Also it would not work for parameterized types; e.g. if there's a bean with type List<String>.

If we want to support unnamed beans in templates it should be clear that we're doing a type lookup, e.g. something like {cdi:byType('UserInfo')} and {cdi:byType('io.quarkus.oidc.UserInfo')} (byType | lookup | find | ...). I know it's more typing and not very nice but the proposal above IMO hides too much and we will have to cover a lot of edge cases. For example, detect a failure when there's a bean with name UserInfo and type UserInfo.

@FroMage
Copy link
Member

FroMage commented Jul 29, 2024

Qute ref guide is pretty clear that the dot notation is used to access properties or call (virtual) methods.

That documentation can be updated :)

Also it would not work for parameterized types; e.g. if there's a bean with type List.

We could argue that those require the longer alternative {cdi:'org.foo.Bar<Gee>'}.

If we want to support unnamed beans in templates it should be clear that we're doing a type lookup, e.g. something like {cdi:byType('UserInfo')} and {cdi:byType('io.quarkus.oidc.UserInfo')} (byType | lookup | find | ...). I know it's more typing and not very nice

I could get behind {cdi-type:UserInfo} I think that'd be fine.

For example, detect a failure when there's a bean with name UserInfo and type UserInfo

That's easy, the resolution rules are: @Named first, then unqualified type if unambiguous, then FQN. Simple rules.

@ia3andy
Copy link
Contributor

ia3andy commented Jul 30, 2024

If we want to support unnamed beans in templates it should be clear that we're doing a type lookup, e.g. something like {cdi:byType('UserInfo')} and {cdi:byType('io.quarkus.oidc.UserInfo')} (byType | lookup | find | ...). I know it's more typing and not very nice but the proposal above IMO hides too much and we will have to cover a lot of edge cases. For example, detect a failure when there's a bean with name UserInfo and type UserInfo.

I like this solution where when you want something not named, you lookup. The others seem more hacky and confusing (having yet another namespace).

@mkouba
Copy link
Contributor

mkouba commented Sep 30, 2024

Also it would not work for parameterized types; e.g. if there's a bean with type List.

By the way, we completely missed the CDI qualifiers. Which is not an uncommon thing in CDI apps. And qualifiers can have binding members. This all makes the lookup a lot more complex. In other words, all the proposed solutions would only solve some of the use cases.

I will repeat my answer from the zulip discussion:

@Named
public class Names {

   @Inject
   UserInfo userInfo;

   public UserInfo userInfo() {
       return userInfo;
   }
}

and then {cdi:names.userInfo} in the template. Names can inject any number of beans for which @Named is missing. It can define qualifiers etc. This is IMO the best workaround. All the proposed simplifications are IMO insufficient.

@mkouba mkouba closed this as not planned Won't fix, can't repro, duplicate, stale Sep 30, 2024
@FroMage
Copy link
Member

FroMage commented Sep 30, 2024

🤷

@maxandersen
Copy link
Member Author

@mkouba you lost me - how does qualifiers change this ? If there are ambiguities it is completely expected you need to specify something additionally.

Please show where the suggested solutions are Weaker / worse than @Inject UserInfo test; ?

@mkouba
Copy link
Contributor

mkouba commented Oct 4, 2024

@mkouba you lost me - how does qualifiers change this ? If there are ambiguities it is completely expected you need to specify something additionally.

Please show where the suggested solutions are Weaker / worse than @Inject UserInfo test; ?

If there is a bean UserInfo with a qualifier with a member like:

@ApplicationScoped
@PayBy(CHEQUE)
public class UserInfo {
}

Then you can't simply do @Inject UserInfo (because this injection point has implicit @Default qualifier) but use @Inject @PayBy(CHEQUE) UserInfo instead. In other words, the type is not enough. That's what I mean when I say that "This all makes the lookup a lot more complex.".

The workaround proposed would require a minimal change:

@Named
public class Names {

   @Inject
   @PayBy(CHEQUE)
   UserInfo userInfo;

   public UserInfo userInfo() {
       return userInfo;
   }
}

@FroMage
Copy link
Member

FroMage commented Oct 7, 2024

While I would prefer allowing access by type name, I can accept the time is not yet right.

But when I see code like this:

@Named
public class Names {

   @Inject
   @PayBy(CHEQUE)
   UserInfo userInfo;

   public UserInfo userInfo() {
       return userInfo;
   }
}

It feels like boilerplate. The method has no purpose beyond a name. The field is only there to specify a qualifier.

So… hear me out for a shorter proposal:

@Named
public record NamedCdiBeans(@PayBy(CHEQUE) UserInfo userInfo) {
}

Where the record is just a placeholder for things that can be named and have qualifiers. @Inject is implied (after all, it's @Named). And we get the extra bonus that the names must be unique (at least within this record. not perfect and global, but a good start).

WDYT?

Interfaces or classes work too:

@Named
public class NamedCdiBeans {

   @PayBy(CHEQUE)
   UserInfo userInfo;
}
@Named
public interface NamedCdiBeans {

   @PayBy(CHEQUE)
   UserInfo userInfo();
}

But at least here, we don't have boilerplate (beyond the group): we attach names to CDI types (and qualifiers) and nothing superfluous can be removed.

@mkouba
Copy link
Contributor

mkouba commented Oct 7, 2024

The Java record works if you add a valid scope @Dependent/@Singleton:

@Singleton
@Named
public record NamedCdiBeans(@PayBy(CHEQUE) UserInfo userInfo) {
}

Note

The workaround with @Named class I mentioned above would not work unless a scope is used. I forgot that @Named is not a bean defining annotation. I think that we could add the default scope automatically 🤔 , i.e. a named record is @Singleton or something like that.

The class works if you add a scope and make the field public:

@Singleton
@Named
public class NamedCdiBeans {

   @PayBy(CHEQUE) // this will require @Inject if quarkus.arc.auto-inject-fields=false
   public UserInfo userInfo;
}

The @Named interface does not work and it would be confusing because it's not a CDI bean.

@FroMage
Copy link
Member

FroMage commented Oct 7, 2024

Oh, so, when you say they work, you mean they do produce a valid named bean for userInfo, so I can use it in Qute as inject:userInfo? Why does it need @Singleton when you example code did not need it?

@mkouba
Copy link
Contributor

mkouba commented Oct 7, 2024

Oh, so, when you say they work, you mean they do produce a valid named bean for userInfo, so I can use it in Qute as inject:userInfo? Why does it need @Singleton when you example code did not need it?

Of course not! It works in the sense that you can use the named bean to access the injected bean - you need to use the name first, i.e. {cdi:namedCdiBeans.userInfo}. Otherwise it would not make any sense from the CDI POV.

@mkouba
Copy link
Contributor

mkouba commented Oct 7, 2024

Why does it need @Singleton when you example code did not need it?

Pls read the note in my comment ;-).

@FroMage
Copy link
Member

FroMage commented Oct 7, 2024

Ah, so we could do something better then, and find a way to make them available without naming the container…

Something like:

@NamedContainer
public record NamedCdiBeans(@PayBy(CHEQUE) UserInfo userInfo, Something something) {
}

Which would translate (semantically) to:

  • Make the UserInfo bean with @PayBy(CHEQUE) qualifier available under name userInfo
  • Make the Something bean with default qualifier available under name something

@mkouba
Copy link
Contributor

mkouba commented Oct 7, 2024

Ah, so we could do something better then, and find a way to make them available without naming the container…

Something like:

@NamedContainer
public record NamedCdiBeans(@PayBy(CHEQUE) UserInfo userInfo, Something something) {
}

Which would translate (semantically) to:

  • Make the UserInfo bean with @PayBy(CHEQUE) qualifier available under name userInfo
  • Make the Something bean with default qualifier available under name something

Hm, this would require additional validation - to verify the beans are available, to verify there are no duplicate names, etc. Also it would not allow users to specify a different name, e.g. another annotation would be required if you want to use uInfo instead of userInfo (@Named cannot be used because the orginal bean has no bean).

I do understand the need to associate an existing bean with a name (without @Named declared on the bean class) but we have to find a way that would make sense in the context of CDI, and possibly reuse all existing mechanisms for validation etc.

@Ladicek @manovotn guys do you have an idea because it could be a more general CDI feature.

@FroMage
Copy link
Member

FroMage commented Oct 7, 2024

Hm, this would require additional validation - to verify the beans are available,

I'm sure you already have validation for that.

to verify there are no duplicate names,

Same, I'm sure you already validate that.

Also it would not allow users to specify a different name, e.g. another annotation would be required if you want to use uInfo instead of userInfo (@nAmed cannot be used because the orginal bean has no bean).

Of course you can specify a different name, by changing the name of the parameter :)

@NamedContainer
public record NamedCdiBeans(@PayBy(CHEQUE) UserInfo uInfo, Something something) {
}

@Ladicek
Copy link
Contributor

Ladicek commented Oct 7, 2024

If you make @NamedContainer (I would call it @ExportByNames or something like that, but whatever) a @Stereotype that defines a default scope (@Singleton probably makes most sense here I guess?), dependency resolution should happen automatically. The remaining issue is creating names for the resolved beans, which I honestly think is a terrible idea, but if you leave it in Qute, I don't mind :-)

@Ladicek
Copy link
Contributor

Ladicek commented Oct 7, 2024

One more thing. I'm not sure if I understand correctly, but if you want

@NamedContainer
public record Names(@PayBy(CHEQUE) UserInfo uInfo, Something something) {
}

to let you write {cdi:Names.userInfo}, then (in addition to the previous comment) I'd also put put @Named on @NamedContainer, and that would be everything. That should actually already work today.

If you want {cdi:userInfo}, then I think that's a terrible idea, because beans should not get qualifiers from an external source. But, if you leave it in Qute, I don't mind :-)

@manovotn
Copy link
Contributor

manovotn commented Oct 7, 2024

If you make @NamedContainer (I would call it @ExportByNames or something like that, but whatever) a @Stereotype that defines a default scope (@Singleton probably makes most sense here I guess?), dependency resolution should happen automatically.

+1 for named stereotype with a default scope as that will naturally trigger all our validations.
You could also specify a custom name for the "container" bean through that annotation.

@manovotn
Copy link
Contributor

manovotn commented Oct 7, 2024

If you want {cdi:userInfo}, then I think that's a terrible idea, because beans should not get qualifiers from an external source. But, if you leave it in Qute, I don't mind :-)

I'd say at that point you can have annotation transformer (or CDI extension) adding the @Named annotation to those beans with whatever name you wish 🤷

@Ladicek
Copy link
Contributor

Ladicek commented Oct 7, 2024

Yeah, exactly. I just realized we already have one external source of metadata. I wouldn't want to add another.

@mkouba
Copy link
Contributor

mkouba commented Oct 7, 2024

If you want {cdi:userInfo}, then I think that's a terrible idea, because beans should not get qualifiers from an external source. But, if you leave it in Qute, I don't mind :-)

I'd say at that point you can have annotation transformer (or CDI extension) adding the @Named annotation to those beans with whatever name you wish 🤷

Yes, extensions have that possibility. We're talking about user code.

And yes, that's what Stef and others want - to be able to access beans that have no name and @Named cannot be added because the source is not in our control.

So we need a simple API that would be translated into an annotation transformer.

@mkouba
Copy link
Contributor

mkouba commented Oct 7, 2024

Hm, this would require additional validation - to verify the beans are available,

I'm sure you already have validation for that.

@FroMage We do but only if it's used as regular CDI constructs... which is not the case here.

to verify there are no duplicate names,

Same, I'm sure you already validate that.

Also it would not allow users to specify a different name, e.g. another annotation would be required if you want to use uInfo instead of userInfo (@nAmed cannot be used because the orginal bean has no bean).

Of course you can specify a different name, by changing the name of the parameter :)

Ah, yes, That would work.

@NamedContainer
public record NamedCdiBeans(@PayBy(CHEQUE) UserInfo uInfo, Something something) {
}

@manovotn
Copy link
Contributor

manovotn commented Oct 7, 2024

So we need a simple API that would be translated into an annotation transformer.

CDI Lite extension?
You could just add the annotation from there, it can reside directly in user code and it translates into annotation transformer :)

public class MyExtension implements BuildCompatibleExtension {

    @Enhancement(types = SomeBean.class)
    public void enhance(ClassConfig config) {
        config.addAnnotation(Named.class);
    }
}

Plus the service provider entry of course. You are then free to name all the beans that you don't own in any way you like.

But frankly speaking, the workaround with records is IMO even better - sure, you need to use {cdi:Names.userInfo} instead of {cdi:userInfo} but that's doesn't seem very different 🤷

@FroMage
Copy link
Member

FroMage commented Oct 8, 2024

But frankly speaking, the workaround with records is IMO even better - sure, you need to use {cdi:Names.userInfo} instead of {cdi:userInfo} but that's doesn't seem very different 🤷

If that works, I agree that's totally fine.

But… you're saying this already works ATM?

@Stereotype
@Named
@Singleton
public @interface NamedContainer {}

@NamedContainer
public record Names(@PayBy(CHEQUE) UserInfo uInfo, Something something) {
}

And I can do {cdi:Names.uInfo}?

@mkouba
Copy link
Contributor

mkouba commented Oct 8, 2024

But frankly speaking, the workaround with records is IMO even better - sure, you need to use {cdi:Names.userInfo} instead of {cdi:userInfo} but that's doesn't seem very different 🤷

If that works, I agree that's totally fine.

But… you're saying this already works ATM?

@Stereotype
@Named
@Singleton
public @interface NamedContainer {}

@NamedContainer
public record Names(@PayBy(CHEQUE) UserInfo uInfo, Something something) {
}

And I can do {cdi:Names.uInfo}?

You can do {cdi:names.uInfo} (not Names) because the name is defaulted.

And yes, the stereotype should just work. It's effectively:

@Named
@Singleton
public record Names(@PayBy(CHEQUE) UserInfo uInfo, Something something) {
}

@FroMage
Copy link
Member

FroMage commented Oct 8, 2024

Well, that's very nice! We should document that, and/or provide the stereotype annotation (if we can find a good name).

Does this work for you, @maxandersen @ia3andy ?

@mkouba
Copy link
Contributor

mkouba commented Oct 8, 2024

Well, that's very nice! We should document that, and/or provide the stereotype annotation (if we can find a good name).

Does this work for you, @maxandersen @ia3andy ?

Hm, yes we should definitely document this. I'll send a PR later today.

As for the stereotype - maybe something like @TemplateBeans?

@TemplateBeans
public record User(@PayBy(CHEQUE) UserInfo info, Something something) {
}

And in the template {cdi:user.info} and {cdi:user.something}.

@FroMage
Copy link
Member

FroMage commented Oct 8, 2024

@TemplateBeans is fine, I suppose, though @NamedBeans comes to mind too…

@mkouba
Copy link
Contributor

mkouba commented Oct 8, 2024

@TemplateBeans is fine, I suppose, though @NamedBeans comes to mind too…

What if we instead of using a new stereotype simply state that if quarkus-qute is present any Java record annotated with @Named is singleton by default?

@Named // @Singleton added automatically
public record User(@PayBy(CHEQUE) UserInfo info, Something something) {
}

So there's no need for a new annotation name?

@Ladicek @manovotn @ia3andy @maxandersen

@FroMage
Copy link
Member

FroMage commented Oct 8, 2024

Works for me :)

@manovotn
Copy link
Contributor

manovotn commented Oct 8, 2024

What if we instead of using a new stereotype simply state that if quarkus-qute is present any Java record annotated with @nAmed is singleton by default?

That might interfere with user defined beans that they just put @Named on without any scope.
Although I do realize it would take some specific setup for it to matter.

Then again, we could also just keep the bean @Dependent which should work even now just with @Named in place, right?

@mkouba
Copy link
Contributor

mkouba commented Oct 8, 2024

What if we instead of using a new stereotype simply state that if quarkus-qute is present any Java record annotated with @nAmed is singleton by default?

That might interfere with user defined beans that they just put @Named on without any scope. Although I do realize it would take some specific setup for it to matter.

How could it interfere? If you annotate a record with @Named but add no scope then the record is not recognized as a bean (unless you add the record with AdditionalBeanBuildItem).

Then again, we could also just keep the bean @Dependent which should work even now just with @Named in place, right?

I will use AutoAddScopeBuildItem which does not add the default scope (@Singleton in my case) if a scope annotation is already present. And yes, @Dependent should work just fine.

@manovotn
Copy link
Contributor

manovotn commented Oct 8, 2024

How could it interfere? If you annotate a record with @nAmed but add no scope then the record is not recognized as a bean (unless you add the record with AdditionalBeanBuildItem).

Ugh, don't mind me. For some reason I thought we pick up class based beans even if they only declare a qualifier 🤷

I will use AutoAddScopeBuildItem which does not add the default scope (@singleton in my case) if a scope annotation is already present. And yes, @dependent should work just fine.

Yea, that sounds good 👍

mkouba added a commit to mkouba/quarkus that referenced this issue Oct 8, 2024
- so that it can be easily used as an intermediate CDI bean for beans
that are not annotated with jakarta.inject.Named
- related to quarkusio#41932
@mkouba
Copy link
Contributor

mkouba commented Oct 8, 2024

I've sent a PR: #43775

mkouba added a commit to mkouba/quarkus that referenced this issue Oct 8, 2024
- so that it can be easily used as an intermediate CDI bean for beans
that are not annotated with jakarta.inject.Named
- related to quarkusio#41932
bschuhmann pushed a commit to bschuhmann/quarkus that referenced this issue Nov 16, 2024
- so that it can be easily used as an intermediate CDI bean for beans
that are not annotated with jakarta.inject.Named
- related to quarkusio#41932
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/qute The template engine kind/enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants