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

Behavior of when FromXmlParser.Feature.EMPTY_ELEMENT_AS_NULL enabled different after 2.12.0 #579

Closed
lazyr1ch opened this issue Mar 2, 2023 · 17 comments
Labels
will-not-fix Issue for which there is no plan to fix as described or requested

Comments

@lazyr1ch
Copy link

lazyr1ch commented Mar 2, 2023

I have a class

@Data
public class FooView {
    private String str;
}

and enabled feature FromXmlParser.Feature.EMPTY_ELEMENT_AS_NULL for XmlMapper.
Then i trying to call
FooView fooView = xmlMapper.readValue(content, FooView.class)
and set content as:
<Content/>
As result i've had a NP:
java.lang.NullPointerException: Cannot invoke method getStr() on null object

Before 2.12.0 returned result object (fooView) was not null object.

Version information
2.12.1 and greater. 2.12.0 gets another bug too.

@lazyr1ch lazyr1ch added the to-evaluate Issue that has been received but not yet evaluated label Mar 2, 2023
@cowtowncoder
Copy link
Member

This is not enough to reproduce your issue: I'd need one to be able to help. So just a unit test that specifies exact calls (description has most of it but I am not sure how you get NPE)

Note, too, that reproduction can not use other frameworks like Lombok: they need to be stand-alone.

@cowtowncoder cowtowncoder removed the to-evaluate Issue that has been received but not yet evaluated label Mar 3, 2023
@cowtowncoder
Copy link
Member

cowtowncoder commented Mar 5, 2023

Also, wrong repo, transferred to jackson-dataformat-xml

@cowtowncoder cowtowncoder transferred this issue from FasterXML/jackson-databind Mar 5, 2023
@cowtowncoder
Copy link
Member

No information to reproduce; may be re-opened/re-filed with test case

@lazyr1ch
Copy link
Author

lazyr1ch commented May 22, 2023

import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.dataformat.xml.XmlMapper;
import com.fasterxml.jackson.dataformat.xml.deser.FromXmlParser;
import org.junit.Test;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.util.Objects;

import static org.junit.Assert.assertNotNull;

public class TestMappingOfInputView {
	final static Logger logger = LoggerFactory.getLogger(TestMappingOfInputView.class);

	public static class TestView {
		private String value = "defaultValue";

		public String getValue() {
			return value;
		}

		public void setValue(String value) {
			this.value = value;
		}

		@Override
		public String toString() {
			return "TestView{" +
					"value='" + value + '\'' +
					'}';
		}

		@Override
		public boolean equals(Object o) {
			if (this == o) return true;
			if (o == null || getClass() != o.getClass()) return false;

			TestView testView = (TestView) o;

			return Objects.equals(value, testView.value);
		}

		@Override
		public int hashCode() {
			return value != null ? value.hashCode() : 0;
		}
	}
	
	@Test
	public void test1() throws Exception {
		ObjectMapper mapper = makeXmlMapper();
		
		String xml = "<Foo/>";
		TestView testView = mapper.readValue(xml, TestView.class);
		
		assertNotNull(testView);
	}

	private ObjectMapper makeXmlMapper() {
		XmlMapper mapper = new XmlMapper();
		mapper.enable(FromXmlParser.Feature.EMPTY_ELEMENT_AS_NULL);
		return mapper;
	}
}

@lazyr1ch
Copy link
Author

@cowtowncoder Hi! I added test for my case. For jackson version < 2.12.0 test complete sucessfully, but for >=2.12.0 test is failed.

@cowtowncoder
Copy link
Member

@pavelturch Ok so do you expect testView to be null or not? Assertion suggests:

 assertNotNull(testView);

not but since 2.15 seems to produce non-null Object I am bit confused about this bug report (as test as written does not fail).

Which versions have you tested?

@lazyr1ch
Copy link
Author

@pavelturch Ok so do you expect testView to be null or not? Assertion suggests:

 assertNotNull(testView);

not but since 2.15 seems to produce non-null Object I am bit confused about this bug report (as test as written does not fail).

Which versions have you tested?

I'm expecting that testView is not null. I was tested it on 2.12.1, 2.12.7, 2.13.5, 2.14.3, 2.15.1 and I got null object in assertion. Runs under OpenJDK 1.8. My maven project have a parent "spring-boot-starter-parent 2.5.14" and I override jackson-bom.version property. I checked dependency conflicts and make sure that overrided version is used. What else can affect?

@cowtowncoder cowtowncoder reopened this May 22, 2023
@cowtowncoder
Copy link
Member

I'll have to first try to reproduce this, but... I also think that from name EMPTY_ELEMENT_AS_NULL it seems like it actually should produce null, not an "empty" Object. Regardless of how Jackson 2.12.x behaved.

@lazyr1ch
Copy link
Author

lazyr1ch commented May 22, 2023 via email

@cowtowncoder
Copy link
Member

@pavelturch I think I should re-phrase my question: do you really need to want to enable EMPTY_ELEMENT_AS_NULL? There should typically be no need for that unless you really want nulls. So it'd make sense to just disable it in your case.

@lazyr1ch
Copy link
Author

@cowtowncoder I really need a backward compatibility for our API :) And yes, I want to enable EMPTY_ELEMENT_AS_NULL.

@cowtowncoder
Copy link
Member

cowtowncoder commented May 24, 2023

Ok. Realistically speaking if all versions since 2.12.1 return null, I think it is the case that right now behavior seen is the standard expected behavior: changing it to not return null would be backwards incompatible change despite it once upon a time behaving in a different way.

Looking at release notes, I think #435 is the fix that changed behavior.
As far I can see feature description explains expected semantics.
I will still need to add an actual test case (hope to get to that tomorrow after resolving other issues) but it looks like I might consider this "behaves as intended" and close.

NOTE: one possibility for supporting alternate behavior while retaining compatibility is to add more Feature choices. This is a possibility to separate handling at root value level.

@cowtowncoder cowtowncoder changed the title Enabled FromXmlParser.Feature.EMPTY_ELEMENT_AS_NULL breaks up old functionality since 2.12.0 Behavior of when FromXmlParser.Feature.EMPTY_ELEMENT_AS_NULL enabled different after 2.12.0 May 24, 2023
@lazyr1ch
Copy link
Author

lazyr1ch commented May 24, 2023

Ok. I understand, thank you.

Here is another case with mapper.enable(FromXmlParser.Feature.EMPTY_ELEMENT_AS_NULL).
For version 2.11.x in test1 I get result1 is non-null, result2 is non-null object too, but result3.getValue() is null-object.
For version >=2.12.1 in test1 I get result1 is null, result2 is null, result 3 is null.
It seems that I won't be able to use the new version until I refactor the code and disable EMPTY_ELEMENT_AS_NULL. But the risks still remain.

`
@test
public void test1() throws Exception {
ObjectMapper mapper = makeXmlMapper();

	String xml = "<Foo/>";
	TestView result1 = mapper.readValue(xml, TestView.class);

	assertNotNull(result1);
}

@Test
public void test2() throws Exception {
	ObjectMapper mapper = makeXmlMapper();

	String xml = "<Foo/>";
	List<TestView> result2 = mapper.readValue(xml, new TypeReference<List<TestView>>() {
	});

	assertNotNull(result2);
}

@Test
public void test3() throws Exception {
	ObjectMapper mapper = makeXmlMapper();

	String xml = "<Foo><value/></Foo>";
	TestView result3 = mapper.readValue(xml, TestView.class);

	assertNull(result3.getValue());
}

`

@cowtowncoder
Copy link
Member

Yes, in general with this setting any and all empty XML elements (<elem/>) would be exposed as null tokens so in that sense your result is consistent with intent of this flag.

That said I think it's not a great feature, partly as in XML there is no semantic distinction between <elem/> and <elem></elem> -- yet this feature introduces distinction. So perhaps it should be deprecated (or maybe even removed) from Jackson 3.0 altogether. Its semantics are unexpected and implementation brittle.

But for what it is worth I don't think behavior wrt root element will be changed at this point.

cowtowncoder added a commit that referenced this issue May 29, 2023
@cowtowncoder
Copy link
Member

Ok: this works as intended. I added a test to verify the behavior as sort of documentation.

@cowtowncoder cowtowncoder added the will-not-fix Issue for which there is no plan to fix as described or requested label May 29, 2023
@lazyr1ch
Copy link
Author

@cowtowncoder Ok, thanks

@cowtowncoder
Copy link
Member

@lazyr1ch no problem & apologies for the mess we got here. I don't like introducing backwards-incompatible changes like this. Thank you for your understanding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
will-not-fix Issue for which there is no plan to fix as described or requested
Projects
None yet
Development

No branches or pull requests

2 participants