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

Synthetic beans / BeanRegistrarBuildItem doesn't support injection of the produces item like CDI producers #3699

Closed
tandraschko opened this issue Aug 26, 2019 · 15 comments
Labels
area/arc Issue related to ARC (dependency injection) kind/enhancement New feature or request

Comments

@tandraschko
Copy link
Contributor

Description
Syntetic beans / BeanRegistrarBuildItem doesn't support injection of the produces item like CDI producers
e.g. https://github.com/tandraschko/quarkus-myfaces/blob/master/quarkus-myfaces/runtime/src/main/java/io/quarkus/myfaces/runtime/producer/SimpleBeanCreator.java

Implementation ideas

@tandraschko tandraschko added the kind/enhancement New feature or request label Aug 26, 2019
@tandraschko tandraschko changed the title Syntetic beans / BeanRegistrarBuildItem doesn't support injection of the produces item like CDI producers Synthetic beans / BeanRegistrarBuildItem doesn't support injection of the produces item like CDI producers Aug 26, 2019
@geoand
Copy link
Contributor

geoand commented Aug 26, 2019

cc @manovotn @mkouba

@mkouba
Copy link
Contributor

mkouba commented Aug 26, 2019

Well, synthetic/custom beans in CDI do not support injection as the extension controls the implementation of Contextual.create(CreationalContext<T>). For normal scoped beans it's ok to use CDI.current().select() or Arc.container().instance() methods inside the BeanCreator#create() method. For dependent beans you would have to handle @PreDestroy callbacks correctly.

In theory, we could try to enhance the BeanCreator API so that an implementation could use an Instance<Object> directly.

@mkouba mkouba added the area/arc Issue related to ARC (dependency injection) label Aug 26, 2019
@tandraschko
Copy link
Contributor Author

tandraschko commented Aug 26, 2019

I think the handling in MyFaces is also bit weird:

  • we collect all classes annotated with @FacesConverter (they are probably already scanned by CDI and are beans as @FacesConverter is a @Qualifier)
  • for each converter, we create a synthetic bean again with @Dependent scope

i think for this special case, we should avoid to create a synthetic bean again and i will try to refactor MyFaces here

However, in generell, i think providing such a mechanism could be a benefit in future

@manovotn
Copy link
Contributor

The addition of beans in Quarkus is designed to be as close to CDI's original means as possible. And even in CDI you cannot really have injectable params for when you add synthetic bean.

The approach with Instance that Martin mentions could work but I think that it lies with user to handle destroy of dependent instance properly. And at that point, you might as well just get the instance yourself. But maybe I am missing some bits.

@stale stale bot added the stale label Nov 13, 2019
@maxandersen maxandersen removed the stale label Nov 13, 2019
@quarkusio quarkusio deleted a comment from stale bot Apr 2, 2020
@mkouba
Copy link
Contributor

mkouba commented Apr 2, 2020

The approach with Instance that Martin mentions could work but I think that it lies with user to handle destroy of dependent instance properly.

So we could follow the logic used for normal injection (including producers), i.e. that @Dependent instances will be dependent objects of the synthetic bean and thus destroyed when the synthetic bean instance is destroyed.

We could add a default method to the BeanCreator:

default T create(CreationalContext<T> creationalContext, Map<String, Object> params, Instance<Object> instance) {
  return create(creationalContext, params);
}

@tandraschko @manovotn WDYT?

@manovotn
Copy link
Contributor

manovotn commented Apr 2, 2020

That sounds good. But if you grant user access to the Instance, they might handle destruction themselves. Which means you cannot know if they did it already or if you should.

Also, keeping both methods opens up a way to implement both differently and we wouldn't know which one to invoke (well, we'd invoke the new one anyway but still). I know it doesn't make sense to do that, but it introduces another possible user error - if we add this method, we should probably deprecate the previous one and eventually delete it.

@mkouba
Copy link
Contributor

mkouba commented Apr 2, 2020

Which means you cannot know if they did it already or if you should.

That is not a problem. A destroyed @Dependent bean is removed from the relevant CreationalContext and so if there are no @Dependent instances left then InstanceImpl.destroy() is just a no-op.

well, we'd invoke the new one anyway but still

Yes, we would always call a new one and the default impl would be to call the old one. The only problem I see is that we would have to make the old one also default (it's very similar to the ObserverMethod.notify(EventContext<T>) in CDI 2.0). It's not very nice but I don't think it's a big issue.

we should probably deprecate the previous one and eventually delete it.

That's also an option.

@manovotn
Copy link
Contributor

We are relatively close:tm: to supporting CDI 4 Lite extension SPI as means to create synthetic beans in Quarkus 3.
The standardized means to create a bean will be via SyntheticBeanCreator which already offers Instance as its parameter and should achieve what this issue describes.

With that in mind, can we close this issue @mkouba?

@mkouba
Copy link
Contributor

mkouba commented Nov 25, 2022

With that in mind, can we close this issue @mkouba?

I don't think so. We don't plan to switch to CDI 4 Lite extension SPI in quarkus extensions, i.e. build items will remain the preffered and idiomatic way of registering synthetic beans in quarkus.

That said, it's IMO a minor issue with low priority.

@manovotn
Copy link
Contributor

Sorry, I wasn't clear. I know build items will be there but I assume we'll adapt them in some way so that the Lite extension API actually goes through them. @Ladicek probably knows more as he has a draft stored somewhere.
With that in mind, we can (or will even have to?) adjust Quarkus API to meet the requirement stated here as well.

@Ladicek
Copy link
Contributor

Ladicek commented Nov 25, 2022

My implementation of BCExtensions currently doesn't use the BeanCreator / BeanDestroyer API, it uses the bytecode generation API instead (for both creation and destruction functions). That could change relatively easily, as all the heavy lifting happens earlier.

In fact, I could probably change it right away, because I have introduced a special method InstanceImpl.forSynthesis() to obtain the Instance<Object> for programmatic lookup, based on given CreationalContext (which the BeanCreator / BeanDestroyer already have), and everything else is already there. That method could probably be used (after some refactoring) for providing a lookup object to BeanCreator / BeanDestroyer as well. See https://github.com/Ladicek/quarkus-jakarta-arc/blob/main/independent-projects/arc/runtime/src/main/java/io/quarkus/arc/impl/InstanceImpl.java#L44 and https://github.com/Ladicek/quarkus-jakarta-arc/blob/main/independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/cdi/build/compatible/extensions/ExtensionsEntryPoint.java#L205 if you feel adventurous :-)

@Ladicek
Copy link
Contributor

Ladicek commented Nov 25, 2022

Oh and btw, I personally think that the BCExtensions API is in many ways better than the ArC extension API and the Quarkus build items. It "just" has 2 significant downsides:

  • no direct access to bytecode generation (only synthetic beans and observers may carry build-time data to runtime)
  • less complete (mainly, it doesn't allow transforming beans, observers or injection points)

The 2nd issue can be partially solved when it comes to CDI concepts (but not when it comes to ArC / Quarkus concepts, obviously) (and probably will, in time), the 1st... not so much.

@mkouba
Copy link
Contributor

mkouba commented Nov 28, 2022

Oh and btw, I personally think that the BCExtensions API is in many ways better than the ArC extension API and the Quarkus build items. It "just" has 2 significant downsides:

* no direct access to bytecode generation (only synthetic beans and observers may carry build-time data to runtime)

* less complete (mainly, it doesn't allow transforming beans, observers or injection points)

The 2nd issue can be partially solved when it comes to CDI concepts (but not when it comes to ArC / Quarkus concepts, obviously) (and probably will, in time), the 1st... not so much.

IMO the most important downside is that you can't use the build items as "input" and "output", e.g. register a synthetic bean if a build item is produced or produce a build item if a bean with a specific type is present. Pls correct me if I'm wrong and this is possible. Also note that the very same problem was present in CDI portable extensions. Basically, it's a very nice API if an extension is very CDI-centric and does not care much about the other parts of the stack.

@Ladicek
Copy link
Contributor

Ladicek commented Nov 28, 2022

I tried to to say that by the edit to my last sentence, but you're right, it should have been an extra item in the list. I didn't make a proper difference between comparing with the ArC extension API and with the Quarkus extension API. The power of BCExtensions, when implemented in ArC, is limited by the power of ArC extensions. That said, I believe (though I have yet to try it myself) that a lot of CDI-based frameworks can achieve 50% - 90% of their integration needs just with BCExtensions.

Pls correct me if I'm wrong and this is possible.

Actually I think it might be possible. The design of BCExtensions allows for custom, non-portable extension points. A @Synthesis method, for example, could declare a parameter of type SyntheticComponents (which is standard, CDI-defined), and also another parameter of a custom, proprietary type that would interface with the Quarkus extension API in some way. I'm not sure if we wanna do that, but it should be doable to some extent.

@mkouba
Copy link
Contributor

mkouba commented Apr 17, 2023

The synthetic injection points API is the preferred solution in Quarkus.

@mkouba mkouba closed this as completed Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/arc Issue related to ARC (dependency injection) kind/enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants