Skip to content

Conversation

giulong
Copy link
Contributor

@giulong giulong commented May 1, 2025

As requested in #3072, deserialization will not fail if the injectable values provided to the object reader does not contain a property and:

  • that property is explicitly annotated with @JacksonInject(optional = OptBoolean.TRUE) or

  • the DeserializationFeature.FAIL_ON_UNKNOWN_INJECT_VALUE is switched off, i.e.:

    new ObjectMapper()
        .reader()
        .without(DeserializationFeature.FAIL_ON_UNKNOWN_INJECT_VALUE)

⚠️ Depends on jackson-annotations#291

@giulong giulong changed the title Part of #3072: Make @JacksonInject not fail when there's no corresponding value Part of databind#3072: Make @JacksonInject not fail when there's no corresponding value May 1, 2025
@cowtowncoder cowtowncoder added cla-needed PR looks good (although may also require code review), but CLA needed from submitter 2.20 Issues planned at 2.20 or later labels May 1, 2025
@cowtowncoder
Copy link
Member

First of all: thank you for contributing this!

Added a note on CLA in the other PR -- only needs to be done once to cover both.

@giulong
Copy link
Contributor Author

giulong commented May 2, 2025

gonna provide the CLA asap

@giulong
Copy link
Contributor Author

giulong commented May 2, 2025

CLA just sent

@cowtowncoder cowtowncoder added cla-received PR already covered by CLA (optional label) and removed cla-needed PR looks good (although may also require code review), but CLA needed from submitter labels May 2, 2025
if (_injectableValues == null) {
return reportBadDefinition(ClassUtil.classOf(valueId), String.format(
"No 'injectableValues' configured, cannot inject value with id [%s]", valueId));
final JacksonInject.Value injectableValue = getAnnotationIntrospector()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we would not be doing annotation introspection during actual deserialization; pretty much everything is and should be introspected during deserializer construction.
Need to figure out how to avoid that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. I opened this PR on my fork to let you evaluate the idea before pushing here a quite different approach.
Basically, I added the _optional field in the ValueInjector so that the BeanDeserializerFactory constructs them with it.

Tests are passing. If it's ok, i'll do the usual overload-and-deprecate before merging it on this one and continue the review.

Additionally: should I try to move the FAIL_ON_UNKNOWN_INJECT_VALUE feature in some other early phase, or it's fine to evaluate it in DeserializationContext and InjectableValues? We cannot do that in the same place (BeanDeserializerFactory) since it's called too early:

ObjectReader reader = newReader() // the BeanDeserializerFactory kicks in here, before de-activating the feature
                .without(FAIL_ON_UNKNOWN_INJECT_VALUE);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have to look deeper to have an informed opinion, but by default I assume you know what you are doing (seems that way), i.e. your intuition is probably right. And yes, DeserializationFeatures can be change on per-call basis (so decision based on them need to be more dynamic), whereas annotations are static (even mix-ins) and decision likewise more dynamic. In this case we then we have "interesting" combination.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@giulong Ok yes, checking DeserializationFeature.FAIL_ON_UNKNOWN_INJECT_VALUE is cheap and can/should be done as needed, not earlier. Only annotation access need to be front-loaded.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And yes, I think the approach in referenced PR makes sense -- let's go with that!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do

Object ob = _values.get(key);
if (ob == null && !_values.containsKey(key)) {
throw new IllegalArgumentException("No injectable id with value '"+key+"' found (for property '"+forProperty.getName()+"')");
final JacksonInject.Value injectableValue = ctxt.getAnnotationIntrospector()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here too; really should handle introspection so we would not need to do that processing every time injectable value not found. Not sure if that is doable.

@cowtowncoder
Copy link
Member

CLA just sent

Received, we are good to go wrt that.

@giulong giulong requested a review from cowtowncoder May 9, 2025 16:48
{
_member.setValue(beanInstance, findValue(context, beanInstance));
final Object value = findValue(context, beanInstance);
if (!JacksonInject.Value.empty().equals(value)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite sure why this test is needed... ?

@@ -0,0 +1,124 @@
package com.fasterxml.jackson.databind.tofix;
Copy link
Member

@cowtowncoder cowtowncoder May 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong package: this is for failing tests :)

(I will move to under .../deser/inject)

Copy link
Member

@cowtowncoder cowtowncoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Filed #5151 for some follow-up work.

@cowtowncoder cowtowncoder merged commit f15b71d into FasterXML:2.x May 10, 2025
8 checks passed
@cowtowncoder
Copy link
Member

@giulong Merged and #3072 fixed -- thank you very much for contributing this!

Also realized there is older unresolved issue #1381, for which @JacksonInject.useInput was added (in Jackson 2.9) but which is not supported.
Would be great to implement that but I am not sure I have time to work on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.20 Issues planned at 2.20 or later cla-received PR already covered by CLA (optional label)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants