Skip to content

(3.0) NullNode.stringValue() should return null ... #5288

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.

Sign up for GitHub

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 6 commits into from
Sep 5, 2025
Merged

(3.0) NullNode.stringValue() should return null ... #5288

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0ca929e
Resolve 5287
JooHyukKim Aug 31, 2025
d56c74b
Merge branch '3.x' into resolve-5287
cowtowncoder Sep 3, 2025
9d49cc1
Minor fixes, javadoc changes
cowtowncoder Sep 3, 2025
677323f
Merge branch '3.x' into resolve-5287
cowtowncoder Sep 4, 2025
6ad1c4c
Partial revert based on discussions
cowtowncoder Sep 4, 2025
0838f18
Merge branch 'resolve-5287' of github.com:JooHyukKim/jackson-databind…
cowtowncoder Sep 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions release-notes/VERSION
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ Versions: 3.x (for earlier see VERSION-2.x)
#5270: Remove `ObjectMapper.setSerializationInclusion()` from 3.0 (deprecate in 2.20)
#5272: Replace `ObjectMapper.getRegisteredModuleIds()` (2.x) with
`ObjectMapper.getRegisteredModules()` (3.0)
#5287: Change JsonNode.stringValue() of NullNode to return null, not fail (3.0)

3.0.0-rc8 (13-Aug-2025)

Expand Down
23 changes: 14 additions & 9 deletions src/main/java/tools/jackson/databind/JsonNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -545,36 +545,41 @@ public Optional<JsonNode> asOptional() {

/**
* Method that will try to access value of this node as a Java {@code String}
* which works if (and only if) node contains JSON String value:
* which works if (and only if) node contains JSON String or {@code null} value:
* if not, a {@link JsonNodeException} will be thrown.
* In case of JSON {@code null}, Java {@code null} is returned.
*<p>
* NOTE: for more lenient conversions, use {@link #asString()}
* NOTE: for more conversions, use {@link #asString()} instead.
*<p>
* NOTE: in Jackson 2.x, was {@code textValue()}.
* NOTE: in Jackson 2.x, this method was named {@code textValue()}.
*
* @return {@code String} value this node represents (if JSON String)
* @return {@code String} value this node represents (if JSON String),
* {@code null} for JSON {@code null}
*
* @throws JsonNodeException if node value is not a JSON String value
* @throws JsonNodeException if node value is not a JSON String or Null value
*/
public abstract String stringValue();

/**
* Method similar to {@link #stringValue()}, but that will return specified
* {@code defaultValue} if this node does not contain a JSON String.
* {@code defaultValue} if this node does not contain a JSON String. This
* default value case includes JSON {@code null}.
*
* @param defaultValue Value to return if this node does not contain a JSON String.
*
* @return Java {@code String} value this node represents (if JSON String);
* {@code defaultValue} otherwise
* {@code defaultValue} otherwise -- only returns {@code null} if {@code defaultValue}
* is {@code null}
*/
public abstract String stringValue(String defaultValue);

/**
* Method similar to {@link #stringValue()}, but that will return
* {@code Optional.empty()} if this node does not contain a JSON String.
* {@code Optional.empty()} if this node does not contain a JSON String
* (NOTE: JSON null is not considered a String here)
*
* @return {@code Optional<String>} value (if node represents JSON String);
* {@code Optional.empty()} otherwise
* {@code Optional.empty()} otherwise (including for JSON null)
*/
public abstract Optional<String> stringValueOpt();

Expand Down
13 changes: 13 additions & 0 deletions src/main/java/tools/jackson/databind/node/NullNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,19 @@ protected String _asString() {
return "";
}

// Explicit overrides for all overloads for documentation purposes

@Override
public String stringValue() { return null; }

@Override
public String stringValue(String defaultValue) { return defaultValue; }

@Override
public Optional<String> stringValueOpt() {
return Optional.empty();
}

/*
/**********************************************************************
/* Overridden JsonNode methods, scalar access, numeric
Expand Down
14 changes: 12 additions & 2 deletions src/test/java/tools/jackson/databind/node/JsonNodeStringValueTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
import tools.jackson.databind.testutil.DatabindTestUtil;
import tools.jackson.databind.util.RawValue;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
import static org.junit.jupiter.api.Assertions.*;

/**
Expand Down Expand Up @@ -65,10 +65,20 @@ public void stringValueFromStructural()
@Test
public void stringValueFromNonNumberMisc()
{
_assertStringValueFailForNonString(NODES.nullNode());
_assertStringValueFailForNonString(NODES.missingNode());
}

@Test
public void stringValueFromNullNode()
{
JsonNode node = NODES.nullNode();
assertEquals(null, node.stringValue());

// But also check defaulting
assertEquals("foo", node.stringValue("foo"));
Copy link
Member

@cowtowncoder cowtowncoder Sep 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

No: should return null, now that it is legitimate String value.

Copy link

@sdeleuze sdeleuze Sep 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think I am stongly in favor of keeping the PR as it was since null typically express the absence of value. I think the least surprising behavior is the previous PR implementation, with node.stringValue("foo") returning "foo" and node.stringValueOpt().isPresent() returning false when the string is null. Both should work consistently as a default value is supposed to be returned in the absence of value.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm okay I am a bit confused now.
Seems like we missed talking what each case should handle defaultValue.

Which one should be?

  1. assertEquals("foo", nullNode.stringValue("foo"));
  2. assertEquals(null, nullNode.stringValue("foo"));

Let's continue discussion on #5287

Copy link
Member

@cowtowncoder cowtowncoder Sep 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Hmmh. I guess I changed my mind -- will revert to earlier state. This mostly thanks to Optional limitation.

assertFalse(node.stringValueOpt().isPresent());
}

// // // asString() tests

@Test
Expand Down
Loading