feat(#1381): Add a way to specify "inject-only" with @JacksonInject

[giulong]

useInput=TRUE in JacksonInject now make deserialization discard the injected value in favor of input (if any)

[giulong]

moved since this should not fail anymore

[giulong]

According to useInput's javadocs:

Default is OptBoolean.DEFAULT, which translates to OptBoolean.TRUE: this is
for backwards compatibility (2.8 and earlier always allow binding input value).

This combination: input YES, injectable YES, useInput DEFAULT should actually behave as if we had useInput = TRUE, that is dropping the injected value and returning the input.

Nevertheless, since useInput never worked that way, this would mean introducing breaking changes. My suggestion is to keep things as per this test and change the javadoc accordingly:

Default is OptBoolean.DEFAULT, which translates to OptBoolean.FALSE

[giulong]

This class shows how the behavior changes for all the combinations of the other test class when all injected fields are optional. Do you think we still need a required property in the annotation?

I feel like combining useInput and optional would cover all cases, also because optional should drive the same logic (but reversed) of required. Given this, adding a required property would make things unclear for the users, and the internal logic would be quite messy. Does this make sense?

[cowtowncoder]

LGTM.

[cowtowncoder]

This is part I don't fully understand: if injectable is only added if no input is to be used, how do things work with useInput = false, injectable value being present.

[giulong]

As per this test, with no input but injected value present, the injected value is always returned regardless of useInput:

@Test
@DisplayName("input NO, injectable YES, useInput DEFAULT|TRUE|FALSE => injected")
void test2() throws Exception {
    assertEquals("injected", injectedMapper.readValue(empty, InputDefault.class).getField());
    assertEquals("injected", injectedMapper.readValue(empty, InputTrue.class).getField());
    assertEquals("injected", injectedMapper.readValue(empty, InputFalse.class).getField());
}

I read it like "we have no input but we have a value to inject, so we don't care about anything else: let's use the injected value". Is this the expected behavior? Or you meant something else?

[cowtowncoder]

Logic-wise, yes, you are correct (expected behavior I agree with).

But I meant code; meaning... what does addInjectable() do -- to me, it's the thing that injects non-input (injectable) value. So why is that contingent on "useInput"? And I think check here seems wrong in that sense: injectable value may be needed even input is used.
It is not only injected if useInput is false (or missing); can also be injected if useInput is true but input has no value for property.

[giulong]

It's a way to eagerly discard the injectable if we're going to use the input anyways. I think it's better in terms of performance since any further evaluation is useless, and there are many methods involved down that path.
But I also get this:

injectable value may be needed even input is used

Not now maybe, but in general I get that if a field is marked as injected, we should add it to the internal injectables for whatever future logic.

I can try to change this, which means passing useInput to the builder (last param in the snippet), as we made for optional:

builder.addInjectable(PropertyName.construct(m.getName()),
                            m.getType(),
                            beanDesc.getClassAnnotations(), m, entry.getKey(), optional, useInput);

Maybe we could also think of passing the whole injectableValue to the builder, instead of passing its properties one by one.

But I think this is not going to be easy. What do you think, is it worth it?

[cowtowncoder]

Yes, figured it was meant as optimization.
But the problem with " if we're going to use the input anyways" is that there might not be any input.
So it's not "use input if any matching, otherwise null" but rather "use input if present; if not, use injected value". Otherwise why even mark it with @JacksonInject at all, actually.

So we cannot know statically which one to use.

And yes, I think this is necessary to handle properly.

But I understand things are messy, tricky, complicated, esp. when passing things via Constructors (CreatorProperty properties) vs Field/MethodProperty ones.

[cowtowncoder]

Actually, no: if I change things to put @JacksonInject into Constructor parameter (instead of Field) -- common alternative (although adding on Field is allowed too), a few tests fail.

So this doesn't quite work fully yet.

[cowtowncoder]

Ok. I don't think this fix is complete: I changed test set up slightly (in a way I think users are more likely to use it -- although original way is legit too... but both should work) and now a few tests fail.

So would need to figure a way to make things fully work with combination of input and/or injectable values.

[giulong]

Fair point, I missed that constructor properties take another path. On it

[giulong]

This is the moment when creator properties are resolved, so I think it's ok to have the logic here, as per the method's javadoc as well:

/**
* Method called to do necessary post-processing such as injection of values
* and verification of values for required properties,
* after either {@link #assignParameter(SettableBeanProperty, Object)}
* returns true (to indicate all creator properties are found), or when
* then whole JSON Object has been processed,
*/
public Object[] getParameters(SettableBeanProperty[] props)

I placed the new part right before the code that checks for null creator properties in case we have an injected value to use, so to not fail. Should I move it after?

[giulong]

We now have the tests to cover all the scenarios (I guess), meaning combinations of injected creator properties vs fields, with or without JacksonInject.optional() and DeserializationFeature.FAIL_ON_UNKNOWN_INJECT_VALUE. So, in case other changes are needed, at least we have a no regression suite to check them.

[cowtowncoder]

We now have the tests to cover all the scenarios (I guess), meaning combinations of injected creator properties vs fields, with or without JacksonInject.optional() and DeserializationFeature.FAIL_ON_UNKNOWN_INJECT_VALUE. So, in case other changes are needed, at least we have a no regression suite to check them.

I will try to have a good look tomorrow, to answer with proper understanding.

Thank you for going through this again.

[giulong]

I reworked the logic to always add the properties annotated with @JacksonInject to the injectables, regardless of their optional and useInput values.

Few things to note, for completeness:

1. I'm doing comparisons like value == JacksonInject.Value.empty(). I think it's better to not use equals here, since we're looking for that specific constant.

2. In this case:

   @Test
   @DisplayName("FAIL_ON_UNKNOWN_INJECT_VALUE NO, input NO, injectable NO, useInput DEFAULT|TRUE|FALSE => exception")
   void test1() {
       assertThrows(ValueInstantiationException.class,
               () -> plainMapper.readValue(empty, InputDefault.class));
       assertThrows(ValueInstantiationException.class,
               () -> plainMapper.readValue(empty, InputDefaultConstructor.class));
       assertThrows(ValueInstantiationException.class,
               () -> plainMapper.readValue(empty, InputTrue.class));
       assertThrows(ValueInstantiationException.class,
               () -> plainMapper.readValue(empty, InputTrueConstructor.class));
       assertThrows(ValueInstantiationException.class,
               () -> plainMapper.readValue(empty, InputFalse.class));
       assertThrows(ValueInstantiationException.class,
               () -> plainMapper.readValue(empty, InputFalseConstructor.class));
   }

   we expect an exception, since we have no input nor injectable value. Since FAIL_ON_UNKNOWN_INJECT_VALUE is disabled as well, the injection process does not throw the usual MissingInjectableValueExcepion. The ValueInstantiationException comes from the AnnotatedConstructor.call method, since the arg provided is actually JacksonInject.Value.empty().
   In the injection process, we use that value as a way to signal that we tried to resolve the injectable, but due to useInput, optional and FAIL_ON_UNKNOWN_INJECT_VALUE the injection should actually be skipped. I think this is fine since we need a special marker (null isn't good since it could be a valid value), but from the user's perspective I also think throwing a generic ValueInstantiationException is not the best: seeing a message like Cannot construct instance of ..., problem: argument type mismatch could be misleading.

3. This implementation worsens If JacksonInject is specified for field and deserialized by the Creator, the inject process will be executed twice. #4218, making the injection process happen thrice. Checked with JacksonInject4218Test.

I hope the overall implementation is good enough to be merged, despite the bullets above. What do you think?