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

Option for stringification of objects #37

Closed
Grinnz opened this issue May 27, 2015 · 19 comments
Closed

Option for stringification of objects #37

Grinnz opened this issue May 27, 2015 · 19 comments
Assignees
Milestone

Comments

@Grinnz
Copy link

Grinnz commented May 27, 2015

Based on the behavior of Mojo::JSON and some discussion on dealing with references there, this is a suggestion for an option to stringify blessed references instead of encoding to null or throwing an exception, when no TO_JSON method is found. Perhaps stringify all objects such as Mojo::JSON does currently (better for debugging), or another possibility is to detect if the object has overload defined. Either way would make Cpanel::JSON::XS compatible with Mojo::JSON for structures which contain e.g. Mojo::ByteStream objects, or DateTime objects.

@rurban rurban self-assigned this May 27, 2015
@rurban
Copy link
Owner

rurban commented May 27, 2015

I agree. I've tried both variants back and forth, and there needs to be an option.

@kraih
Copy link

kraih commented Jun 27, 2015

👍 I would really like to use that option.

@rurban
Copy link
Owner

rurban commented Jun 29, 2015

The counter argument is explained here: https://www.youtube.com/watch?v=Gzx6KlqiIZE&feature=youtu.be

serializers who allow deserialization of objects, and JSON and Data::MessagePack are the only ones currently who do not, are unsafe.
our security team has lot of troubles with those serializers.

@Grinnz
Copy link
Author

Grinnz commented Jun 29, 2015

This feature does not allow for deserialization of objects, it is only an additional way to serialize them; when deserialized, they will still always be strings.

@mrenvoize
Copy link

Seems like a nice to have :)

@nnutter
Copy link

nnutter commented Oct 17, 2015

👍 Being able to fallback on stringification of an object for serialization would be more useful than just getting null. Could do something like convert_blessed([qw(TO_JSON to_string)]) or make another option like stringify_blessed(1) that just did $obj . ''.

@zoffixznet
Copy link

👍

@rurban
Copy link
Owner

rurban commented Oct 22, 2015

Can somebody come up with a testcase please?
The true and false values would be \1 and \0, the Mojo::JSON docs are wrong there.

This test works ok:

use Test::More;
BEGIN {
  eval "require Mojo::JSON;";
  if ($@) {
    plan skip_all => "Mojo::JSON required for testing interop";
    exit 0;
  } else {
    plan tests => 6;
  }
}

use Mojo::JSON ();
use Cpanel::JSON::XS ();

my $booltrue = q({"is_true":true});
my $boolfalse = q({"is_false":false});
#my $boolpl     = { is_false => \0, is_true => \1 };
my $js = Mojo::JSON::decode_json( $booltrue );
is( $js->{is_true}, 1 );

my $cjson = Cpanel::JSON::XS->new;
is($cjson->encode( $js ), $booltrue)
  or diag "\$Mojolicious::VERSION=$Mojolicious::VERSION,".
  " \$Cpanel::JSON::XS::VERSION=$Cpanel::JSON::XS::VERSION";

$js = Mojo::JSON::decode_json( $boolfalse );
is( $cjson->encode( $js ), $boolfalse );
is( $js->{is_false}, 0 );

$js = $cjson->decode( $booltrue );
is( $cjson->encode( $js ), $booltrue ) or diag(ref $js->{is_true} );
$js = $cjson->decode( $boolfalse );
is( $cjson->encode( $js ), $boolfalse ) or diag(ref $js->{is_false} );

@Grinnz
Copy link
Author

Grinnz commented Oct 22, 2015

This issue is regarding stringification of objects, booleans are not a problem (anymore).

@rurban
Copy link
Owner

rurban commented Oct 22, 2015

So we'll add the option stringify_blessed, which does the stringify overload on encode, but not on decode for security reasons. What else?

@Grinnz
Copy link
Author

Grinnz commented Oct 22, 2015

Here's a basic test case:

use Test::More;
use Cpanel::JSON::XS ();
use Time::Piece;

my $json = Cpanel::JSON::XS->new->allow_blessed(1);
# $json->stringify_blessed(1); ??? possible option

my $time = localtime;
my $encoded = $json->encode({time => $time});
my $decoded = $json->decode($encoded);
is($decoded->{time}, "$time", 'Time::Piece object was stringified') or diag($encoded);

done_testing;

EDIT: changed to explicit stringification test

@rurban rurban added this to the Next Release milestone Oct 23, 2015
rurban pushed a commit that referenced this issue Oct 23, 2015
works ok with new Mojo, but fails with old
See GH #37
rurban pushed a commit that referenced this issue Oct 23, 2015
Mojo does not store their booleans as JSON::PP::Boolean
as everybody else does.
See GH #37
@rurban
Copy link
Owner

rurban commented Oct 23, 2015

See branch stringify_blessed-gh37 WIP

@rurban
Copy link
Owner

rurban commented Nov 15, 2015

I couldn't get the AMAGIC call yet working, so I'll postpone this feature.

@rurban
Copy link
Owner

rurban commented Nov 25, 2015

I'll keep the name convert_blessed for this feature. No need for an extra stringify_blessed.
See the branch convert_blessed-gh37

@zoffixznet
Copy link

👍

rurban pushed a commit that referenced this issue Nov 26, 2015
New feature to optionally stringify perl objects when
encoding to json.

See GH #37
rurban pushed a commit that referenced this issue Nov 26, 2015
fixes for older versions: 5.12, <5.18 and 5.6
>5.18 the stash is overloaded, <5.18 the sv.

we got an already unrefed sv in encode_stringify.
temp. RV it to call AMG_CALLunary

Fixes GH #37
@rurban
Copy link
Owner

rurban commented Nov 26, 2015

The feature works now >5.8.
The only question remaining is if to always embed the overloaded stringification into ""?
e.g.

$json->encode( { time => localtime } ) eq '{"time":"Thu Nov 26 11:15:16 2015"})

or

=> '{"time":Thu Nov 26 11:15:16 2015}'

Without embedded doubel-quotes it would allow to stringify into native js primitives, which might be a wanted feature.
like '{"time": { someother_object: 1} }', but then you should really use the TO_JSON method.

So I enforce "" now. It makes it easier to use normal default perl stringifications.

@kraih
Copy link

kraih commented Nov 26, 2015

Agreed, enforcing "" seems much better. Anyone who wants to generate something else than a JSON string can just use TO_JSON.

rurban pushed a commit that referenced this issue Nov 26, 2015
with 5.8 we need to rebless the rv to set AMG.
For safety recalc the overload magic methods also.

Fixes GH #37 for 5.8
@rurban
Copy link
Owner

rurban commented Nov 26, 2015

All tests pass, I'll merge it now to master

@rurban rurban closed this as completed Nov 26, 2015
@Grinnz
Copy link
Author

Grinnz commented Nov 29, 2015

EDIT: Moved to new issue #46

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

No branches or pull requests

6 participants