Skip to content

BeanDeserializer does not reset jsonParser currentValue after deserialising nested Object/Collection Node #1834

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Eoin-Mur opened this issue Nov 22, 2017 · 2 comments
Labels
need-test-case To work on issue, a reproduction (ideally unit test) needed

Comments

@Eoin-Mur
Copy link

Eoin-Mur commented Nov 22, 2017

BeanDeserializer assigns the current value before walking the objects fields.
If the objects fields value is a nested object / collection the deserialiser invoked for these values follows the same pattern invoking setCurrentValue. Similairy any custom serialiser for a field may invoke setCurrentValue.
As the currentValue is not reset after deserialisng the nested object or collection. The parsers current value remains the value of traversed nested object or collection.
This is a issue for any custom serializer which might invoke the getCurrentValue as the value is now out of sync with the currentContext and is pointing to some object outside its tree.

This pattern is used throughout BeanDeserializer and CollectionDeserializer and from the comments introduced via ticket: #631

This is present in the current release 2.9.2

/**
     * Streamlined version that is only used when no "special"
     * features are enabled.
     */
    private final Object vanillaDeserialize(JsonParser p,
    		DeserializationContext ctxt, JsonToken t)
        throws IOException
    {
        final Object bean = _valueInstantiator.createUsingDefault(ctxt);
        // [databind#631]: Assign current value, to be accessible by custom serializers
        p.setCurrentValue(bean);
        if (p.hasTokenId(JsonTokenId.ID_FIELD_NAME)) {
            String propName = p.getCurrentName();
            do {
                p.nextToken();
                SettableBeanProperty prop = _beanProperties.find(propName);

                if (prop != null) { // normal case
                    try {
                        prop.deserializeAndSet(p, ctxt, bean);
                    } catch (Exception e) {
                        wrapAndThrow(e, bean, propName, ctxt);
                    }
                    continue;
                }
                handleUnknownVanilla(p, ctxt, bean, propName);
            } while ((propName = p.nextFieldName()) != null);
        }
        return bean;
    }

Example scenario vanillaDeserialize sets the current value as the object walking(ObjA), the field to serialise is an empty ArrayList the deserialiser invoked for example is CollectionDeserializer which follows this patten and sets the current value in the parser as the ArrayList.
CollectionDeserializer returns setting the value of the field in ObjA to the deserialised ArrayList but the parsers current value remains as the ArrayList.

Fix would be in all deserialisers following this pattern to save a reference to the previousValue and before exiting reset the currentValue

@Override
    public Collection<Object> deserialize(JsonParser p, DeserializationContext ctxt,
            Collection<Object> result)
        throws IOException
    {
        // Ok: must point to START_ARRAY (or equivalent)
        if (!p.isExpectedStartArrayToken()) {
            return handleNonArray(p, ctxt, result);
        }

        Object prevVal = p.getCurrentValue(); // Keep reference to parents value
        p.setCurrentValue(result);

        JsonDeserializer<Object> valueDes = _valueDeserializer;
        final TypeDeserializer typeDeser = _valueTypeDeserializer;
        CollectionReferringAccumulator referringAccumulator =
            (valueDes.getObjectIdReader() == null) ? null :
                new CollectionReferringAccumulator(_containerType.getContentType().getRawClass(), result);

        JsonToken t;
        while ((t = p.nextToken()) != JsonToken.END_ARRAY) {
            // Process elements
        }
        p.setCurrentValue(prevValue); // reset value to the parent before returning to it
        return result;
    }
@cowtowncoder
Copy link
Member

cowtowncoder commented Nov 22, 2017

One thing to note is that current value settings are hierarchic, and parent type's current value is retained within parser/output context by streaming API. Databinding level is only concerned about assigning current value for current level of the stack, and then writeStartObject() / writeEndObject() (and same with Array) take care of rest of management. Unwrapping will be problematic (and possible simply incompatible with tracking) and there may be some other edge cases with structural changes.
So it should not be necessary to explicitly change the value; however, logic has to work reliably with respect to structure.

Having said that what I need is the reproduction of issue, before trying to figure out where handling is incorrect. This makes it easier to see if and how the problem can be resolved as well as adding regression test(s) to guard against future breakages (for specific problem).

@cowtowncoder cowtowncoder added the need-test-case To work on issue, a reproduction (ideally unit test) needed label Apr 2, 2020
@cowtowncoder
Copy link
Member

Would need a reproduction; may be re-opened/re-filed with one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need-test-case To work on issue, a reproduction (ideally unit test) needed
Projects
None yet
Development

No branches or pull requests

2 participants