quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.56k stars 2.62k forks source link

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

Closed tandraschko closed 1 year ago

tandraschko commented 5 years ago

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

geoand commented 5 years ago

cc @manovotn @mkouba

mkouba commented 5 years ago

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.

tandraschko commented 5 years ago

I think the handling in MyFaces is also bit weird:

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 commented 5 years ago

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.

mkouba commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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:

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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