update to MicroProfile Context Propagation 1.3 #343

Ladicek · 2022-01-06T13:04:16Z

This includes the move from Jakarta EE 8 (javax.*) to 9 (jakarta.*).

Ladicek · 2022-01-06T13:05:23Z

This is very much work in progress, there's a ton of test failures that I don't understand (Weld screams about unsatisfied dependencies) and I don't understand anything about resteasy-context-propagation.

manovotn · 2022-01-06T14:12:47Z

Hmm, some of those error are really weird.

Error: Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.266 s <<< FAILURE! - in io.smallrye.context.test.cdi.providedBeans.ExecutorAndThreadContextInjectionTest
Error: testInjectionWithConfigurationAndSharing(io.smallrye.context.test.cdi.providedBeans.ExecutorAndThreadContextInjectionTest) Time elapsed: 0.265 s <<< ERROR!
org.jboss.weld.exceptions.DeploymentException:
WELD-001408: Unsatisfied dependencies for type ThreadContext with qualifiers @default
at injection point [UnbackedAnnotatedField] @Inject @ThreadContextConfig io.smallrye.context.test.FullStackResource.threadContext
at io.smallrye.context.test.FullStackResource.threadContext(FullStackResource.java:0)

For instance this test doesn't even use the FullStackResource but it somehow gets included. Probably discovery?

Either way, the issue seems to be with injected TC and ME instances. E.g. functionality provided by this CDI extension
@Ladicek make sure you change the META-INF entry for it accordingly, I don't see it in the current PR which would mean the extension is not working at all.
See https://github.com/smallrye/smallrye-context-propagation/blob/main/cdi/src/main/resources/META-INF/services/javax.enterprise.inject.spi.Extension

Ladicek · 2022-01-06T14:45:08Z

Ouch, I did it again! Or forgot to do it, actually. I was struggling for quite a while with MicroProfile Fault Tolerance TCK in SmallRye Fault Tolerance, only to figure out that I've changed everything except the name of the javax.enterprise.inject.spi.Extension file in META-INF/services. Same here, obviously.

Ladicek · 2022-01-06T14:46:43Z

Fixed. Now, I really don't feel like dealing with NoClassDefFound javax/xml/bind/JAXBException 😆

Ladicek · 2022-01-06T14:48:19Z

Ah OK, that's RestAssured still depending on Jakarta EE 8 stuff.

Ladicek · 2022-01-06T15:14:00Z

Replaced the usage of RestAssured with Apache HTTP Client and the FullStackTest now fails with RESTEASY003880: Unable to find contextual data of type: jakarta.ws.rs.core.UriInfo. That seems to be caused by the missing resteasy-context-propagation, though I didn't really investigate.

manovotn · 2022-01-06T15:41:23Z

@Ladicek the CI failure is related to formatting BTW.

That seems to be caused by the missing resteasy-context-propagation, though I didn't really investigate.

Hm, don't know about that. In this codebase I only found https://github.com/smallrye/smallrye-context-propagation/blob/main/tests/src/test/java/io/smallrye/context/storage/RESTEasyContext.java
Which seems to be a fake replacement for actual context hosted in (presumably) resteasy repo. But even this one should likely suffice for tests here?

Ladicek · 2022-01-06T15:56:02Z

OK, so I found that RESTEasy moved a lot of MicroProfile stuff to a separate repo https://github.com/resteasy/resteasy-microprofile, including Context Propagation, and that also entails changed Maven coordinates. What used to be org.jboss.resteasy:resteasy-context-propagation is now org.jboss.resteasy.microprofile:microprofile-context-propagation. Updated. There's a test failure now which I don't really understand :-)

Ladicek · 2022-01-06T15:58:30Z

OK the formatter is bloody idiotic, I changed the commented out piece of code to appease its tasteless taste.

Ladicek · 2022-01-06T16:01:29Z

FTR, there are some strange error messages that are caused by a bug in RESTEasy (https://issues.redhat.com/browse/RESTEASY-3073). I fixed that in resteasy/resteasy#3004

Ladicek · 2022-01-06T16:06:34Z

OK, that test failure was a silly mistake I made when transforming the RestAssured code to use Apache HTTP Client. All should be passing now.

manovotn · 2022-01-06T16:08:47Z

OK the formatter is bloody idiotic, I changed the commented out piece of code to appease its tasteless taste.

LOL

FTR, there are some strange error messages that are caused by a bug in RESTEasy (https://issues.redhat.com/browse/RESTEASY-3073). I fixed that in resteasy/resteasy#3004

That's some dedication. Thanks for looking deep into this :-)

Ladicek · 2022-01-06T16:15:10Z

OK, all tests are passing, so I'm undrafting this. The 1st commit seems fine to me, though the 2nd one certainly deserves some discussion.

manovotn

Both commits look good to me. Thanks @Ladicek!

manovotn · 2022-01-07T09:49:14Z

pom.xml

@@ -93,6 +96,23 @@
                <artifactId>rxjava</artifactId>
                <version>${version.rxjava1}</version>
            </dependency>
+
+            <!-- SmallRye Parent 34 manages Weld to an older version that doesn't have JBoss Class File Writer 1.2.5.Final -->


Shouldn't there be a parent update anyway before all other SR projects move to jakarta namespace?

Parent is actually updated in this commit, and smallrye-jakarta-parent 34 should be fine. Except it has old Weld version, which leads to the infamous issue of jbossas/jboss-classfilewriter#24 :-)

Oh, I see. It has Weld version 4 but for some reason not the latest release :-/
Just FTR, it should be enough to just override the classfilewriter version here but having latest Weld version is likely even better.

manovotn · 2022-01-07T12:17:10Z

@Ladicek I just realized; shouldn't we do a version bump as part of this PR?
I can see MP CP didn't do a major version increase (which is surprising but OK) but maybe we should? Or at least minor version bump?

Cc @FroMage

Ladicek · 2022-01-07T13:05:33Z

Ah good point.

I'm not sure why MP ConProp went with a minor bump, all other MP specs (that I know of) have bumped a major. I'd suggest bumping a major here, too. Let me add a commit.

Ladicek · 2022-01-07T13:06:52Z

Ah and one other thing -- I believe what SmallRye projects do right now is having a dedicated jakarta branch for this effort, and main stays on Jakarta EE 8 for the time being. But better to coordinate with @radcortez on that front.

This includes the move from Jakarta EE 8 (javax.*) to 9 (jakarta.*).

This is to workaround RestAssured's dependency on Jakarta EE 8. See rest-assured/rest-assured#1510.

Ladicek · 2022-01-07T13:08:29Z

Bumped to 2.0.0-SNAPSHOT.

manovotn

PR is LGTM, thanks for bumping the version as well.

Let's wait for @radcortez regarding the branch question before actually merging this.
I'd assume that main moves to jakarta and we create another branch for javax but that's apparently not what's planned :)

xstefank · 2022-01-07T14:44:31Z

Let me help here. So jakarta changes should go into new jakarta branch and there should be a major bump. - https://github.com/smallrye/smallrye-config/tree/jakarta or https://github.com/smallrye/smallrye-health/tree/jakarta

radcortez · 2022-01-07T15:37:15Z

Let me help here. So jakarta changes should go into new jakarta branch and there should be a major bump. - https://github.com/smallrye/smallrye-config/tree/jakarta or https://github.com/smallrye/smallrye-health/tree/jakarta

+1

The reason for this is because Quarkus is still on javax and most of our development is targeting Quarkus, so I believe it would be harder for us to have to backport all the work to javax.

manovotn · 2022-01-10T09:52:49Z

I've created the jakarta branch and changed the base branch of this PR to point there.
We should be good to go now.

Ladicek force-pushed the jakarta branch from 025552f to 400fc68 Compare January 6, 2022 13:09

Ladicek force-pushed the jakarta branch from 400fc68 to c8cc8f7 Compare January 6, 2022 14:46

Ladicek force-pushed the jakarta branch from 24f35a9 to d926012 Compare January 6, 2022 15:53

Ladicek force-pushed the jakarta branch from d926012 to c750a08 Compare January 6, 2022 15:58

Ladicek force-pushed the jakarta branch from c750a08 to aceb5b7 Compare January 6, 2022 16:06

Ladicek force-pushed the jakarta branch from aceb5b7 to 680f10f Compare January 6, 2022 16:08

Ladicek marked this pull request as ready for review January 6, 2022 16:15

Ladicek force-pushed the jakarta branch 2 times, most recently from e5e8e9f to 764477b Compare January 6, 2022 16:19

manovotn previously approved these changes Jan 7, 2022

View reviewed changes

Ladicek added 2 commits January 7, 2022 14:08

update to MicroProfile Context Propagation 1.3

e8a4223

This includes the move from Jakarta EE 8 (javax.*) to 9 (jakarta.*).

replace usage of RestAssured with Apache HTTP Client

380c418

This is to workaround RestAssured's dependency on Jakarta EE 8. See rest-assured/rest-assured#1510.

Ladicek dismissed manovotn’s stale review via 380c418 January 7, 2022 13:08

Ladicek force-pushed the jakarta branch from 764477b to 380c418 Compare January 7, 2022 13:08

manovotn approved these changes Jan 7, 2022

View reviewed changes

manovotn changed the base branch from main to jakarta January 10, 2022 09:51

radcortez approved these changes Jan 10, 2022

View reviewed changes

manovotn merged commit 7344089 into smallrye:jakarta Jan 10, 2022

Ladicek deleted the jakarta branch January 10, 2022 12:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update to MicroProfile Context Propagation 1.3 #343

update to MicroProfile Context Propagation 1.3 #343

Ladicek commented Jan 6, 2022 •

edited

Loading

Ladicek commented Jan 6, 2022

manovotn commented Jan 6, 2022

Ladicek commented Jan 6, 2022

Ladicek commented Jan 6, 2022

Ladicek commented Jan 6, 2022

Ladicek commented Jan 6, 2022

manovotn commented Jan 6, 2022

Ladicek commented Jan 6, 2022

Ladicek commented Jan 6, 2022

Ladicek commented Jan 6, 2022

Ladicek commented Jan 6, 2022

manovotn commented Jan 6, 2022

Ladicek commented Jan 6, 2022

manovotn left a comment

manovotn Jan 7, 2022

Ladicek Jan 7, 2022

manovotn Jan 7, 2022

manovotn commented Jan 7, 2022

Ladicek commented Jan 7, 2022

Ladicek commented Jan 7, 2022

Ladicek commented Jan 7, 2022

manovotn left a comment

xstefank commented Jan 7, 2022

radcortez commented Jan 7, 2022

manovotn commented Jan 10, 2022

update to MicroProfile Context Propagation 1.3 #343

update to MicroProfile Context Propagation 1.3 #343

Conversation

Ladicek commented Jan 6, 2022 • edited Loading

Ladicek commented Jan 6, 2022

manovotn commented Jan 6, 2022

Ladicek commented Jan 6, 2022

Ladicek commented Jan 6, 2022

Ladicek commented Jan 6, 2022

Ladicek commented Jan 6, 2022

manovotn commented Jan 6, 2022

Ladicek commented Jan 6, 2022

Ladicek commented Jan 6, 2022

Ladicek commented Jan 6, 2022

Ladicek commented Jan 6, 2022

manovotn commented Jan 6, 2022

Ladicek commented Jan 6, 2022

manovotn left a comment

Choose a reason for hiding this comment

manovotn Jan 7, 2022

Choose a reason for hiding this comment

Ladicek Jan 7, 2022

Choose a reason for hiding this comment

manovotn Jan 7, 2022

Choose a reason for hiding this comment

manovotn commented Jan 7, 2022

Ladicek commented Jan 7, 2022

Ladicek commented Jan 7, 2022

Ladicek commented Jan 7, 2022

manovotn left a comment

Choose a reason for hiding this comment

xstefank commented Jan 7, 2022

radcortez commented Jan 7, 2022

manovotn commented Jan 10, 2022

Ladicek commented Jan 6, 2022 •

edited

Loading