Skip to content

Fix for #615: Use ValueInstantiators to rename creator properties so they match property definitions#806

Merged
cowtowncoder merged 10 commits intoFasterXML:3.xfrom
mcvayc:fix/issue-615
Mar 16, 2026
Merged

Fix for #615: Use ValueInstantiators to rename creator properties so they match property definitions#806
cowtowncoder merged 10 commits intoFasterXML:3.xfrom
mcvayc:fix/issue-615

Conversation

@mcvayc
Copy link
Contributor

@mcvayc mcvayc commented Mar 15, 2026

Hi,

This PR was intended to fix #615 but in the process tests for #149, #517, #735, #795 in the tofix package started passing as well. I've listed all the issues for which test are now passing in the commits, but still need to verify for those that the entire issue is fixed.

The initial solution was co-written by Claude Code and me. After collaboratively creating a fix, I subsequently read through the code and edited it for quality and consistency. I believe the solution is now in decent shape, at least for #615.

Please let me know what you think. I'm happy to make further edits.

// Second check: rename map from property definitions
String newName = renames.get(propName);
if (newName != null && !newName.equals(propName)) {
creatorProps[i] = prop.withSimpleName(newName);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Other than directly editing the array items, is there a better way to update the creator properties of the instantiator? I didn't see a setter.

Copy link
Member

Choose a reason for hiding this comment

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

That does seem dangerous. Ideally would not tweak internal data structures nor have setters but create new instances.

I'll have to think about this one.

Copy link
Contributor Author

@mcvayc mcvayc Mar 16, 2026

Choose a reason for hiding this comment

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

I added a proposed improvement in the latest commit. This is safer but less performant, since we copy the array even if there is no need. Another option would be to only copy the array after we identify the first rename, but then we need to check if the array is not null each time that we rename.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is acceptable: this does not occur for every read but only when deserializer is constructed.

// [dataformat-xml#423]
@JacksonTestFailureExpected
@Test
public void testXmlTextViaCtor423() throws Exception
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test started passing so I split it out so it could be removed from the tofix package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test now resides in src/test/java/tools/jackson/dataformat/xml/deser/XmlTextViaCtor423Test.java

@mcvayc
Copy link
Contributor Author

mcvayc commented Mar 15, 2026

I see the build errors, let me take a look.

@mcvayc mcvayc force-pushed the fix/issue-615 branch 2 times, most recently from 8b1cbb5 to c9b82bb Compare March 15, 2026 22:25
@mcvayc
Copy link
Contributor Author

mcvayc commented Mar 15, 2026

Sorry about that. I somehow missed adding a module file to the commit.

I've rebased and confirmed that everything is passing on my end.

[INFO] --- jacoco:0.8.14:report (report) @ jackson-dataformat-xml ---
[INFO] Loading execution data file C:\Users\chips\Documents\GitHub\mcvayc\jackson-dataformat-xml\target\jacoco.exec
[INFO] Analyzed bundle 'Jackson-dataformat-XML' with 53 classes
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  15.826 s
[INFO] Finished at: 2026-03-15T18:36:53-04:00
[INFO] ------------------------------------------------------------------------

@cowtowncoder cowtowncoder added the cla-received PR already covered by CLA (optional label) label Mar 15, 2026
@mcvayc
Copy link
Contributor Author

mcvayc commented Mar 15, 2026

Additionally, since the rebase it looks like test #795 now also passes so I've also moved that one out of tofix.

@github-actions
Copy link

🧪 Code Coverage Report

Metric Coverage Change
Instructions coverage 71.13% 📈 +0.660%
Branches branches 65.50% 📈 +0.210%

Coverage data generated from JaCoCo test results

@github-actions
Copy link

🧪 Code Coverage Report

Metric Coverage Change
Instructions coverage 71.20% 📈 +0.730%
Branches branches 65.48% 📈 +0.190%

Coverage data generated from JaCoCo test results

@github-actions
Copy link

🧪 Code Coverage Report

Metric Coverage Change
Instructions coverage 71.22% 📈 +0.720%
Branches branches 65.76% 📈 +0.180%

Coverage data generated from JaCoCo test results

@github-actions
Copy link

🧪 Code Coverage Report

Metric Coverage Change
Instructions coverage 71.22% 📈 +0.720%
Branches branches 65.76% 📈 +0.180%

Coverage data generated from JaCoCo test results

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!

@mcvayc
Copy link
Contributor Author

mcvayc commented Mar 16, 2026

LGTM!

Thank you! I found one more optimization I'd like to push.

@github-actions
Copy link

🧪 Code Coverage Report

Metric Coverage Change
Instructions coverage 71.21% 📈 +0.710%
Branches branches 65.83% 📈 +0.250%

Coverage data generated from JaCoCo test results

@cowtowncoder
Copy link
Member

@mcvayc This looks very good -- thank you for contributing it! I'll add release notes and merge it for inclusion in 3.2.0!

@cowtowncoder
Copy link
Member

LGTM!

Thank you! I found one more optimization I'd like to push.

Ok I'll wait after updating release notes.

@mcvayc
Copy link
Contributor Author

mcvayc commented Mar 16, 2026

LGTM!

Thank you! I found one more optimization I'd like to push.

Ok I'll wait after updating release notes.

Never mind, you beat me to it with your streamlining commit. :-)

I was going to delay the array copy.

@cowtowncoder
Copy link
Member

cowtowncoder commented Mar 16, 2026

@mcvayc Ok; good to, will merge. Looks like #306 is still partially failing, but fixing 5 issues is a pretty big achievement!

@github-actions
Copy link

🧪 Code Coverage Report

Metric Coverage Change
Instructions coverage 71.21% 📈 +0.710%
Branches branches 65.83% 📈 +0.250%

Coverage data generated from JaCoCo test results

@cowtowncoder cowtowncoder merged commit 7ebba38 into FasterXML:3.x Mar 16, 2026
3 checks passed
@cowtowncoder
Copy link
Member

@mcvayc Ok; good to, will merge. Looks like #306 is still partially failing, but fixing 5 issues is a pretty big achievement!

I am working on #306 -- test is buggy.

cowtowncoder added a commit that referenced this pull request Mar 16, 2026
@mcvayc
Copy link
Contributor Author

mcvayc commented Mar 16, 2026

@mcvayc Ok; good to, will merge. Looks like #306 is still partially failing, but fixing 5 issues is a pretty big achievement!

I am working on #306 -- test is buggy.

Makes sense.

Thanks for all that you do for the Jackson projects and for being so supportive of contributions.

cowtowncoder added a commit that referenced this pull request Mar 16, 2026
@cowtowncoder
Copy link
Member

Thanks for all that you do for the Jackson projects and for being so supportive of contributions.

Thank you for the kind words. It is good to get feedback like this.

And thank you for providing the fixes here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-received PR already covered by CLA (optional label)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Deserialization of Xml with @JacksonXmlText using @JsonCreator (into java.util.Map) fails

2 participants