Allow configuration of rest client default scope #2940

machi1990 · 2019-06-23T11:58:42Z

Fixes #2798

/cc @geoand

geoand · 2019-06-23T12:05:47Z

Thanks @machi1990!

I will take a look most likely tomorrow. Perhaps @michalszynkiewicz would also like to review it

geoand · 2019-06-23T12:09:41Z

One comment, I think we should keep this to a single commit.

machi1990 · 2019-06-23T12:30:14Z

Thanks @machi1990!

I will take a look most likely tomorrow. Perhaps @michalszynkiewicz would also like to review it

Okay thanks @geoand . I still have a failing TCK, I thought opening a PR would help with figuring out why.

geoand · 2019-06-23T12:39:56Z

FWIW, here is an example of how I was debugging tests when I ran things locally:

 mvn clean  verify -f tcks/pom.xml -Dtest=InvokeWithJsonBProviderTest --projects 'io.quarkus:quarkus-tck-microprofile-rest-client'

geoand · 2019-06-24T12:59:53Z

@machi1990 do you need any help with this one?

machi1990 · 2019-06-24T13:05:27Z

@machi1990 do you need any help with this one?

Hey @geoand, thanks. Sure. I am yet to figure out why the test are failing for Linux Build and passing for the other platforms.

Also, I got one TCK for default scope passing and not the other. But the integration test I added for the cases are passing. Any ideas?

geoand · 2019-06-24T13:07:02Z

I'll have to take a closer look later on today or tomorrow morning. If you find any fixes in the meantime, feel free to push :)

geoand · 2019-06-25T04:03:18Z

@machi1990 seems like you are making progress, 👍 . However I am guessing that there is still some corner case not addressed since CDIInvokeSimpleGetOperationTest and CDIInvokeAsyncSimpleGetOperationTest are failing with what looks like a CDI error that the PR indends to fix :)

machi1990 · 2019-06-25T05:31:03Z

CDIInvokeSimpleGetOperationTest

hey @geoand , thanks for checking my progress. Yes, I was able to take a look yesternight: had a day away from comp (was in conference).

So here is what I am observing locally, only running the failing test (which I believe to be just CDIInvokeSimpleGetOperationTest) passes:

mvn verify -f tcks/pom.xml --projects 'io.quarkus:quarkus-tck-microprofile-rest-client' -Dtest=HasSingletonScopeTest

If I only run the two tests which were excluded before this PR, they work just fine as well:

mvn verify -f tcks/pom.xml --projects 'io.quarkus:quarkus-tck-microprofile-rest-client' -Dtest=CDIInvokeAsyncSimpleGetOperationTest,CDIInvokeSimpleGetOperationTest

However, If I run the same tests by adding HasSingletonScopeTest test which seems to set the scope to Singleton through /mp-rest/scope leads to failure on the first test.

mvn verify -f tcks/pom.xml --projects 'io.quarkus:quarkus-tck-microprofile-rest-client' -Dtest=CDIInvokeSimpleGetOperationTest,HasSingletonScopeTest

Thoughts?

geoand · 2019-06-25T05:37:01Z

That's interesting for sure! I don't know enough about the tests to know what is going on, but what I would do is debug the tests (as described here) and try to figure out exactly what is happening.

machi1990 · 2019-06-26T11:21:17Z

...sions/arc/deployment/src/main/java/io/quarkus/arc/deployment/NoArgsConstructorProcessor.java

@@ -127,6 +112,27 @@ public boolean skipValidation(InjectionTargetInfo target, ValidationRule rule) {
        }
    }

+    private Collection<ClassInfo> collectTargetClass() {


Modified because to be able to create a default noArgConstructor for AppScope and RequestScope once. Otherwise, TCK tests in HasRequestScopeTest and HasAppScopeTest were failing with a <cinit> error.

We would need validation from @mkouba on this one

It wouldn't hurt to describe what's actually changed and why? ;-)

Why
While working on this PR, I noticed the following error:

Caused by: java.lang.RuntimeException: Failed to start quarkus at io.quarkus.runner.ApplicationImpl1.<clinit>(ApplicationImpl1.zig:205) ... 112 more Caused by: java.lang.ClassFormatError: Duplicate method name "<init>" with signature "()V" in class file org/eclipse/microprofile/rest/client/tck/cditests/HasAppScopeTest$MyAppScopedApi at java.lang.ClassLoader.defineClass1(Native Method) at java.lang.ClassLoader.defineClass(ClassLoader.java:763) at java.lang.ClassLoader.defineClass(ClassLoader.java:642) at io.quarkus.runner.RuntimeClassLoader.findClass(RuntimeClassLoader.java:217) at io.quarkus.runner.RuntimeClassLoader.loadClass(RuntimeClassLoader.java:163) at java.lang.ClassLoader.loadClass(ClassLoader.java:357) at org.eclipse.microprofile.rest.client.tck.cditests.HasAppScopeTest_MyAppScopedApi_4a90cb1eedbe1d7e3f457833a0ee883318179f2e_Synthetic_Bean.<init>(HasAppScopeTest_MyAppScopedApi_4a90cb1eedbe1d7e3f457833a0ee883318179f2e_Synthetic_Bean.zig:120) at io.quarkus.arc.setup.Default_ComponentsProvider.getComponents(Default_ComponentsProvider.zig:222) at io.quarkus.arc.ArcContainerImpl.<init>(ArcContainerImpl.java:84) at io.quarkus.arc.Arc.initialize(Arc.java:18) at io.quarkus.arc.runtime.ArcDeploymentTemplate.getContainer(ArcDeploymentTemplate.java:33) at io.quarkus.deployment.steps.ArcAnnotationProcessor$build7.deploy_0(ArcAnnotationProcessor$build7.zig:53) at io.quarkus.deployment.steps.ArcAnnotationProcessor$build7.deploy(ArcAnnotationProcessor$build7.zig:141) at io.quarkus.runner.ApplicationImpl1.<clinit>(ApplicationImpl1.zig:191) ... 112 more

When running rest-client tck tests notably when running and HasAppScopeTest or HasRequestScopeTest. My understanding is, we tried to generate a no arg constructor twice and looking at the modified code, an HashSet is used while ClassInfo does not declare an hashCode method leading to MyAppScopedApi.class being recorded twice.

What changed
Record a normal scoped bean class once based on its DotName which has a hashCode. The DotName is provided by ClassInfo.name().

Is that the way to go about it? Open to changing this to something different. @mkouba @geoand

Seems reasonable to me, @mkouba WDYT?

@machi1990 otherwise the PR looks good! So pls revert the changes in NoArgsConstructorProcessor and just ignore the interfaces there.

@mkouba @geoand thanks for your analysis.

Modifier.isInterface(annotationInstance.target().asClass().flags())

I'll revert this part and use this instead.

@mkouba did all the meaningful analysis 😆

@mkouba @geoand thanks for your analysis.

Modifier.isInterface(annotationInstance.target().asClass().flags())

I'll revert this part and use this instead.

This had been addressed. @mkouba @geoand

I'll let @mkouba review since he was the one that found the problem

tcks/microprofile-rest-client/tck-suite.xml

machi1990 · 2019-07-01T07:38:53Z

tcks/microprofile-rest-client/tck-suite.xml

-            <class name="org.eclipse.microprofile.rest.client.tck.cditests.HasRequestScopeTest">
+
+
+            <!-- excluded because of race scope configuration on SimpleGetApi interface -->


@mkouba I added a comment about here. If you run the tests one by one, they all pass. If you run them together, they fail randomly: one pass and not the other, etc.

That's really weird and we should probably try to find the real cause first. I'll try to run the tests locally.

So I think that the problem is that ConfigProvider.getConfig() caches the ConfigProviderResolver in a static field. We do work around this limitation in QuarkusConfigProducer by using ConfigProviderResolver.instance().getConfig() instead. I'll try to update the code and run the tests.

Hm, it didn't help most probably because the rest client beans are registered in STATIC_INIT and ConfigProviderResolver is re-set during RUNTIME_INIT...

Hey @mkouba, thanks for the investigation. How do you advise we proceed regarding this?

I've created #3089 - pls add this link to the relevant comment in the tck-suite.xml.

Then I think we can merge this PR.

…est/scope. Fixes quarkusio#2797. Makes Dependent default scope. Fixes quarkusio#2798

machi1990 assigned geoand Jun 23, 2019

machi1990 force-pushed the 2798 branch from 8c7b63b to 325cd29 Compare June 23, 2019 12:31

geoand added the coding-in-progress label Jun 23, 2019

machi1990 force-pushed the 2798 branch 2 times, most recently from 5c9e9fe to b7f99fb Compare June 23, 2019 16:08

emmanuelbernard requested a review from geoand June 24, 2019 12:57

machi1990 force-pushed the 2798 branch from b7f99fb to bf6f762 Compare June 24, 2019 20:04

machi1990 force-pushed the 2798 branch from bf6f762 to 708fe27 Compare June 26, 2019 10:43

machi1990 commented Jun 26, 2019

View reviewed changes

machi1990 force-pushed the 2798 branch 2 times, most recently from 2bb30d3 to 8631726 Compare June 28, 2019 14:00

geoand removed the coding-in-progress label Jun 29, 2019

machi1990 mentioned this pull request Jun 29, 2019

MicroProfile Rest Client property mp/scope has not been implemented #2797

Closed

mkouba reviewed Jul 1, 2019

View reviewed changes

tcks/microprofile-rest-client/tck-suite.xml Show resolved Hide resolved

machi1990 commented Jul 1, 2019

View reviewed changes

mkouba mentioned this pull request Jul 4, 2019

Test - stale config values #3089

Closed

feat(rest-client): allow configuration of default scope through /mp-r…

faf4a98

…est/scope. Fixes quarkusio#2797. Makes Dependent default scope. Fixes quarkusio#2798

machi1990 force-pushed the 2798 branch from 8631726 to faf4a98 Compare July 4, 2019 07:38

mkouba approved these changes Jul 4, 2019

View reviewed changes

mkouba merged commit 8d841dc into quarkusio:master Jul 4, 2019

machi1990 deleted the 2798 branch July 4, 2019 12:45

gsmet added this to the 0.19.0 milestone Jul 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow configuration of rest client default scope #2940

Allow configuration of rest client default scope #2940

machi1990 commented Jun 23, 2019

geoand commented Jun 23, 2019 •

edited

Loading

geoand commented Jun 23, 2019 •

edited

Loading

machi1990 commented Jun 23, 2019

geoand commented Jun 23, 2019

geoand commented Jun 24, 2019

machi1990 commented Jun 24, 2019

geoand commented Jun 24, 2019

geoand commented Jun 25, 2019

machi1990 commented Jun 25, 2019

geoand commented Jun 25, 2019 •

edited

Loading

machi1990 Jun 26, 2019

geoand Jun 26, 2019

mkouba Jun 27, 2019

machi1990 Jun 27, 2019

geoand Jun 28, 2019

mkouba Jun 28, 2019

machi1990 Jun 28, 2019

geoand Jun 28, 2019

machi1990 Jun 29, 2019

geoand Jun 29, 2019

machi1990 Jul 1, 2019

mkouba Jul 1, 2019

machi1990 Jul 1, 2019

mkouba Jul 1, 2019

mkouba Jul 1, 2019

machi1990 Jul 3, 2019

mkouba Jul 4, 2019

machi1990 Jul 4, 2019

		<class name="org.eclipse.microprofile.rest.client.tck.cditests.HasRequestScopeTest">


		<!-- excluded because of race scope configuration on SimpleGetApi interface -->

Allow configuration of rest client default scope #2940

Allow configuration of rest client default scope #2940

Conversation

machi1990 commented Jun 23, 2019

geoand commented Jun 23, 2019 • edited Loading

geoand commented Jun 23, 2019 • edited Loading

machi1990 commented Jun 23, 2019

geoand commented Jun 23, 2019

geoand commented Jun 24, 2019

machi1990 commented Jun 24, 2019

geoand commented Jun 24, 2019

geoand commented Jun 25, 2019

machi1990 commented Jun 25, 2019

geoand commented Jun 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoand commented Jun 23, 2019 •

edited

Loading

geoand commented Jun 23, 2019 •

edited

Loading

geoand commented Jun 25, 2019 •

edited

Loading