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.

Sign up for GitHub

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

Jump to bottom

Remove TODOs from 3.0 #4411

Closed
tomas-langer opened this issue Jun 20, 2022 · 9 comments
Closed

Remove TODOs from 3.0 #4411

tomas-langer opened this issue Jun 20, 2022 · 9 comments
Assignees
@klustria
Milestone
3.0.0

Comments

@tomas-langer
Copy link
Member

Check all instances of TODO 3.0.0-JAKARTA and fix the issues (or remove the TODO if the issue is fixed/works as designed).
@tomas-langer tomas-langer added this to the 3.0.0 milestone Jun 20, 2022
@klustria
Copy link
Member

Partial resolution was made via PR: #4447 which covers the following:

  1. Remove comments in JdbcMetricsHistogram, JdbcMetricsTimer and NoOpMetricImpl as it was reviewed that no extra work needs to be done on them
  2. HelidonInboundHeaderProvider was restored to use implements InboundHeadersProvider because org.glassfish.jersey.ext.microprofile:jersey-mp-rest-client dependency was restored back via PR 3884
  3. org.jboss.classfilewriter:jboss-classfilewriter dependency is added in integrations/graal/mp-native-image-extension after it was removed from weld-se-core so that internal class ClassFileUtils can be restored in WeldSubstitutions

@klustria
Copy link
Member

klustria commented Jun 29, 2022 •
edited
Loading

Other fixes are being worked on by Laird and Tomas:

  1. PR Updates persistence.xmls to 3.0. Re-enables JPA tests. Removes eclips… #4446 in progress to fix weaving by @ljnelson
  2. @tomas-langer is working on scripts related failures via re-enable native image tests in pipeline on master #4341 and is forecasted to be resolved with new version of parsson dependency

@klustria
Copy link
Member

Remaining issues to work on:

  1. Some todos related Micronaut: https://github.com/oracle/helidon/blob/master/integrations/micronaut/cdi/src/main/java/io/helidon/integrations/micronaut/cdi/MicronautCdiExtension.java#L313-L329, https://github.com/oracle/helidon/blob/master/integrations/micronaut/cdi/src/main/java/io/helidon/integrations/micronaut/cdi/MicronautCdiExtension.java#L351-L356 and https://github.com/oracle/helidon/blob/master/integrations/micronaut/data/src/test/java/io/helidon/integrations/micronaut/cdi/data/MicronautDataCdiExtensionTest.java#L111-L120
  2. Find suitable replacement for javax.annotation:javax.annotation-api (because this is an old version of the Java EE API). Tried jakarta.platform:jakarta.jakartaee-api:8.0.0 and works but was advised this is also not a good alternative as it is for the whole profile. The Jakarta specific library which was recommended jakarta.annotation:jakarta.annotation-api:jar:1.3.5 was unfortunately is causing a DependencyConvergence failure because it is also pulling v2.0.0 of this library from other dependencies like helidon-config, helidon-common-service-loader, and helidon-config-hocon:
[WARNING] 
Dependency convergence error for jakarta.annotation:jakarta.annotation-api:2.0.0 paths to dependency are:
+-io.helidon.config:helidon-config-etcd:3.0.0-SNAPSHOT
  +-io.helidon.config:helidon-config:3.0.0-SNAPSHOT
    +-jakarta.annotation:jakarta.annotation-api:2.0.0
and
+-io.helidon.config:helidon-config-etcd:3.0.0-SNAPSHOT
  +-io.helidon.common:helidon-common-media-type:3.0.0-SNAPSHOT
    +-io.helidon.common:helidon-common-service-loader:3.0.0-SNAPSHOT
      +-jakarta.annotation:jakarta.annotation-api:2.0.0
and
+-io.helidon.config:helidon-config-etcd:3.0.0-SNAPSHOT
  +-jakarta.annotation:jakarta.annotation-api:1.3.5
and
+-io.helidon.config:helidon-config-etcd:3.0.0-SNAPSHOT
  +-io.helidon.config:helidon-config-hocon:3.0.0-SNAPSHOT
    +-jakarta.annotation:jakarta.annotation-api:2.0.0

[WARNING] Rule 0: org.apache.maven.plugins.enforcer.DependencyConvergence failed with message:
Failed while enforcing releasability. See above detailed error message.
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  1.336 s
[INFO] Finished at: 2022-06-28T09:42:38-07:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-enforcer-plugin:3.0.0-M1:enforce (enforce-dependencies) on project helidon-config-etcd: Some Enforcer rules have failed. Look above for specific messages explaining why the rule failed. -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException

@klustria
Copy link
Member

For the DependencyConvergence error, I found a way to include jakarta.annotation:jakarta.annotation-api:jar:1.3.5 even if jakarta.annotation:jakarta.annotation-api:jar:2.0.0 is transitively being pulled by other dependencies. This can be done by adding a . at the end of groupId like this:

        <dependency>
            <groupId>jakarta.annotation.</groupId>
            <artifactId>jakarta.annotation-api</artifactId>
            <version>1.3.5</version>
            <scope>provided</scope>
            <optional>true</optional>
        </dependency>

I found this suggestion from: https://stackoverflow.com/questions/24962607/multiple-versions-of-the-same-dependency-in-maven. Somehow a dot in the groupId is translated as a / as mentioned in this doc: https://maven.apache.org/pom.html#maven-coordinates.

@klustria
Copy link
Member

After discussing with @tomas-langer, we are not doing above workaround to add dot . at the end of the groupId, but instead stick with the existing javax.annotation:javax.annotation-api dependency library.

@klustria
Copy link
Member

klustria commented Jul 6, 2022

Remaining TODO in the micronaut area:

and

Per discussion with Tomas, the goal is to upgrade to the latest Micronaut libraries with the hope that they have switch to jakarta validation, but looks like this is not the case. The latest micronaut-validation-3.5.2.pom has this:

<dependency>
  <groupId>javax.validation</groupId>
  <artifactId>validation-api</artifactId>
  <version>2.0.1.Final</version>
  <scope>compile</scope>
</dependency>

which means it does not support jakarta validation.

@klustria
Copy link
Member

klustria commented Jul 7, 2022 •
edited
Loading

Micronaut 3.5.2 guide https://docs.micronaut.io/3.5.2/guide/index.html#beanValidation and SNAPSHOT guide https://docs.micronaut.io/snapshot/guide/index.html#beanValidation both mentions:

Since Micronaut 1.2, Micronaut has built-in support for validating beans annotated with javax.validation annotations

Created PR: #4476, the goal of which is to use 2.0.2 of jakarta.validation-api instead of 3.0.0 to revert back to the use of javax.validation rather than jakarta.validation which is what even the latest version of micronaut-validation supports.

@klustria klustria mentioned this issue Jul 7, 2022
Merged
@klustria
Copy link
Member

klustria commented Jul 8, 2022 •
edited
Loading

For the remaining TODO:

For this issue, I finally figured out that both bean definitions that was deemed duplicate need to be added after comparing it with v2.5.1 behavior. Here’s what I debugged for what we deemed are duplicate bean definitions:

In Helidon 2.5.1 (We add both):

io.helidon.integrations.micronaut.cdi.MicronautBean@62d0ac62 - io.helidon.integrations.micronaut.cdi.$TestMicronautBeanDefinition
io.helidon.integrations.micronaut.cdi.MicronautBean@3003697 - io.helidon.integrations.micronaut.cdi.$$TestMicronautBeanDefinition$InterceptedDefinition

In Helidon 3.0 (We sort and add only the first one)

io.helidon.integrations.micronaut.cdi.TestMicronautBean - io.helidon.integrations.micronaut.cdi.$TestMicronautBean$Definition$Intercepted$Definition
io.helidon.integrations.micronaut.cdi.TestMicronautBean - io.helidon.integrations.micronaut.cdi.$TestMicronautBean$Definition

The first field in both lists is the MicronautBean.toString() and the next field after the dash is MicronautBean.definitionRef().getBeanDefinitionName(). In Helidon 3, it looks like a duplicate and that is because toString() was overridden to return the bean type while in Helidon 2.5, it returns the object name. I have changed that to add hashCode so it will be unique. Also we don’t need to choose of which to add between the 2 bean definitions that we thought are duplicate, instead we add both just like in v2.5. So the new code will look like the same as the code prior to the Jakarta 3.0 change. PR related to the mentioned change : #4494

@klustria klustria mentioned this issue Jul 8, 2022
Merged
@klustria
Copy link
Member

klustria commented Jul 8, 2022

List of PRs that complete this task:

  1. Updates persistence.xmls to 3.0. Re-enables JPA tests. Removes eclips… #4446
  2. Fix code with 'TODO 3.0.0-JAKARTA' comment #4447
  3. Re-enable Packaging Tests #4412 (as part of re-enable native image tests in pipeline on master #4341)
  4. Removed final from proxied types, these could no longer be proxied. #4489 (as part of re-enable native image tests in pipeline on master #4341)
  5. Revert back to using javax.validation as jakarta.validation is not supported by micronaut #4476
  6. Load both bean definitions of the same type instead of choosing just one #4494

Searching TODO 3.0.0-JAKARTA in the project does not yield any more result.

@klustria klustria closed this as completed Jul 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@klustria klustria

Labels
None yet
Projects
Backlog
Archived in project
Milestone
3.0.0
Development

No branches or pull requests
2 participants
@tomas-langer @klustria