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

Merge jakarta.web.beans and web.beans #7958

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

matthiasblaesing
Copy link
Contributor

@matthiasblaesing matthiasblaesing commented Nov 12, 2024

The jakarta.web.beans module was created as a copy of the web.beans
module, just using the new namespaces after the death of JavaEE and the
inception of JakartaEE. Other modules followed a different route, where
the existing implementations were extended to support both the old and
new XML/package namespaces.

This becomes a problem, when it hits the UI as the user then does not
know what to do (which template/menu entry to choose and "what the heck
is JakartaEE and why is that not JavaEE anymore?!"). What is more
codepaths are duplicated and fixes for one area are highly likely to be
relevant for the other also.

This change integrates the unittests from the jakarta.web.beans project,
if they actually check jakarta* functions and modifies the codepaths, so
that both namespaces are supported.

This PR also fixes and reenables the CI/CD integration of the web.beans tests.

Closes: #7918
Closes: #6875

Start: 85 passed, 45 failed

- Add "Java SE Platforms and Libraries" as test dependency

Status: 125 passed, 5 failed

- Reference code uses wrong placement of annotations, method annotations
  come before the return type

- There is not mention in the spec, that an @Injectable field may not be
  initialized

Status: 130 passed
The jakarta.web.beans module was created as a copy of the web.beans
module, just using the new namespaces after the death of JavaEE and the
inception of JakartaEE. Other modules followed a different route, where
the existing implementations were extended to support both the old and
new XML/package namespaces.

This becomes a problem, when it hits the UI as the user then does not
know what to do (which template/menu entry to choose and "what the heck
is JakartaEE and why is that not JavaEE anymore?!"). What is more
codepaths are duplicated and fixes for one area are highly likely to be
relevant for the other also.

This change integrates the unittests from the jakarta.web.beans project,
if they actually check jakarta* functions and modifies the codepaths, so
that both namespaces are supported.

Closes: apache#7918
Local run is clean, runtime is high, but seems reasonable (5:30min)
@matthiasblaesing matthiasblaesing added Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) Java EE/Jakarta EE [ci] enable enterprise job enterprise [ci] enable enterprise job labels Nov 12, 2024
@matthiasblaesing matthiasblaesing added this to the NB25 milestone Nov 12, 2024
@matthiasblaesing matthiasblaesing added the ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) label Nov 13, 2024
@apache apache locked and limited conversation to collaborators Nov 13, 2024
@apache apache unlocked this conversation Nov 13, 2024
Copy link
Member

@mbien mbien left a comment

Choose a reason for hiding this comment

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

i think this makes sense since the copied modules would likely never diverge enough to warrant having two impls. Only looked through this on github so far.

Copy link
Member

Choose a reason for hiding this comment

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

this class was retrofitted to use varargs at some point, but not all methods have them. I believe the changeset can be greately reduced by adding this:

    public static boolean hasAnnotation(Element element, CompilationInfo info, String... annotations) {
        return getAnnotationMirror(element, info, annotations) != null;
    }

this would get rid of many if-blocks.

consider also removing those two:

    public static AnnotationMirror getAnnotationMirror(Element element,
            String annotation,CompilationInfo info )
    {
        return getAnnotationMirror(element, info, annotation);
    }
    public static boolean hasAnnotation(Element element, String annotation,
            CompilationInfo info )
    {
        return getAnnotationMirror(element, info)!=null;
    }

since it already exists as varargs variant with swapped params. This isn't public API I believe - should be fine.

@matthiasblaesing
Copy link
Contributor Author

@mbien thanks for the suggestion. Pushed an update, might squashed with b4c6e55 before merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) enterprise [ci] enable enterprise job Java EE/Jakarta EE [ci] enable enterprise job Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form)
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Navigate context menu contains duplicated entries 2 "Contexts and Dependency Injection" choices for New
2 participants