Skip to content

Conversation

@GKotsifos
Copy link

Fixes Json Module should output type error on iterables that are not encodable

Description of changes in release / Impact of release:

Updates the JSON encoder to reject user-defined iterables, raising a TypeError instead of serializing them as arrays.

Documentation

(insert text here)

Risks of this release

Is this a breaking change?

  • Yes
  • No

If you answered Yes then describe why is it so

(insert text here if applicable)

Is there a way to disable the change?

  • Use previous release
  • Use a feature flag
  • No

Additional details go here

(insert text here if applicable)

// e.g. tuple, list
if (x instanceof StarlarkIterable) {
// only real tuple, list JSON arrays
if (x instanceof StarlarkList || x instanceof Tuple) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have a test for this? I understand why, but would this break behavior that folks are depending on?

Copy link
Author

Choose a reason for hiding this comment

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

Fair point, if anyone is serializing custom iterables then this change will break them. Regarding the test, I was thinking of adding one but saw this line:

// Tests at //src/test/java/net/starlark/java/eval:testdata/json.star

and could not find a json.star file so I assumed there are not tests for json. I can create some if needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@GKotsifos the tests are here, I can see how that's confusing:

https://github.com/verygoodsecurity/starlarky/blob/master/libstarlark/src/test/java/net/starlark/java/eval/testdata/json.star

Maybe update the comment to say it's at libstarlark/...

Copy link
Author

Choose a reason for hiding this comment

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

@mahmoudimus correct me if I am wrong but these tests are for this class:

https://github.com/verygoodsecurity/starlarky/blob/master/libstarlark/src/main/java/net/starlark/java/lib/json/Json.java

and not the JsonModule.java where the changes are:

https://github.com/verygoodsecurity/starlarky/blob/master/larky/src/main/java/com/verygood/security/larky/modules/JsonModule.java

although now that you mention it we should probably apply the same fix to Json.java as well.

Also looking at these tests, I saw this line:

assert_eq(json.encode(range(3)), "[0,1,2]") # a built-in iterable

where even though range is a built-in iterable it should still fail since in python only lists and tuples are mapped by default to JSON arrays and I assume we want to mimic the exact behavior of python. But the description of the issue only states we should reject user-defined iterables. What do you think?

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.

Json Module should output type error on iterables that are not encodable

2 participants