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

dont inherit producers #2903

Closed
wants to merge 2 commits into from
Closed

Conversation

dev-qnz
Copy link
Contributor

@dev-qnz dev-qnz commented Jan 16, 2024

I suspect that not inheriting a producer fails in combination with an @Observes ProcessAnnotatedType Extension.

A producer works on its own and also with subclasses around like this

class Parent {
    @Produces Foo produceFoo() { ... }
}

class Child extends Parent {
}

so that only one Foo is produced and Foo resolves.

However, in combination with an @Observes ProcessAnnotatedType Extension like below, Foo does not any longer resolve successfully even though the @Observes ProcessAnnotatedType Extension did not do any real harm on its own beside its mere existence.

public static class ProcessAnnotatedTypeExtension implements Extension {
    <T> void processAnnotatedType(@Observes ProcessAnnotatedType<T> processAnnotatedType) {
        processAnnotatedType.configureAnnotatedType();
    }
}

@dev-qnz
Copy link
Contributor Author

dev-qnz commented Jan 17, 2024

@manovotn
Copy link
Contributor

manovotn commented Jan 17, 2024

I've tried to write a naive test based on your description and I am not seeing the same issue.
Here's what I did - https://github.com/weld/core/compare/master...manovotn:core:reproduce2903?expand=1

What am I missing?
FTR you can run that via:

  • mvn clean install -DskipTests
  • mvn clean verify -Dtest=MethodProducerExtensionTest -f tests-arquillian/pom.xml

@manovotn
Copy link
Contributor

FTR the CI failure seems related to your changes, but before we jump into that, I'd like to understand what's the basic scenario where this fails.

@dev-qnz
Copy link
Contributor Author

dev-qnz commented Jan 17, 2024

I've tried to write a naive test based on your description and I am not seeing the same issue. Here's what I did - https://github.com/weld/core/compare/master...manovotn:core:reproduce2903?expand=1

Oops you are right, that works for me, too. However, with a producer field instead of a producer method, it fails, at least for me, and does not without configureAnnotatedType. See https://github.com/weld/core/compare/master...dev-qnz:weld-core:reproduce2903?expand=1. I'd appreciate if you could check if you can confirm that failure and if so whether I did something stupid or not.

@manovotn
Copy link
Contributor

I've tried to write a naive test based on your description and I am not seeing the same issue. Here's what I did - https://github.com/weld/core/compare/master...manovotn:core:reproduce2903?expand=1

Oops you are right, that works for me, too. However, with a producer field instead of a producer method, it fails, at least for me, and does not without configureAnnotatedType. See https://github.com/weld/core/compare/master...dev-qnz:weld-core:reproduce2903?expand=1. I'd appreciate if you could check if you can confirm that failure and if so whether I did something stupid or not.

Sure, will take a look tomorrow!

@dev-qnz
Copy link
Contributor Author

dev-qnz commented Jan 18, 2024

https://jakarta.ee/specifications/cdi/4.0/jakarta-cdi-spec-4.0#alternative_metadata_sources states that the container should call AnnotatedField or AnnotatedMethod...

.getJavaMember().getDeclaringClass() to determine the type in the type hierarchy that declared the ...

... field or method respectively.

Eventually, I figure, member.getDeclaringType().getJavaClass() should work just as well. The current impl uses one or the other way in different places and could possibly be more consistent in that respect. Apparently I couldn't resist the temptation to follow the opinion on that of the specs and now there are possibly more changes than actually necessary with respect to the original issue.

@manovotn
Copy link
Contributor

Tried the producer field scenario and indeed it looks like there is inheritance happening where it shouldn't.
Specifically, this looks like a violation of https://jakarta.ee/specifications/cdi/4.0/jakarta-cdi-spec-4.0#member_level_inheritance:

If X declares a non-static producer field x then Y does not inherit this field. (This behavior is different to what is defined in the Common Annotations for the Java Platform specification.)

FTR, I have created a WELD issue to track this - https://issues.redhat.com/browse/WELD-2773

What's puzzling is how is this being caused by the extension invoking pat.configureAnnotatedType() doing nothing.

@manovotn
Copy link
Contributor

manovotn commented Jan 18, 2024

Hm, right, so the fact we manipulate the annotated type in an extension creates a new AT which ends up having an internal property discovered set to false which leads to a different code branch - https://github.com/weld/core/blob/master/impl/src/main/java/org/jboss/weld/annotated/enhanced/jlr/EnhancedAnnotatedTypeImpl.java#L171-L210

Surprisingly, discovered ATs use

if (field.getJavaMember().getDeclaringClass().equals(javaClass))

to determine if the field belongs to them whereas non-discovered instead perform

if (annotatedField.getJavaMember().getDeclaringClass().equals(javaClass))

That's technically all I need to locally fix for the test to pass.
I've sent that under #2904 to see the CI result.

.getJavaMember().getDeclaringClass() to determine the type in the type hierarchy that declared the field or method respectively.

@dev-qnz This rings a bell, I vaguely recall encountering this before and for some reason choosing not to internally use that everywhere. Some of the failing tests you can see in the CI might be a hint; I need to take a deeper look into this.
FTR you can run the failing test locally with mvn clean verify -Dtest=AssignabilityOfRawAndParameterizedTypesTest -f jboss-tck-runner/pom.xml

Some of your changes likely trigger a cycle as I am getting this:

[ERROR] org.jboss.cdi.tck.tests.lookup.typesafe.resolution.parameterized.AssignabilityOfRawAndParameterizedTypesTest.arquillianBeforeClass  Time elapsed: 7.305 s  <<< FAILURE!
org.jboss.weld.exceptions.WeldException: Java heap space
Caused by: java.lang.OutOfMemoryError: Java heap space

@manovotn
Copy link
Contributor

Also, what got me stuck for a bit is that your tests are:
a) not running by default when you just perform mvn clean install - probably because of their structure
b) they are not reproducing the issue because the extension is not added as a service provider and hence isn't working

I did some quick and dirty changes to see if they would pass with my single-file change in the other PR and they do. Well, the DisposerInheritanceTest passes even with no changes - it cover an area that was working already.

So basically all other file changes in this PR are not related to the producer inheritance problem itself.
I'd probably be inclined to keep the changes to minimum and solve this with #2904 if you agree @dev-qnz ?

@dev-qnz
Copy link
Contributor Author

dev-qnz commented Jan 22, 2024

So basically all other file changes in this PR are not related to the producer inheritance problem itself. I'd probably be inclined to keep the changes to minimum and solve this with #2904 if you agree @dev-qnz ?

that's fine with me. thanks for your collaboration.

@dev-qnz dev-qnz closed this Jan 22, 2024
@dev-qnz dev-qnz deleted the inherited-producer branch January 22, 2024 10:06
@manovotn
Copy link
Contributor

Alright, thank you for bringing this to my attention 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants