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

Fix @ApplicationScoped ValueExtractors #20386

Merged

Conversation

yrodiere
Copy link
Member

@yrodiere yrodiere commented Sep 24, 2021

Fixes #20375.

It turns out the problem was how we generate class signatures in client proxies.

"class signatures" are the metadata in Java classes that provides un-erased type information, such as type parameters for implemented interfaces. See https://docs.oracle.com/javase/specs/jvms/se14/html/jvms-4.html#jvms-4.7.9.1 for more information.

Some time ago, we had to add some feature to generate those class signatures in generated client proxies. IIUC, the hack was necessary for Kotlin class generation. See #10769 (comment) for more information.

However, that signature generation was incorrect: it was essentially copy-pasting the signature of the parent (proxied) class. Consequently, the generic info of the proxy class was identical to that of the proxied class. If the parent class implemented ValueExtractor<List<?>>, then the signature of the proxied class looked as if the proxied class implemented that interface directly, too. Which is incorrect.

It gets worse, though: the proxied class appeared as if it implemented that interface... but only when querying generic-aware methods, such as getAnnotatedInterfaces(). When querying non-generic-aware methods, such as getInterfaces(), it would appear that the proxied class did not implement ValueExtractor, at least not directly.

As you can imagine, that was a lot of fun to debug.

This PR changes signature generation for client proxy classes so that:

  1. We only generate a signature if the proxied class/interface has type parameters: this means the proxy class will have type parameters as well (at least it should have), and also that it will extend/implement a parameterized version of the proxied class.
  2. Whenever we generate a proxy for class MyClass<T> implements SomeInterface<T>, the generated proxy has a signature that matches a declaration similar to class MyClass_ClientProxy<T> extends MyClass<T>. That will, at least, be consistent with the non-generic-aware class metadata.

In the case of Hibernate Validator, a value extractor declared as MyExtractor implements ValueExtractor<List<?>> will yield a proxy without any signature at all, since the proxy does not need type parameters and MyExtractor cannot be parameterized. Thus the Hibernate Validator test now passes.

Let's see if it's enough to make all tests added in #10769 pass.

@yrodiere
Copy link
Member Author

Note : this PR is based on #20382, which should be merged first. I will mark this one as draft in the meantime, even though CI passes just fine.

@yrodiere yrodiere marked this pull request as draft September 24, 2021 22:12
@yrodiere yrodiere force-pushed the i20375-valueextractor-applicationscoped branch from 1ed71ab to 18e6feb Compare September 27, 2021 07:54
@yrodiere yrodiere marked this pull request as ready for review September 27, 2021 07:54
@yrodiere
Copy link
Member Author

#20382 has been merged; I rebased this PR on main and marked as ready for review.

@quarkus-bot
Copy link

quarkus-bot bot commented Sep 27, 2021

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building 18e6feb

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status Name Step Failures Logs Raw logs
Attach pull request number ⚠️ Check → Logs Raw logs
CI Sanity Check ⚠️ Check → Logs Raw logs

/**
* Returns true if the given class has type parameters or if its superclass or superinterfaces require a signature
*/
public static boolean needsSignature(ClassInfo klass) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to remove this method? It looks orthogonal to the issue at hand?

But I'll let @mkouba decide about all this anyway.

Copy link
Member Author

@yrodiere yrodiere Sep 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not orthogonal; it was used in ClientProxyGenerator to know whether we need to create a signature for a proxy class. We used to pass the proxied class as a parameter. But that was wrong: a proxy class needing a signature is not the same as the proxied (parent) class needing a signature.

For example:

class MyClass implements Supplier<Integer> {
}

class MyClass_Proxy extends MyClass {
}

The proxied class (MyClass) needs a signature, since it extends a parameterized type, but the proxy class (MyClass_Proxy) doesn't.

Since this method was being misused, and the only use was in ClientProxyGenerator, and it very much looks like internal code, I think it should be removed.

@gsmet gsmet requested a review from mkouba September 27, 2021 09:41
@quarkus-bot
Copy link

quarkus-bot bot commented Sep 27, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 18e6feb

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
JVM Tests - JDK 17 Build ⚠️ Check → Logs Raw logs
MicroProfile TCKs Tests Verify Failures Logs Raw logs

Full information is available in the Build summary check run.

⚠️ Errors occurred while downloading the build reports. This report is incomplete.

Failures

⚙️ MicroProfile TCKs Tests #

- Failing: tcks/resteasy-reactive 

📦 tcks/resteasy-reactive

Failed to execute goal org.codehaus.mojo:exec-maven-plugin:3.0.0:exec (test) on project quarkus-tck-resteasy-reactive: Command execution failed.

📦 tcks/resteasy-reactive/target/testsuite/tests

com.sun.ts.tests.jaxrs.platform.container.completioncallback.JAXRSClient0164.argumentIsNullWhenRegistredClassTest line 209 - More details - Source on GitHub

com.sun.ts.tests.jaxrs.common.JAXRSCommonClient$Fault: Unexpected response content No name has been set yet expecting NULL
	at com.sun.ts.tests.jaxrs.platform.container.completioncallback.JAXRSClient0164.assertString(JAXRSClient0164.java:458)
	at com.sun.ts.tests.jaxrs.platform.container.completioncallback.JAXRSClient0164.argumentIsNullWhenRegistredClassTest(JAXRSClient0164.java:209)

@yrodiere yrodiere force-pushed the i20375-valueextractor-applicationscoped branch from 18e6feb to 30f382e Compare September 28, 2021 07:51
@yrodiere
Copy link
Member Author

The test failure in the MicroProfile TCK seems unrelated; I certainly cannot reproduce it locally. I rebased on main and force-pushed, let's see if it happens again.

@mkouba
Copy link
Contributor

mkouba commented Sep 29, 2021

I'll let @FroMage review this one as he's the author of the original code.

@mkouba mkouba requested a review from FroMage September 29, 2021 13:21
@yrodiere
Copy link
Member Author

Hey @FroMage , any opinion on this?

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a closer look and the changes make sense to me.

Let's wait a few days to see if @FroMage has some feedback. If not, I'll merge.

@gsmet gsmet merged commit 2e5b57c into quarkusio:main Oct 14, 2021
@quarkus-bot quarkus-bot bot added this to the 2.5 - main milestone Oct 14, 2021
@gsmet
Copy link
Member

gsmet commented Oct 14, 2021

Thanks!

@yrodiere yrodiere deleted the i20375-valueextractor-applicationscoped branch May 31, 2022 09:35
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) area/hibernate-validator Hibernate Validator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hibernate Validator: @ApplicationScoped custom ValueExtractor bean lead to ValueExtractorDefinitionException
3 participants