Skip to content

Fix regression in ObjectNode.with() #5099

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

Merged
merged 2 commits into from
Apr 22, 2025
Merged

Conversation

rschmitt
Copy link
Contributor

This fixes a regression that seems to have been introduced in d7adffa.

This fixes a regression that seems to have been introduced in
d7adffa.
@@ -87,7 +87,8 @@ public ObjectNode with(String exprOrProperty) {
.getClass().getName() + "`)");
}
ObjectNode result = objectNode();
return _put(exprOrProperty, result);
_put(exprOrProperty, result);
return result;
Copy link
Member

Choose a reason for hiding this comment

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

What does _put return to create regression ? Seems like it should work.

Copy link
Member

@cowtowncoder cowtowncoder Apr 22, 2025

Choose a reason for hiding this comment

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

I looked at original change and I think it's the difference between returning this (which _put() returns) vs ObjectNode being created that is to be returned here.

But I'd appreciate it if you can double-check @JooHyukKim .

Copy link
Member

Choose a reason for hiding this comment

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

Yeah you are right on returing this vs created-ObjectNode

@cowtowncoder
Copy link
Member

First of all, thank you for reporting this & providing patch! Good catch.

Second: before merging the fix, I'd need a CLA (unless you have sent one in the part OR are covered by one of CCLAs (there are a few bigger corporations that have submitted ones)), from:

https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf

it only needs to be sent once, before the first merged PR. The usual way is to print it, fill & sign, scan/photo, email to cla at fasterxml dot com.
Once we get it I can proceed with merging the fix, which will make it in 2.19.0 release (as I said, good catch!).

@rschmitt
Copy link
Contributor Author

I'd need a CLA (unless you have sent one in the part OR are covered by one of CCLAs (there are a few bigger corporations that have submitted ones))

Amazon is covered under an existing CCLA.

@cowtowncoder
Copy link
Member

I'd need a CLA (unless you have sent one in the part OR are covered by one of CCLAs (there are a few bigger corporations that have submitted ones))

Amazon is covered under an existing CCLA.

Yes indeed. So this is under that CCLA -- we are good, thanks!

ps. No wonder name sounded familiar -- I should have checked my emails first. Hope things are going well!

@cowtowncoder cowtowncoder added the cla-received PR already covered by CLA (optional label) label Apr 22, 2025
@cowtowncoder cowtowncoder changed the title Fix regression in ObjectNode.with() Fix regression in ObjectNode.with() Apr 22, 2025
@cowtowncoder cowtowncoder merged commit 21a11f4 into FasterXML:2.19 Apr 22, 2025
6 checks passed
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.

3 participants