Add TCK tests for SyntheticBeanBuilder.withInjectionPoint()

[manovotn]

Fixes #699

These are created based on a set of tests I was first developing for Weld implementation.

For starters, it is a draft as I need to take a second look at it myself but I can't really focus on it anymore for now, so I'll try to do that tomorrow :) (EDIT: done)
I tried to cover all of the various cases I could think of but I could definitely use @Ladicek to take a look if there is any other case you had in mind.

One thing I am not really comfortable with is the fact that we now want impls to always throw exception if you ask for a type/qualifier combination that wasn't previously stated as required.
It makes sense for build-time approach with optimizations, but while implementing this in Weld, I basically have to put in place restrictions that don't make much sense - there is nothing preventing me from giving user access to the bean he previously didn't state if the bean exists.
To me, the main value lies in early validation of the requested bean in the CDI app but I am not sure I am happy with preventing user from accessing other beans should they wish to 🤷
WDYT @Ladicek @Azquelt, wouldn't it make more sense to require the early validation but allow to resolve any bean at your own risk?

[manovotn]

The PR is now ready for review.

WRT jakartaee/cdi#964 - the tests here can be easily adapter (just removing the workaround) to account for the new variant. I just kept it as-is to have it build and be able to test it without snapshots.

[manovotn]

I wasn't sure what to add to the audit file as we don't really cover these parts in the spec but instead do so in the javadoc. I am open to better suggestions :)

[Ladicek]

What you have here looks good to me. I wouldn't lose sleep over this :-)

[Ladicek]

One thing I am not really comfortable with is the fact that we now want impls to always throw exception if you ask for a type/qualifier combination that wasn't previously stated as required. It makes sense for build-time approach with optimizations, but while implementing this in Weld, I basically have to put in place restrictions that don't make much sense - there is nothing preventing me from giving user access to the bean he previously didn't state if the bean exists. To me, the main value lies in early validation of the requested bean in the CDI app but I am not sure I am happy with preventing user from accessing other beans should they wish to 🤷 WDYT @Ladicek @Azquelt, wouldn't it make more sense to require the early validation but allow to resolve any bean at your own risk?

I didn't review this PR yet, but I want to reply to this: I very much disagree. The design here aligns with producer methods. If you want to lookup arbitrary beans, you have to inject Instance<Object> explicitly. It is possible, but you don't get it out of the box, you have to ask for it.

In other words, you're basically pushing back to the Instance<Object> approach we had originally. We can just revert the PR that adds withInjectionPoint(), if that's what you wish, but you can't have it both ways :-)

[manovotn]

In other words, you're basically pushing back to the Instance<Object> approach we had originally. We can just revert the PR that adds withInjectionPoint(), if that's what you wish, but you can't have it both ways :-)

Not really @Ladicek

We are deprecating the previous approach so I assume that at some point in the future, we will no longer have that method whatsoever. And that's OK, I take into consideration that we only retain this approach.

More importantly, I understand the value build-time approach gets from the validation of those injection points.
In Quarkus, that is what allows us to take note of what beans are required and cannot be optimized away.

However, do you actually get any value from enforcing runtime failure on a bean that is present anyway? Genuine question because I don't think you do. And BTW this is not just a custom Foo bean that you added elsewhere but also built-in beans like BeanContainer. Perhaps I am just missing something though :)

[Ladicek]

Well, if you honestly think it's good for the specification to allow different behaviors of such a basic thing as dependency injection in different implementations...

[manovotn]

Well, if you honestly think it's good for the specification to allow different behaviors of such a basic thing as dependency injection in different implementations...

Not sure I follow? That's not what I said.

[Ladicek]

This is exactly what you're suggesting in

I am not sure I am happy with preventing user from accessing other beans should they wish to

[Azquelt]

One thing I am not really comfortable with is the fact that we now want impls to always throw exception if you ask for a type/qualifier combination that wasn't previously stated as required. It makes sense for build-time approach with optimizations, but while implementing this in Weld, I basically have to put in place restrictions that don't make much sense - there is nothing preventing me from giving user access to the bean he previously didn't state if the bean exists.

I would argue that the main value is consistency and portability. It makes it harder to write an extension that e.g. works on Weld but doesn't work on Quarkus.

[manovotn]

I would argue that the main value is consistency and portability. It makes it harder to write an extension that e.g. works on Weld but doesn't work on Quarkus.

But that's not portability between Lite and Full, just between CDI and Quarkus where Quarkus' optimization contradicts other spec requirements 🤷

This is exactly what you're suggesting in

I am not sure I am happy with preventing user from accessing other beans should they wish to

Still nope. You can already access any bean you want to via Instance all over the place.
Adding a hard check here won't change that.

I guess I don't like that the two APIs (PE and BCE) are just diverging in their capabilities here - PE allows you to do the validation as well (you can add IPS which are used just for validation which is exactly like withInjectionPoints) but you can then resolve any bean while BCE is slapping your fingers eagerly.

Of course I understand that without this, the requirement to declare IPs isn't enforceable - I was just questioning the usability of the API from any implementation that does not optimize away unused beans.
And assuming we didn't enforce the failure, impls that want to have this optimization could still attempt the resolution and if the bean isn't present, they could blow up with precise exception detailing on what went wrong.

That said, I won't insist on this and seeing as others are OK with this API, we can keep it as is - it's just that I wanted to share my experience when I was writing these TCKs :)

[Ladicek]

I would argue that the main value is consistency and portability. It makes it harder to write an extension that e.g. works on Weld but doesn't work on Quarkus.

But that's not portability between Lite and Full, just between CDI and Quarkus where Quarkus' optimization contradicts other spec requirements 🤷

No it doesn't. We currently have what could arguably be described as a bug in Quarkus where we ignore the presence of BCE-registered synthetic beans during unused beans removal, because any BCE-registered synthetic bean would automatically disable unused beans removal, because SyntheticBeanCreator unconditionally gives users an Instance<Object>.

We could fix that bug, but I would argue using injection points here is a more elegant solution: we don't just validate that the IPs can be satisfied, we can also enforce that the SyntheticBeanCreator cannot obtain more than it declared. This is precisely what all other kinds of beans do as well, and if they need dynamic lookup, they need to inject Instance<Object> explicitly. I believe synthetic beans should be more consistent with other beans, not less. In other words, PE's produceWith() is an exception, not a rule.

This is exactly what you're suggesting in

I am not sure I am happy with preventing user from accessing other beans should they wish to

Still nope. You can already access any bean you want to via Instance all over the place. Adding a hard check here won't change that.

Providing an Instance<Object> to all SyntheticBeanCreators was a straightforward way to bridge BCEs with PEs, but other than that, it was a mistake. I would even argue it was a mistake when it got added to PEs via produceWith(), it's just that noone cared enough for precision.

I guess I don't like that the two APIs (PE and BCE) are just diverging in their capabilities here - PE allows you to do the validation as well (you can add IPS which are used just for validation which is exactly like withInjectionPoints) but you can then resolve any bean while BCE is slapping your fingers eagerly.

PEs and BCEs have always been wildly different in their capabilities, notably because there's a rift between deployment time and runtime and only a limited form of objects can be transferred through that rift. This is not an issue -- it is what enables build-time implementations.

Also, stop focusing on validation; withInjectionPoint() is way more than that. It is exactly like declaring an IP in a managed bean or producer method. Validation is a nice side effect, but the core value is that it makes synthetic bean closer to other beans.

Of course I understand that without this, the requirement to declare IPs isn't enforceable - I was just questioning the usability of the API from any implementation that does not optimize away unused beans. And assuming we didn't enforce the failure, impls that want to have this optimization could still attempt the resolution and if the bean isn't present, they could blow up with precise exception detailing on what went wrong.

If you like the previous API better, just say so :-)

[Ladicek]

These 2 don't look right. The creator and disposer don't create/destroy the TrackedBean explicitly, so it behaves as a regular dependent instance of the synthetic bean. Therefore, it lives as long as the synthetic bean lives, and these values should both be 1, not 2. Unless I'm missing something...?

[manovotn]

Aha, so my expectation here differ. I would expect new instance of the dependent bean in both cases (and that's what I did in Weld impl too).
The reason being, it is aligned with what happens if you attempt the same in producer/disposer combination. There, you get a new dependent bean instance in both cases.

EDIT: And it would also be different to what you'd get in PE where a similar case will always lead to two different beans 🤔

[Ladicek]

You are right and I was wrong. This is indeed how producers/disposers work and how synthetic beans should also work. What you have is correct here and the SyntheticInjections javadoc is incorrect.

I retract my comment here and will submit a spec PR to clarify.

[Ladicek]

Hold on... why do we still assert assertEquals(TrackedBean.destroyedCount.get(), 0); after the creation function completes? I'm utterly confused now. Let me re-read the spec once again.

[Ladicek]

OK, I got it. So for producer methods, all @Dependent objects injected into a producer method are dependent objects of the produced instance and so are destroyed when the instance is destroyed. For disposer methods, all @Dependent objects are destroyed when the invocation completes.

The same happens for synthetic beans, so this test is correct. Let me just once again take a look at the BCE javadoc to make sure this is aligned.

[Azquelt]

That looks right to me.

One difference between synthetic beans and producer methods that sticks out to me here, is that for the producer method I'd expect two separate injection points, one for the producer method and another for the disposer method, whereas in this synthetic bean case there's only one injection point but we obtain a bean for it twice. I think this difference is ok.

[Ladicek]

Tests mostly LGTM, baring a few comments. One more general (not even asking to change, although I won't stop you :-) ): if the implementation class of a synthetic bean is FooBar, by convention, the creator is FooBarCreator and disposer is FooBarDisposer. A lot of tests in this PR don't follow that, which makes them harder to read.

[manovotn]

I have addressed the remarks excluding the one with dependent bean instance being shared between creator and disposer. While implementable, this would be different from all the other places in CDI which I think would be quite surprising.

[Azquelt]

I'm approving what's here, but I've noted some test cases I think it would be good to cover in the comments.

[manovotn]

I have added a separate commit (to be squashed) with the tests suggested by @Azquelt.
Let me know if that hits the spot.

[Azquelt]

Thanks 👍