Skip to content
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

JsonStreamContext "currentValue" wrongly references to @JsonTypeInfo annotated object #3160

Closed
aritzbastida opened this issue May 18, 2021 · 5 comments
Milestone

Comments

@aritzbastida
Copy link

aritzbastida commented May 18, 2021

Describe the bug
When a POJO attribute contains an object of a class annotated with @JsonTypeInfo, the method serializeWithType() changes the current value, so that it references that inner object when processing it (such as MailAttachment, in the example below). This behavior was introduced due to #631.

However, this current value also has a side-effect on subsequent sibling attributes. In the following sample, the JsonGenerator would state MailStatement as current value when processing "actions" and "properties". This behaviour breaks Jackson Property Filters, such as Squiggly (https://github.com/bohnman/squiggly/issues/62)

Note that if the Attachment class has no @JsonTypeInfo annotation, then "actions" and "properties" correctly state Issue object as current value.

{
	"id": "ISSUE-1",
	"issueSummary": "Dragons Need Fed",
	"issueDetails": "I need my dragons fed pronto.",
	"reporter": {
		"firstName": "Daenerys",
		"lastName": "Targaryen",
		"entityType": "User"
	},
	"assignee": {
		"firstName": "Jorah",
		"lastName": "Mormont",
		"entityType": "User"
	},
	"attachment": {
		"@class": ".MailAttachment",
		"id": "1234",
		"name": "Mail",
		"from": "from@mail.com",
		"to": "to@mail.com"
	},
	"actions": [
		{
			"id": null,
			"type": "COMMENT",
			"text": "I'm going to let Daario get this one..",
			"user": {
				"firstName": "Jorah",
				"lastName": "Mormont",
				"entityType": "User"
			}
		},
		{
			"id": null,
			"type": "CLOSE",
			"text": "All set.",
			"user": {
				"firstName": "Daario",
				"lastName": "Naharis",
				"entityType": "User"
			}
		}
	],
	"properties": {
		"email": "motherofdragons@got.com",
		"priority": "1"
	}
}

Attribute processing, together with their JsonStreamContext "currentValue":

== id: com.github.bohnman.squiggly.model.Issue@222114ba
== issueSummary: com.github.bohnman.squiggly.model.Issue@222114ba
== issueDetails: com.github.bohnman.squiggly.model.Issue@222114ba
== reporter: com.github.bohnman.squiggly.model.Issue@222114ba
== firstName: com.github.bohnman.squiggly.model.User@1283bb96
===== reporter: com.github.bohnman.squiggly.model.Issue@222114ba
== lastName: com.github.bohnman.squiggly.model.User@1283bb96
===== reporter: com.github.bohnman.squiggly.model.Issue@222114ba
== entityType: com.github.bohnman.squiggly.model.User@1283bb96
===== reporter: com.github.bohnman.squiggly.model.Issue@222114ba
== assignee: com.github.bohnman.squiggly.model.Issue@222114ba
== firstName: com.github.bohnman.squiggly.model.User@74f0ea28
===== assignee: com.github.bohnman.squiggly.model.Issue@222114ba
== lastName: com.github.bohnman.squiggly.model.User@74f0ea28
===== assignee: com.github.bohnman.squiggly.model.Issue@222114ba
== entityType: com.github.bohnman.squiggly.model.User@74f0ea28
===== assignee: com.github.bohnman.squiggly.model.Issue@222114ba
== attachment: com.github.bohnman.squiggly.model.Issue@222114ba
== id: com.github.bohnman.squiggly.model.MailAttachment@409bf450
===== attachment: com.github.bohnman.squiggly.model.MailAttachment@409bf450
== name: com.github.bohnman.squiggly.model.MailAttachment@409bf450
===== attachment: com.github.bohnman.squiggly.model.MailAttachment@409bf450
== from: com.github.bohnman.squiggly.model.MailAttachment@409bf450
===== attachment: com.github.bohnman.squiggly.model.MailAttachment@409bf450
== to: com.github.bohnman.squiggly.model.MailAttachment@409bf450
===== attachment: com.github.bohnman.squiggly.model.MailAttachment@409bf450
== actions: com.github.bohnman.squiggly.model.MailAttachment@409bf450
== properties: com.github.bohnman.squiggly.model.MailAttachment@409bf450

Version information
2.9 - 2.12.3

@aritzbastida aritzbastida added the to-evaluate Issue that has been received but not yet evaluated label May 18, 2021
aritzbastida pushed a commit to aritzbastida/jackson-databind that referenced this issue May 19, 2021
…rary (implemented as a Jackson Property Filter)

Reported here: FasterXML#3160
@cowtowncoder
Copy link
Member

Quick question: is this during serialization (writing)?

I think I'd need a minimal reproduction: I trust there is an issue, but fixing it requires careful verification as testing of current value stability is bit limited.

@cowtowncoder cowtowncoder added need-test-case To work on issue, a reproduction (ideally unit test) needed and removed to-evaluate Issue that has been received but not yet evaluated labels May 26, 2021
aritzbastida pushed a commit to aritzbastida/jackson-databind that referenced this issue May 27, 2021
…ue" wrongly references to @JsonTypeInfo annotated object)
aritzbastida pushed a commit to aritzbastida/jackson-databind that referenced this issue May 27, 2021
…ue" wrongly references to @JsonTypeInfo annotated object)
@aritzbastida
Copy link
Author

Thanks for answering!!

Yes, it happens during serialization. In fact, the issue is caused by the change made in serializeWithType() (#631). I didn't check whether there is a similar problem during deserialization, though.

As for the test case, I extended testIssue2475 (see below) so that it breaks with the error: java.lang.Error: Field 'set', context not that of Item2475 instance. Once strategy attribute is processed, the next attribute (set) does no longer refer to the Item2475 class. Note that, if we remove @JsonTypeInfo annotation from Strategy, the filter works just OK.

To fix this test, we can revert the changes in #631; that is, remove the statement gen.setCurrentValue(bean); in serializeWithType() method. As a result, we would have a consistent behavior for filters, not depending on @JsonTypeInfo any more. I checked that the whole jackson-databind test suite passes without errors. Note, however, this will break (external) custom serializers which demanded this specific behavior for polymorphic types.

package com.fasterxml.jackson.databind.ser;

public class TestCustomSerializers extends BaseMapTest
{

    @JsonFilter("myFilter")
    @JsonPropertyOrder({ "id", "strategy", "set" })
    public static class Item2475 {
    	
        private Collection<String> set;
        private Strategy strategy;
		private String id;

        public Item2475(Collection<String> set, String id) {
            this.set = set;
            this.strategy = new Foo(42);
            this.id = id;
        }

        public Collection<String> getSet() {
            return set;
        }
        
        public Strategy getStrategy() {
			return strategy;
		}        

        public String getId() {
            return id;
        }
    }
      
    @JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "type")    
    @JsonSubTypes({ @JsonSubTypes.Type(name = "Foo", value = Foo.class) })
    interface Strategy { }

    static class Foo implements Strategy {
        public int foo;

        @JsonCreator
        Foo(@JsonProperty("foo") int foo) {
            this.foo = foo;
        }
    }     

    // [databind#2475]
    public void testIssue2475() throws Exception {
        SimpleFilterProvider provider = new SimpleFilterProvider().addFilter("myFilter", new MyFilter2475());
        ObjectWriter writer = MAPPER.writer(provider);
        
        // contents don't really matter that much as verification within filter but... let's
        // check anyway
        assertEquals(aposToQuotes("{'id':'ID-1','strategy':{'type':'Foo','foo':42},'set':[]}"),
        		writer.writeValueAsString(new Item2475(new ArrayList<String>(), "ID-1")));

        assertEquals(aposToQuotes("{'id':'ID-2','strategy':{'type':'Foo','foo':42},'set':[]}"),
        		writer.writeValueAsString(new Item2475(new HashSet<String>(), "ID-2")));
        
    }    
}

@cowtowncoder cowtowncoder removed the need-test-case To work on issue, a reproduction (ideally unit test) needed label May 28, 2021
@cowtowncoder
Copy link
Member

Thank you for adding a test case! I'll probably try to trim it down further.

As to specific place(s), I'll have to look; but serializeWithType() is defined in rather a lot of places.
In case of, say, BeanSerializerBase, I am guessing that just moving setting (which is needed in general, it cannot be just removed) after call to writeTypePrefix() might work.

@aritzbastida
Copy link
Author

Exactly, I referred to BeanSerializerBase, specifically the lines added as a result of [databind#631].

    @Override
    public void serializeWithType(Object bean, JsonGenerator gen, SerializerProvider provider, 
       TypeSerializer typeSer) throws IOException {

        if (_objectIdWriter != null) {
            gen.setCurrentValue(bean); // [databind#631]
            _serializeWithObjectId(bean, gen, provider, typeSer);
            return;
        }
        gen.setCurrentValue(bean); // [databind#631]
        ...
    }

I look forward to your analysis. Thanks!

@cowtowncoder cowtowncoder added this to the 2.13.0 milestone Jul 9, 2021
@cowtowncoder
Copy link
Member

Finally found time to check this and yes, ordering of that call is incorrect -- it needs to occur after START_OBJECT gets called, as that is the context in which current value should be updated. Test verifies this.
Will be included in 2.13.0-rc1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants