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

Update the JSON toString() to allow better debugging #4659

Merged
merged 8 commits into from
Apr 6, 2023

Conversation

pmlopes
Copy link
Contributor

@pmlopes pmlopes commented Apr 3, 2023

Motivation:

Vert.x JsonObject/JsonArray toString() methods call the codec encode method. While this is handy for debugging it has several drawbacks. When dealing with very large objects, a log statement or debug can incur on some performance penalty. Worse, when debugging, IDEs will try to show you a string representation of the object so it can impact your productivity.

When debugging circular references, you will notice that the debugger will constantly throw StackOverflowError slowing your IDE to a non use state.

This PR introduces a breaking change in the toString() methods of JsonObject and JsonArray, note that this should not impact most users as JSON encoding should always be done with encode() or encodePrettilly().

The changes try to follow what other runtimes do. Like with V8 or Python inspect/dirs, the proposed toString() methods will return a JSON like representation of the object/array. It will enforce a few limits:

  1. max nested objects/arrays 3 (this means that deep objects will be shown as ellipsis, but calling toString() from a nested object will start counting from that node)
  2. Arrays and Objects will only list the 1st 100 elements
  3. String values will be truncated to the 1st 10000 characters
  4. Circular references are detected and identified by the system hashcode of the json wrapper container.

The reason to avoid using the container directly is that, although Map and List will detect self references, they would fail to detect other circular references, leading again to StackOverflowError.

Signed-off-by: Paulo Lopes <pmlopes@gmail.com>
Signed-off-by: Paulo Lopes <pmlopes@gmail.com>
Signed-off-by: Paulo Lopes <pmlopes@gmail.com>
Signed-off-by: Paulo Lopes <pmlopes@gmail.com>
Signed-off-by: Paulo Lopes <pmlopes@gmail.com>
Signed-off-by: Paulo Lopes <pmlopes@gmail.com>
@tsegismont
Copy link
Contributor

This would be in Vert.x 5 only, right? Asking because of the breaking aspect.

Is the inspection format standardized or have you inspired from what other platforms provide?

@pmlopes
Copy link
Contributor Author

pmlopes commented Apr 3, 2023

@tsegismont yes, this is 5.0.0 only. The format is similar to what the java Map/List does with a small difference:

e.g.:

// java map toString() shows as:
{key=value,keyN=valueN}
// java list toString() renders:
[1,2,null,true,{key=value}]

This format uses : instead of = for maps as it's closer to what JSON does, and it includes 1 space after a , or : for readability. But I'm totally fine to make it more "java-ish" by removing the whitespace and use =. As that still keeps basic debugging readable and non-intrusive in debug sessions.

V8/node uses the format the PR uses, but does escape strings, to avoid showing stuff like:

// assume {"my\nkey", "value"} being printed as:
{my
key: "value"}

And node/python do have options for ascii coloring and choosing between compact (single line output) versus (indented output). Given that in the JDK the norm is to use a compact toString() I've kept the system simple and only do single line output.

@pmlopes pmlopes added this to the 5.0.0 milestone Apr 3, 2023
@pmlopes
Copy link
Contributor Author

pmlopes commented Apr 3, 2023

This PR is very useful for:

eclipse-vertx/vertx-json-schema#100
eclipse-vertx/vertx-openapi#24

@Yaytay
Copy link
Member

Yaytay commented Apr 3, 2023

When logging large objects having the output be JSON is particularly useful because we can stick it somewhere else, reformat it and read it easily.
I really dislike the Java map output because it makes that a lot more awkward.
So I definitely prefer the ":".

Copy link
Contributor

@tsegismont tsegismont left a comment

Choose a reason for hiding this comment

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

LGTM

@tsegismont
Copy link
Contributor

@pmlopes as discussed off-list, it would be nice to have a switch for old behavior as we had for Base64 encoding/decoding

Signed-off-by: Paulo Lopes <pmlopes@gmail.com>
@pmlopes
Copy link
Contributor Author

pmlopes commented Apr 3, 2023

I've added a flag vertx.json.tostring=legacy to get the <5.0.0 behavior (toString is an alias to encode).

@pmlopes
Copy link
Contributor Author

pmlopes commented Apr 3, 2023

@Yaytay this PR will not change the JSON encoding, you can still encode it with obj.encode() which is what obj.toString() was doing. What the PR changes is that toString() shall now return a debug friendly string that will not bring down your VM if you made a circular reference somewhere.

For example on a large object toString() will return:

{created: "2020-05-12T15:10:37Z", device: {device_info: {device_fw: 204, device_sn: "06-02133", device_trait: 2, device_type: 190}, timeseries: [{...}]}}

Note the {...} which means that that object is deep, so if you need to print it you do:

// using dot notation to avoid writing all the getJsonObject() calls...
obj.device.device_info.timeseries.toString()

BUT if you do:

obj.encode()

You still see the full thing:

{"created":"2020-05-12T15:10:37Z","device":{"device_info":{"device_fw":204,"device_sn":"06-02133","device_trait":2,"device_type":190},"timeseries":[{"configuration":{"sensors":[{"measurements":["BATTERY","BATTERY_MV"],"port":7,"sensor_bonus_value":"Unavailable","sensor_firmware_ver":"Unavailable","sensor_number":133,"sensor_sn":"Unavailable"},{"measurements":["REFERENCE_KPA","TEMPC_LOGGER"],"port":8,"sensor_bonus_value":"Unavailable","sensor_firmware_ver":"Unavailable","sensor_number":134,"sensor_sn":"Unavailable"}],"valid_since":"2018-08-11T21:45:00Z","values":[[1534023900,0,19,[{"description":"Battery Percent","error":false,"units":"%","value":100},{"description":"Battery Voltage","error":false,"units":" mV","value":7864}],[{"description":"Reference Pressure","error":false,"units":" kPa","value":100.62},{"description":"Logger Temperature","error":false,"units":" °C","value":28.34}]]]}}]}}

Signed-off-by: Paulo Lopes <pmlopes@gmail.com>
@pmlopes pmlopes merged commit 6582902 into master Apr 6, 2023
@vietj
Copy link
Member

vietj commented Apr 9, 2023

this PR has been merged to rapidly, it broke the build of vertx-sql-client at least (https://github.com/eclipse-vertx/vertx-sql-client/actions/runs/4643518385/jobs/8218213912) and it might have breaken more.

I will revert it for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants