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

Remove override via application.xml test #428

Merged
merged 2 commits into from
Feb 14, 2024

Conversation

KyleAure
Copy link
Contributor

@KyleAure KyleAure commented Feb 8, 2024

When writing tests for qualifier I wrote a test that is invalid.

Based on the Concurrency javadoc:

* If a {@code managed-executor} and {@code ManagedExecutorDefinition}
* have the same name, their attributes are merged to define a single
* {@code ManagedExecutorService} definition, with each attribute that is specified
* in the {@code managed-executor} deployment descriptor entry taking
* precedence over the corresponding attribute of the annotation.
* If any qualifier elements are specified, the set of qualifier elements
* replaces the qualifiers attribute of the annotation.

I assumed that meant that a deployment descriptor in application.xml would merge/override values defined on the class annotation @ManagedThreadFactoryDefinition.

This was the wrong assumption based on the platform spec:
https://github.com/jakartaee/platform/blob/29bf7cc3266097d3b68d8d849ccd01eefbae9a1b/specification/src/main/asciidoc/platform/ApplicationAssemblyDeployment.adoc?plain=1#L1069-L1081

Which seems to indicate that metadata-complete is what defines whether or not merging should happen between deployment descriptor and annotation defined resources.

It wouldn't make sense for application.xml to have a metadata-complete attribute since there are no packages/classes directly in the application EAR (there are only modules and libraries).
The schema backs this up by not having a metadata-complete attribute for application.xml and therefore we can infer that any resource defined in application.xml is complete and will never be merged, making this test invalid.

@KyleAure KyleAure added bug Something isn't working TCK labels Feb 8, 2024
@KyleAure KyleAure self-assigned this Feb 8, 2024
Copy link
Contributor

@njr-11 njr-11 left a comment

Choose a reason for hiding this comment

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

The changes in this pull look good, but can we rename InvalidQualifier3 to something else? It is very confusing that this qualifier's name includes invalid when it is actually valid, just sometimes overridden or not depending on whether the web.xml is used.

@KyleAure
Copy link
Contributor Author

No one seems against this fix so I'll go ahead and merge.

@KyleAure KyleAure merged commit b84edec into jakartaee:main Feb 14, 2024
2 of 3 checks passed
@KyleAure KyleAure deleted the fix-qualifier-tests branch February 14, 2024 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working TCK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants