Test BeanContainer assignability API methods #526

Azquelt · 2024-01-12T18:37:11Z

Tests for .isMatchingBean and .isMatchingEvent.

Fixes #518

Azquelt · 2024-01-12T18:38:46Z

.../java/org/jboss/cdi/tck/tests/beanContainer/assignability/BeanObserverAssignabilityTest.java

+        assertFalse(getCurrentBeanContainer().isMatchingBean(reducedBeanTypes, Set.of(), Object.class, Set.of()),
+                "Bean matched Object despite it not being in bean types");


Wasn't sure about this test.

Every bean has Object as a bean type. Should Object be implied if not passed in the beanTypes set?

Every bean has Object as a bean type. Should Object be implied if not passed in the beanTypes set?

Yes, that's always added.
I think there was even a TCK for @Typed that asserted that Object is added automatically.

(Lurking. Curious if this is generally true in the specification (to be clear, I know about https://jakarta.ee/specifications/cdi/4.0/jakarta-cdi-spec-4.0#bean_types)? I see that certainly for @Typed Object.class is implied, but if I make a synthetic bean and explicitly do not include Object.class in its Set<Type>...? For example, https://jakarta.ee/specifications/cdi/4.0/apidocs/jakarta.cdi/jakarta/enterprise/inject/spi/configurator/beanattributesconfigurator#types(java.util.Set) says nothing about Object.class being intrinsically added.)

For example, https://jakarta.ee/specifications/cdi/4.0/apidocs/jakarta.cdi/jakarta/enterprise/inject/spi/configurator/beanattributesconfigurator#types(java.util.Set) says nothing about Object.class being intrinsically added.)

Yes, I am aware of that. However, event a synth bean is a bean and as therefore the first link you posted should still apply. At least in my opinion :)
And I think that's also what Weld does; not sure if there is a TCK for it though 🤷

It might be helpful to clarify whether Object is automatically added with a note on the relevant methods.

For this method, it sounds like we think Object should be automatically added to the set of bean types so this test should change to assertTrue?

Here's a proposal: jakartaee/cdi#744 I went full on the precise mode, as that's just easiest frankly :-)

their own implementation of Bean which may not return Object from getTypes.

That would be an invalid implementation of Bean in my eyes, though the container can compensate for it. All the more reason for using the BeanConfigurator API.

Here's a proposal

That looks good, thank you :-)

I've updated the test to match.

That would be an invalid implementation of Bean in my eyes

Yeah, I agree and it's not the only way you could have an invalid Bean implementation either (e.g. returning a non-scope annotation from getScope()). I guess we could be more specific about how and when these sorts of errors are handled.

I note that BeanConfigurator is still vulnerable to some of the same issues. You could fail to pass Object to types as @ljnelson suggested, or pass a non-scope annotation to scope and we don't currently document what should happen.

Good points. Out of scope of this PR for sure, though :-)

If all synthetic beans are beans, and if all beans have the bean type java.lang.Object, then all synthetic beans have the bean type java.lang.Object. That is a straightforward implication in my eyes.

I am of the same opinion.

The only possible downside is that someone might start asking "why don't we automatically scan for all superclasses and implemented interfaces?", and I wouldn't have a good answer :-)

Because you might want to simulate a scenario where @Typed was used and some types might be omitted.

Yeah, I agree and it's not the only way you could have an invalid Bean implementation either (e.g. returning a non-scope annotation from getScope()). I guess we could be more specific about how and when these sorts of errors are handled.

Well, since you can define arbitrary things if you implement Bean yourself, it would be rather extensive to nail it all down...

Azquelt · 2024-01-12T18:42:30Z

Also worth noting:

The API that these tests depend on hasn't been released yet. I needed to change the cdi.api.version to 4.1.0-SNAPSHOT and build the CDI API locally.
These new API methods aren't implemented yet so I haven't been able to run the tests.

manovotn · 2024-01-12T19:28:03Z

These new API methods aren't implemented yet so I haven't been able to run the tests.

Should be fairly easy to do in Weld, I'll make a branch next week so that we can test it.

.../java/org/jboss/cdi/tck/tests/beanContainer/assignability/BeanObserverAssignabilityTest.java

Ladicek · 2024-01-15T15:16:34Z

Other assertions LGTM.

manovotn

Two more notes besides the one code comment below:

I promised you a Weld branch/pr, which is here -> WELD-2774 Implement BM methods for event/bean assignability weld/core#2906. It passes all tests in the PR's current state apart from the one which I believe is wrong.
I am also pondering whether we should assert something about validity of input such as annotations not being CDI qualifiers or bean types being invalid. On one hand it would be nice, OTOH every invocation of this method will have to perform a solid amount of verification just because we assume users may not know what they are doing 🤷

...ain/java/org/jboss/cdi/tck/tests/beanContainer/assignability/BeanEventAssignabilityTest.java

Ladicek · 2024-01-23T15:15:13Z

I am also pondering whether we should assert something about validity of input such as annotations not being CDI qualifiers or bean types being invalid. On one hand it would be nice, OTOH every invocation of this method will have to perform a solid amount of verification just because we assume users may not know what they are doing

We talked about this internally, my personal opinion is a bit different in that it is typical for BeanManager methods to validate inputs, so I don't see why these methods shouldn't. Here's my proposal: jakartaee/cdi#748

manovotn · 2024-01-23T17:26:55Z

I am also pondering whether we should assert something about validity of input such as annotations not being CDI qualifiers or bean types being invalid. On one hand it would be nice, OTOH every invocation of this method will have to perform a solid amount of verification just because we assume users may not know what they are doing

We talked about this internally, my personal opinion is a bit different in that it is typical for BeanManager methods to validate inputs, so I don't see why these methods shouldn't. Here's my proposal: jakartaee/cdi#748

Yea, added my approval there but @Azquelt might want to take a look as well.

We'll also want some TCK coverage for that - the qualifier validation should be an easy one.
The unresolved type variable might be tricky as the only other case in TCK has this marked as testable=false (see here).
Finally legal bean types - that might be harder because we cannot check how was the set of types trimmed; instead we'd have to check that the type was removed and cannot be used to satisfy given required type.

@Azquelt do you wish to look into these or should me or Ladislav take that?

Dismissing as my complaint was invalid

Azquelt · 2024-01-24T15:19:57Z

@manovotn I'm happy to look at it, I'll add it to this PR unless you'd rather merge this part first.

I understood "legal bean types" to refer to just 2.1.2.1, which I think is only concerned with the type itself and not whether it's been discovered, trimmed or vetoed?

I'm not entirely sure about what an unresolved type variable is (I have also commented on jakartaee/cdi#748), so I'll struggle to test that unless someone can provide an example of something that wouldn't be a valid event object under that rule.

Ladicek · 2024-01-24T15:26:57Z

I'm not entirely sure about what an unresolved type variable is (I have also commented on jakartaee/cdi#748), so I'll struggle to test that unless someone can provide an example of something that wouldn't be a valid event object under that rule.

The audit file for existing tests says that this provision (or rather, the existing provision on which this one is modeled) is untestable. Maybe there's a way to test it, but I'd be fine with leaving that one uncovered.

manovotn · 2024-01-24T16:03:31Z

I'm not entirely sure about what an unresolved type variable is (I have also commented on jakartaee/cdi#748), so I'll struggle to test that unless someone can provide an example of something that wouldn't be a valid event object under that rule.

The audit file for existing tests says that this provision (or rather, the existing provision on which this one is modeled) is untestable. Maybe there's a way to test it, but I'd be fine with leaving that one uncovered.

Yes that's exactly why I linked the tck file. I would also be fine leaving this untested.

manovotn · 2024-01-24T16:09:05Z

@manovotn I'm happy to look at it, I'll add it to this PR unless you'd rather merge this part first.

Feel free to add it here. I would rather merge once we release cdi api anyway.

I understood "legal bean types" to refer to just 2.1.2.1, which I think is only concerned with the type itself and not whether it's been discovered, trimmed or vetoed?

Right, it's just a case where a user can pass in a set of types where some would normally be filtered as illegal for given bean

Azquelt · 2024-01-25T18:13:23Z

I've added tests for the changes in jakartaee/cdi#748.

The only one that didn't pass against @manovotn's branch was this one for having a type variable in the event type. I'd tried to test this in a similar way to FireEventTest.testTypeVariableEventTypeFails.

manovotn · 2024-01-25T18:18:04Z

I've added tests for the changes in jakartaee/cdi#748.

The only one that didn't pass against @manovotn's branch was this one for having a type variable in the event type. I'd tried to test this in a similar way to FireEventTest.testTypeVariableEventTypeFails.

Thanks, I'll check on it.

...ain/java/org/jboss/cdi/tck/tests/beanContainer/assignability/BeanEventAssignabilityTest.java

manovotn

LGTM
The Weld branch I linked earlier is also passing fine.

Ladicek · 2024-01-29T12:50:12Z

My implementation in ArC also passes.

We should probably merge jakartaee/cdi#748 first and then update the audit files here, but I wouldn't really mind otherwise. Except we'll need a new CDI API release (Alpha2?) first, anyway.

Azquelt · 2024-02-05T18:00:51Z

I've rebased and updated the audit files based on jakartaee/cdi#748 now that it's merged.

manovotn · 2024-02-15T16:44:27Z

@Azquelt master branch of CDI TCK repo now has CDI API 4.1.0.Beta1 - please rebase this PR and we should be able to get it merged

Tests for .isMatchingBean and .isMatchingEvent.

Azquelt · 2024-02-15T17:09:23Z

Rebased, squashed and copyright headers updated.

Azquelt commented Jan 12, 2024

View reviewed changes

Azquelt mentioned this pull request Jan 12, 2024

Tweaks to Bean and Event Assignability API jakartaee/cdi#742

Merged

Azquelt force-pushed the assignability-api branch from 9d6ee82 to 8521084 Compare January 12, 2024 18:45

Ladicek reviewed Jan 15, 2024

View reviewed changes

.../java/org/jboss/cdi/tck/tests/beanContainer/assignability/BeanObserverAssignabilityTest.java Outdated Show resolved Hide resolved

Ladicek mentioned this pull request Jan 15, 2024

Improve precision of the bean and event assignability API specification jakartaee/cdi#744

Merged

manovotn mentioned this pull request Jan 23, 2024

WELD-2774 Implement BM methods for event/bean assignability weld/core#2906

Closed

manovotn previously requested changes Jan 23, 2024

View reviewed changes

...ain/java/org/jboss/cdi/tck/tests/beanContainer/assignability/BeanEventAssignabilityTest.java Show resolved Hide resolved

manovotn reviewed Jan 25, 2024

View reviewed changes

...ain/java/org/jboss/cdi/tck/tests/beanContainer/assignability/BeanEventAssignabilityTest.java Outdated Show resolved Hide resolved

manovotn approved these changes Jan 26, 2024

View reviewed changes

Ladicek approved these changes Jan 29, 2024

View reviewed changes

Azquelt force-pushed the assignability-api branch from 79f0969 to 8e948cc Compare February 5, 2024 17:44

Test BeanContainer assignability API methods

466f0f2

Tests for .isMatchingBean and .isMatchingEvent.

Azquelt force-pushed the assignability-api branch from 8e948cc to 466f0f2 Compare February 15, 2024 17:08

starksm64 merged commit 8d19cf7 into jakartaee:master Feb 15, 2024
4 checks passed

Ladicek added this to the CDI 4.1 milestone Feb 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test BeanContainer assignability API methods #526

Test BeanContainer assignability API methods #526

Azquelt commented Jan 12, 2024

Azquelt Jan 12, 2024

manovotn Jan 12, 2024

ljnelson Jan 12, 2024 •

edited

Loading

manovotn Jan 12, 2024

Azquelt Jan 15, 2024

Ladicek Jan 15, 2024

Ladicek Jan 15, 2024

Azquelt Jan 15, 2024

Ladicek Jan 15, 2024

manovotn Jan 15, 2024

Azquelt commented Jan 12, 2024

manovotn commented Jan 12, 2024

Ladicek commented Jan 15, 2024

manovotn left a comment

Ladicek commented Jan 23, 2024

manovotn commented Jan 23, 2024

Azquelt commented Jan 24, 2024

Ladicek commented Jan 24, 2024

manovotn commented Jan 24, 2024

manovotn commented Jan 24, 2024

Azquelt commented Jan 25, 2024

manovotn commented Jan 25, 2024

manovotn left a comment

Ladicek commented Jan 29, 2024

Azquelt commented Feb 5, 2024

manovotn commented Feb 15, 2024

Azquelt commented Feb 15, 2024

		assertFalse(getCurrentBeanContainer().isMatchingBean(reducedBeanTypes, Set.of(), Object.class, Set.of()),
		"Bean matched Object despite it not being in bean types");

Test BeanContainer assignability API methods #526

Test BeanContainer assignability API methods #526

Conversation

Azquelt commented Jan 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljnelson Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Azquelt commented Jan 12, 2024

manovotn commented Jan 12, 2024

Ladicek commented Jan 15, 2024

manovotn left a comment

Choose a reason for hiding this comment

Ladicek commented Jan 23, 2024

manovotn commented Jan 23, 2024

Azquelt commented Jan 24, 2024

Ladicek commented Jan 24, 2024

manovotn commented Jan 24, 2024

manovotn commented Jan 24, 2024

Azquelt commented Jan 25, 2024

manovotn commented Jan 25, 2024

manovotn left a comment

Choose a reason for hiding this comment

Ladicek commented Jan 29, 2024

Azquelt commented Feb 5, 2024

manovotn commented Feb 15, 2024

Azquelt commented Feb 15, 2024

ljnelson Jan 12, 2024 •

edited

Loading