Improve specification of BeanManager.isMatchingBean() and isMatchingEvent()

[Ladicek]

No description provided.

[Azquelt]

I'm not fully sure what this means:

if the eventType contains an unresolved type variable

I guess this is coming from 2.8.1? Though that part of the spec also says that the event object is an instance of a concrete Java class. Should we be requiring eventType to be a Class<?>?

I also note that 2.8.3 is specific about it being the runtime type of the event object that mustn't contain an unresolved type variable.

[Ladicek]

Yeah, so unfortunately what's the runtime type of the event object is not defined anywhere, as far as I can tell. However, it is not a simple Object.getClass().

For example, calling fire() on this bean

@ApplicationScoped
public class MyBean {
    @Inject
    Event<List<String>> event;

    public void fire() {
        event.fire(List.of("foo", "bar"));
    }

    public void observe(@Observes List<String> list, EventMetadata meta) {
        System.out.println(list);
        System.out.println(meta.getType());
    }
}

prints

[foo, bar]
class java.util.ImmutableCollections$List12<class java.lang.String>

with both Weld and ArC.

So a runtime type of an event object may be a parameterized type, so the eventType parameter of isMatchingEvent() needs to be a Type. The caller of isMatchingEvent() would obtain one by new TypeLiteral<List<String>>() {}.getType(), for example.

[Ladicek]

It is a good point though that the eventType name is bad. There's multiple event types, though the content of the set of the event types is fully determined by the runtime type of the event object, which is what we expect the caller to provide. I'm thinking eventObjectType would probably suffice.

[Ladicek]

Changed name of eventType to eventObjectType and the text to refer to "runtime type of event object".

[Azquelt]

I did some more reading and may have confused myself further.

How do we know the type of an event?

It looks like, when you do @Inject Event<Foo> fooEvent and call fooEvent.fire(new FooSubclass()), we expect the event type to be Foo and the event only to be delivered to observers who can observe Foo. It would not call observers of FooSubclass.

That would mean that the resolution of observer methods is not based on the runtime type of the event object, but on the generic type of the injected Event.

• I couldn't find a TCK asserting this either way, though I did find WELD-672.
• ObserverNotificationTest.testObserversNotified seems be annotated with the relevant spec assertion but doesn't test this.

I did note that in the current spec, you can only fire an event through Event<> so you're always specifying the event type when you inject or look that up, but prior to 4.0, you could call BeanManager.fireEvent() where you're not specifying an event type.

To me, the spec seems a little conflicted. On the one hand, 2.8.1 says

An event object is an instance of a concrete Java class with no unresolvable type variables. The event types of the event include all superclasses and interfaces of the runtime class of the event object.

Which strongly suggests that the event type is derived from the runtime type of the event object

Whereas 2.8.2.3 says:

The Event interface provides a method for firing events with a specified combination of type and qualifiers:

...

For an injected Event:

• the specified type is the type parameter specified at the injection point, and
• the specified qualifiers are the qualifiers specified at the injection point.

Which suggests that the event type is derived from the type parameters of the injected Event<>.

I'm not sure whether the we can assume that the first statement applied to BeanManager.fireEvent() and the second applies to Event<>?

Runtime types with unresolvable type variables

I'm still confused by this statement, which is in both 2.8.2.3 and 2.8.3:

If the runtime type of the event object contains an unresolvable type variable, the container must throw an IllegalArgumentException.

At runtime, I don't think it's possible for the type to have an unresolvable type variable, due to type erasure? Maybe this is why these assertions are marked untestable in the TCK audit file?

Maybe this statement was added to indicate that you can't fire an event of type ArrayList<String> via. BeanManager.fireEvent because CDI can't work out the correct event type since ArrayList has type variables, but you could fire an event of MyStringList extends ArrayList<String> because the type parameter in ArrayList has been resolved? If that's the case though, the spec should be clearer that it doesn't apply when using the Event<> interface.

I note that there's a similar separate statement relating to the Event interface in 2.8.2.3:

If the specified type contains a type variable, an IllegalArgumentException is thrown.

This one does make sense since you know the actual type parameter of an injected Event<> bean.

What should we put in this Javadoc

1. I think we should revert your last change and rename the parameter back to eventType. 2.8.3 Observer resolution describes the resolution process in terms of the "event type", and those are the rules we're trying to match here. The spec appears to have two different ways of specifying the event type and it's not totally clear when each is used, but once you get to the point of doing observer resolution, it's assumed you know what the event type is.

2. I think we should document the same restriction that's used for an injected Event because that's now the only way for users to fire events. I.e. we would throw an IllegalArgumentException if eventType includes a type variable.

[manovotn]

couldn't find a TCK asserting this either way, though I did find WELD-672.

That is a veeery old issue and it's not how Weld behaves now.
If you test your assertion with the current code, you'll see that the type of the event is determined from what you pass into the fire() method - in your example that's FooSubclass

Which strongly suggests that the event type is derived from the runtime type of the event object

That is my understanding of how if should work.
It also makes sense from the perspective of EventMetadata#getType() which allows you to introspect the actual type being fired as it can be a subclass of what your observer expects.

Whereas 2.8.2.3 says:

I think this part is useful for establishing what a specified type is because it then says the following:

The methods fire() and fireAsync() fire an event with the specified qualifiers and notify observers, as defined by Observer notification. If the container is unable to resolve the parameterized type of the event object, it uses the specified type to infer the parameterized type of the event types.

Note that is says you fire an event with specified qualifiers but doesn't mention specified type. Furthermore, you then use the specified type to resolve parameterized type of the event object.

ObserverNotificationTest.testObserversNotified seems be annotated with the relevant spec assertion but doesn't test this.

I didn't go through the TCKs yet, I can take a look if I find anything.

[manovotn]

Did a scan of event tests in TCK and it indeed does seem we don't have the payload subtype case covered.

[Azquelt]

Thanks @manovotn that makes more sense.

[Ladicek]

1. I think we should revert your last change and rename the parameter back to eventType. 2.8.3 Observer resolution describes the resolution process in terms of the "event type", and those are the rules we're trying to match here. The spec appears to have two different ways of specifying the event type and it's not totally clear when each is used, but once you get to the point of doing observer resolution, it's assumed you know what the event type is.

I think the specification is unfortunately a little inconsistent. 2.8.1. Event types and qualifier types defines a set of event types, while 2.8.3 Observer resolution defines the resolution algorithm in terms of an event type. The set of event types is determined by the runtime type of the event object, so the two terms are almost equivalent, in a sense. I believe that the term "event type" in the observer resolution algorithm should be interpreted either as "runtime type of the event object" (that seems more likely), or as "any event type that exists in the set of event types".

One extra option would be for isMatchingEvent to not require a type of the event [object], but the event object directly (Object).

2. I think we should document the same restriction that's used for an injected Event because that's now the only way for users to fire events. I.e. we would throw an IllegalArgumentException if eventType includes a type variable.

I agree that we can drop the word "unresolvable", because any type variable the caller could possibly include in the event object type would be unresolvable (that is, we don't have an Event<> injection point that would allow us to potentially resolve it).

It is also how BeanContainer.resolveObserverMethods() is specified (see 2.9.1.9. Observer method resolution).

[Azquelt]

The spec does not say that an event has the @Default qualifier if it has no qualifiers other than @Any.

It instead specifies that different behaviour is applied to observers with @Default qualifier.

Such observer will only be notified for events having either no qualifiers or only @Default qualifier:

Although I think both ways of specifying this have the same result, we should not have the Javadoc here contradicting the spec.

[Ladicek]

Yeah, you're right. I think I found a way to rewrite the text, but it required me to change the terms "event type" and "event qualifiers" to "specified type" and "specified qualifiers" (which are also CDI specification terms, I'm not inventing anything new here).

[Ladicek]

Updated the text to address both @Azquelt's comments. In the javadoc of isMatchingEvent, I changed the terms "event type" and "event qualifiers" to "specified type" and "specified qualifiers" (which are also CDI specification terms), which allowed me to be more precise in how @Default and @Any work when the event type/qualifiers are obtained from the specified type/qualifiers.

[Azquelt]

I think that looks better to me.

Part of my comments definitely came from not having understood how the event type and specified type were used differently (which @manovotn helpfully explained). Whether we use event type, specified type, or event object type probably doesn't matter, but since in practise you can't have a parameterized object type, defining this method in terms of the specified type probably makes it easier to understand.

[Ladicek]

I added one commit where I tried to make BeanContainer.isMatchingEvent() javadoc more precise. I intentionally made it an extra commit to allow comparing, but it should be squashed before merging.

[manovotn]

@Ladicek looks like the license check kicked in

Warning: Files with unapproved licenses:
/home/runner/work/cdi/cdi/api/src/main/java/jakarta/enterprise/inject/build/compatible/spi/InvokerFactory.java

[Ladicek]

Oh isn't that funny. That is of course a correct observation. I think I'll submit a separate PR to fix it.

[Ladicek]

Rebased.

[Ladicek]

I reworded the 2nd commit slightly and I think this PR is now ready (after the 2 commits are squashed, of course).

[Ladicek]

One last tiny change after discussion with @manovotn and squashed. This should be ready to merge as is.

[manovotn]

Thank you and sorry for my nitpicking :)