Arc - remove wildcard types from bean types of class-based beans #30447
Conversation
|
The failures looks related. In fact, it seem that HV is maybe using bean types as a basic for validation. |
This comment has been minimized.
This comment has been minimized.
|
Hey @manovotn. Hibernate Validator uses annotations on type parameters to configure value extractors (which can be CDI beans): It would seem that your change is somehow removing the wildcard and corresponding annotation in the code above, leading HV to not detect the value extractor, leading (later) to a failure because HV cannot extract values from a Does that help? EDIT: Here's the full stacktrace of the failure, for future reference: |
|
Oops, the link to the code was truncated. I edited my comment. |
|
Ah but wait, I found that in the HV code: Maybe that hack is no longer necessary, and doing a simple request with |
|
@manovotn Changing @Singleton
public static class SingletonContainerValueExtractor
implements ValueExtractor<Container<@ExtractedValue ?>> {
@Override
public void extractValues(Container<?> originalValue, ValueReceiver receiver) {
receiver.value("someName", originalValue.value);
}
} Is simply ignored and not added to the CDI context... At least I don't see it in Maybe you should adjust your code to not ignore bean supertypes that contain wildcards, but just erase them (i.e. register the raw supertype |
|
@yrodiere according to CDI spec:
Remaining types should be preserved. There is a TCK test (and the test I added here with the |
|
@manovotn Does that TCK test expect the 3 types to be If it's the former, that's exactly what I was asking for and it would work with HV. If it's the latter, I don't really see the point of that limitation, since it would only remove a feature that users possibly rely on currently (and thus break backwards compatibility, by the way). Not sure we want to be that strict, at least not by default? Configuration flags to enable strict spec compliance come to mind. Alternatively, I suppose we could enable type matching ( If we want to break compatibility instead, in the specific case of Hibernate Validator, we could use jandex to list all types extending |
|
@yrodiere the TCK test explicitly checks that I also brought this up with @mkouba and @Ladicek to get some more CDI expertise on this but the conclusion is what I am telling you.
That decision predates my engagement with the spec so I can only assume it's because of some typesafe resolution cases which would otherwise not work correctly. Note that there has to be similar limitation in WFLY integration. Weld is for sure passing CDI TCKs so it will have the same "problem". I'd assume HV had to deal with it there as well?
This was basically an oversight. We already did similar bean type removal for producer method/field we just forgot to apply this to class beans. With that in mind it's not as much of a breaking change as it is fixing an inconsistency of our behavior. |
It does not. It checks that
WildFly doesn't allow declaring
It can be both. On that note, I'll summon @gsmet to give his opinion: should I change HV to use Jandex to workaround this limitation we're introducing in Arc, or should we do something else? |
|
Well, I can't say I'm excited about this... But if you all are saying what we do is against spec, I suppose we need to go the Jandex way. |
|
Note that this is related to #28558 which is a tracker issue for (eventual) CDI Lite compliance in Quarkus 3.
But that's how it works in CDI. If given type isn't a legal bean type, then you cannot use the raw type either.
True, just pointing that out :) |
I don't mean to be the bad guy pushing this, I just saw and inconsistency and wanted to fix it, sorry @gsmet :-D |
|
Just want to point out one thing: we do consider a "strict compliance" switch, which would be off by default (and we'd turn it on to pass the CDI TCK). As we go through the TCK tests, I'm sure we'll stumble upon cases like this and we'll need to add the switch. The problem is that such switch is global. If we relied on our default relaxed behavior in our extensions, the extensions would start to fail once the user turns the switch on (which they may do for various reasons). So I believe we'll need our extensions to work correctly in the strictly compliant mode as well. |
|
The initial policy of not adhering strictly to specs was not only about user experience, it was also about freeing up some time on our end, because those spec-mandated behaviors that we find uninteresting or even detrimental would not have to be implemented/dealt with. If extensions need to be compatible with both compliant and non-compliant mode, we do have to deal with those behaviors, and that means spending time that we won't spend innovating. Worse, having to be compatible with both compliant and non-compliant mode is actually more effort than having to just be compatible with spec-compliant mode. I understand where y'all are coming from, and I know complaining won't change anything. That won't stop me from being unhappy about it, though :) Anyway, I just pushed a commit that sidesteps the problem. |
yrodiere
force-pushed
the
issue30412_2
branch
from
8db040e to
f5cf529
Compare
|
Good point there. I was mainly thinking that the strictly compliant mode would disallow shortcuts such as omitting |
Understood. I appreciate the workaround you implemented :) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Failing Jobs - Building f5cf529
Full information is available in the Build summary check run. Failures⚙️ Devtools Tests - JDK 11 #- Failing: integration-tests/devtools
📦 integration-tests/devtools✖ ✖ ⚙️ Devtools Tests - JDK 17 #- Failing: integration-tests/devtools
📦 integration-tests/devtools✖ ✖ |
|
The test failures are unrelated, see #30501 |
A complementary fix to #30412 and #30415
I originally forgot to address a scenario where a class based bean was able to retain wildcard type as its bean type.
This is against CDI specification and should not occur (the TCK covers that as well).