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

Allow booleans to be scalar refs #215

Closed
wants to merge 1 commit into from

Conversation

waterkip
Copy link

@waterkip waterkip commented May 9, 2020

The JSON validation breaks when one uses \1 or \0 when using the
serializion in conjuction with JSON::XS, Mojo::JSON and others.

Signed-off-by: Wesley Schwengle wesley@opndev.io

The JSON validation breaks when one uses `\1` or `\0` when using the
serializion in conjuction with JSON::XS, Mojo::JSON and others.

Signed-off-by: Wesley Schwengle <wesley@opndev.io>
@jhthorsen
Copy link
Owner

I don't see any reason to accept this, since scalar refs is not a thing in JSON. Please provide a better argument for why/when this is needed.

@waterkip
Copy link
Author

waterkip commented Jun 6, 2020

$ cat json-bool.pl; perl json-bool.pl
#!/usr/bin/perl
use warnings;
use strict;
require JSON::XS;
require JSON::PP;
require Mojo::JSON;
require Cpanel::JSON::XS;

print JSON::XS::encode_json({ foo => \1 }), $/;
print JSON::PP::encode_json({ foo => \1 }), $/;
print Mojo::JSON::encode_json({ foo => \1 }), $/;
print Cpanel::JSON::XS::encode_json({ foo => \1 }), $/;
print JSON::XS::encode_json({ foo => \0 }), $/;
print JSON::PP::encode_json({ foo => \0 }), $/;
print Mojo::JSON::encode_json({ foo => \0 }), $/;
print Cpanel::JSON::XS::encode_json({ foo => \0 }), $/;

{"foo":true}
{"foo":true}
{"foo":true}
{"foo":true}
{"foo":false}
{"foo":false}
{"foo":false}
{"foo":false}

They all serialize \1 and \0 to true and false. Which is why I think the validation must respect this syntax as well.

@waterkip waterkip removed their assignment Jun 6, 2020
@jhthorsen
Copy link
Owner

You still haven't given me any argument for when this is beneficial.

@waterkip
Copy link
Author

waterkip commented Jun 6, 2020

If I use \1 or \0 in code somewhere and I pass it to the Mojolious Plugin for OpenAPI and I set it to validate the JSON before sending it out it croaks. I dug around and saw that the JSON validator which is used by that plugin doesn't allow this syntax. So when I try to validate the output the output is considered invalid while when I disable the validation it works fine and returns a boolean value as shown in the json-bool.pl example with the various serializers.

Which leads me to 1) Not use the validation 2) Patch the validator that is accepts such values. I opted for 2 because I want to know when I do something wrong. Another workaround would be to serialize to json, deserialize it and then pass it to the validator after which another round of serialization happens. Which I find a bit counter intuitive.

@jhthorsen jhthorsen closed this in c64c8fb Jun 6, 2020
@jhthorsen
Copy link
Owner

Ooops! Closed the wrong issue :(

@jhthorsen jhthorsen reopened this Jun 6, 2020
@jhthorsen
Copy link
Owner

You should really just use false and true in that case. There's no way to send a scalar ref using JSON, so I'm pretty sure this is the wrong fix.

Keeping it open a bit longer though in case other people have relevant information.

@jhthorsen
Copy link
Owner

And when I say false and true, I mean functions such as YAML::XS::false or Mojo::JSON::true (and friends)

@waterkip
Copy link
Author

waterkip commented Jun 7, 2020

From the documentation of Mojo::JSON:

In addition scalar references will be used to generate booleans, based on if their values are true or false.

From the documentation of Types::Serializer (used by JSON::XS):

The recommended way to detect whether a scalar is one of these objects is to check whether the stash is the Types::Serialiser::Boolean or Types::Serialiser::Error stash, and then follow the scalar reference to see if it's 1 (true), 0 (false) or undef (error).

I think Mojo::JSON is used by Mojo and thus by the OpenAPI plugin. So you are saying you don't want to support that native serializer? Seems odd but ok.

@karenetheridge
Copy link
Contributor

Mojo::JSON doesn't use JSON::XS.

@waterkip
Copy link
Author

waterkip commented Jun 7, 2020

I never said that. I just quoted their documentation and the docs of JSON::XS.

jhthorsen pushed a commit that referenced this pull request Jun 8, 2020
 - JSON::Validator::schema() now holds a JSON::Validator::Schema object
   instead of Mojo::JSON::Pointer
 - Add schema classes for Draft4, Draft6 and Draft7
 - Add "duration" and "uuid" formats #210
 - Fix coercing boolean "false" #215
 - Fix not matching "null" should also be a "type" error #217
 - Deprecated JSON::Validator::joi()
 - Deprecated JSON::Validator::singleton()
 - Deprecated JSON::Validator::validate_json()
 - Deprecated JSON::Validator::version()
 - Removed JSON::Validator::generate_definitions_path()
 - Removed support for JSON::Validator::bundle({ref_key => ...})
@jhthorsen
Copy link
Owner

Going to close this issue, since I don't agree with the arguments given.

@jhthorsen jhthorsen closed this Jun 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants