Logging a Path object using structured logging throws StackOverflowError

[maxxedev]

Structured logging has infinite loop and throws StackOverflowError when attempting to log a Path object.

Tested with spring-boot 3.4.3 and latest 3.5.0-SNAPSHOT.

Example code:

System.setProperty("logging.structured.format.console", "logstash");
SpringApplication.run(DemoApplication.class, args);
LoggerFactory.getLogger(DemoApplication.class).atInfo()
		.addKeyValue("foo", Path.of("bar"))
		.log("test");

Exception:

Exception in thread "main" java.lang.StackOverflowError
	...
	at org.springframework.boot.json.JsonValueWriter.append(JsonValueWriter.java:282)
	at org.springframework.boot.json.JsonValueWriter.start(JsonValueWriter.java:144)
	at org.springframework.boot.json.JsonValueWriter.writeArray(JsonValueWriter.java:168)
	at org.springframework.boot.json.JsonValueWriter.write(JsonValueWriter.java:118)
	at org.springframework.boot.json.JsonValueWriter.writeElement(JsonValueWriter.java:190)

[gkdis6]

Hi @nosan ,
I think

else if (value instanceof Path p) {
    write(p.toString());
}

would be a more natural approach here.
Since Path.toString() already provides a platform-independent string representation of the path, logging it directly as a string seems more intuitive than converting it into a JSON array.
This would also align with how other objects like File are typically logged.

[nosan]

Hi @gkdis6,

Since Path.toString() already provides a platform-independent string representation of the path, logging it directly as a string seems more intuitive than converting it into a JSON array.

This is a good option as well.

---

• Jackson: Serializes using Path.toUri().toString() (source).
• Gson: Fails to serialize java.nio.file.Path (issue).
• Yasson: Serializes using Path.toString() (source).

So I would say there is no "right or wrong" approach. For example, this proposal suggests handling Path as an Iterable (JSON Array). Your suggested option is to use toString(), while Jackson's choice is to use URI.

Additionally, it's always possible to explicitly use something like addKeyValue("directory", Path.of("...").toString()).

[nosan]

I reviewed the JsonValueWriter.write(...) implementation again, and I believe the original intention was as follows:

• If it’s a Map, serialize it as a JSON object ({}).
• If it’s an Iterable or an array, serialize it as a JSON array ([]).
• Otherwise, serialize it as a JSON string.

I don’t think anyone intended to serialize java.nio.file.Path as a JSON array. Most likely, the fact that Path extends Iterable<Path> was simply overlooked.

I also checked different java.nio.file.Path JSON serializations:

Map<String, Object> files = new LinkedHashMap<>();
files.put("files",
List.of(Paths.get(".").toAbsolutePath(), Paths.get("1.jar").toAbsolutePath(), Paths.get("/root")));
String json = JsonWriter.standard().write(files).toJsonString();

Iterable Path.forEach():

{
  "files": [
    [
      "Users",
      "dmytronosan",
      "IdeaProjects",
      "spring-boot",
      "spring-boot-project",
      "spring-boot",
      "."
    ],
    [
      "Users",
      "dmytronosan",
      "IdeaProjects",
      "spring-boot",
      "spring-boot-project",
      "spring-boot",
      "1.jar"
    ],
    [
      "root"
    ]
  ]
}

Path.toString():

{
  "files": [
    "/Users/dmytronosan/IdeaProjects/spring-boot/spring-boot-project/spring-boot/.",
    "/Users/dmytronosan/IdeaProjects/spring-boot/spring-boot-project/spring-boot/1.jar",
    "/root"
  ]
}

Path.toUri().toString():

{
  "files": [
    "file:///Users/dmytronosan/IdeaProjects/spring-boot/spring-boot-project/spring-boot/./",
    "file:///Users/dmytronosan/IdeaProjects/spring-boot/spring-boot-project/spring-boot/1.jar",
    "file:///root"
  ]
}

I think treating java.nio.file.Path as a JSON array (Iterable) is quite unusual and meaningless.

With that said, I will update gh-44507 to use Path.toString().

[gkdis6]

When I said, “This would also align with how other objects like File are typically logged.”
I actually meant to talk about how 'Path.toString()' case should be represented.

I really appreciate your time and feedback. Thanks for reviewing! 👍

[mhalbritter]

I wonder if

else if (value instanceof Iterable<?> iterable) {
	writeArray(iterable::forEach);
}

is generally a good idea or if we should switch that to

else if (value instanceof Collection<?> collection) {
	writeArray(collection::forEach);
}

An Iterable can be an infinite "stream", while a Collection is bounded.

@philwebb what do you think?

[nosan]

I'm not sure that would help, as it's also possible to specify an unlimited (recursive) or deeply nested collection or map.
This inspired me to submit a PR to add protection against a StackOverflowError.

else if (value instanceof Iterable<?> iterable) {
writeArray(iterable::forEach);
}

This addresses only this particular issue.