LangChain4JAIServicePortableExtension#processInjectionPoint should treat programmatic lookup differently

[manovotn]

Following code is IMO wrong in how it treats programmatic lookup. One valid type of injection point in CDI is Instance<X>. In this case, the aforementioned code should detect that and based on desired behavior either skip it or perform its logic on the inner X type.

Note that with an Instance<X>, you cannot be sure the user will actually attempt to resolve the X at runtime; instead, they might perform Instance#select() and resolve a subtype instead or not use it at all.

[TheEliteGentleman]

Similar to how I've done it here? https://github.com/sinditech/sindi-langchain4j/blob/main/sindi-langchain4j-jakartaee-portable-extension/src/main/java/za/co/sindi/langchain4j/spi/cdi/extension/LangChain4JAiServiceExtension.java

[TheEliteGentleman]

I've made the change on my branch and created the PR. https://github.com/smallrye/smallrye-llm/pull/28

[manovotn]

This issue is based on my conversation with @ehsavoie where the code I linked would sometimes identify the required type as Instance which is wrong. I thought just changing the single method would fix that. That being said, on closer inspection I can see I was looking into his fork which is slightly behind and the case I am describing was throwing an error where it now only logs. But the same problem should still exist there.

[Emily-Jiang]

Sounds like this issue does not exist in the repo. Shall we close this here @manovotn ?

[manovotn]

@Emily-Jiang Sorry I am not very familiar with this code base (or rather, not at all :)) but while my initial assumption was wrong, the code still looks a but weird. For instance this part:

if (Instance.class.equals(Reflections.getRawType(event.getInjectionPoint().getType()))) {
   processInjectionPoint(event);
}

The event.getInjectionPoint().getEvent() should never return Instance. According to the javadoc of InjectionPoint:

If the injection point is a dynamically selected reference obtained then the metadata obtain reflects the injection point of the {@link Instance}, with the required type and any additional required qualifiers defined by {@linkplain Instance Instance.select()}.

[Emily-Jiang]

@Emily-Jiang Sorry I am not very familiar with this code base (or rather, not at all :)) but while my initial assumption was wrong, the code still looks a but weird. For instance this part:

if (Instance.class.equals(Reflections.getRawType(event.getInjectionPoint().getType()))) {
   processInjectionPoint(event);
}

The event.getInjectionPoint().getEvent() should never return Instance. According to the javadoc of InjectionPoint:

If the injection point is a dynamically selected reference obtained then the metadata obtain reflects the injection point of the {@link Instance}, with the required type and any additional required qualifiers defined by {@linkplain Instance Instance.select()}.

Fair point @manovotn ! The processInjectionPoint(event) would never be executed.

[TheEliteGentleman]

This PR should resolve this.

@Emily-Jiang Sorry I am not very familiar with this code base (or rather, not at all :)) but while my initial assumption was wrong, the code still looks a but weird. For instance this part:

if (Instance.class.equals(Reflections.getRawType(event.getInjectionPoint().getType()))) {
   processInjectionPoint(event);
}

The event.getInjectionPoint().getEvent() should never return Instance. According to the javadoc of InjectionPoint:

If the injection point is a dynamically selected reference obtained then the metadata obtain reflects the injection point of the {@link Instance}, with the required type and any additional required qualifiers defined by {@linkplain Instance Instance.select()}.

Fair point @manovotn ! The processInjectionPoint(event) would never be executed.

[TheEliteGentleman]

@manovotn unfortunately the Instance if-statement is true, but we need to process it based on its parameterized type. I've updated the CDI Extension and simplified the whole process entirely.

[manovotn]

@manovotn unfortunately the Instance if-statement is true, but we need to process it based on its parameterized type. I've updated the CDI Extension and simplified the whole process entirely.

Ok, I stand corrected...and confused :) I wrote a quick test for this in Weld (code) and you are right - it indeed returns Instance<X> as a type so your code against current spec/Weld version is correct.

Now, I am not sure if this is intended behavior or a bug - it is in fact more practical this way as it allows you to observe dynamic resolution injection points explicitly but the specification doesn't seem to have much ground for it apart from saying that:

getInjectionPoint() returns the InjectionPoint object that will be used by the container to perform injection.

[manovotn]

FTR, I have created a specification clarification issue which you can find under https://github.com/jakartaee/cdi/issues/826

[TheEliteGentleman]

Closing this as we've discovered that this is a non-issue and code has been merged to the main branch.