search

tobyink / p5-json-schema

1 stars 4 forks source link

convert_blessed_universally flag in JSON::Schema::Helper is leaky #2

Open matthof opened 10 years ago

matthof commented 10 years ago

Setting the -convert_blessed_universally mode for the JSON module affects JSON usage outside of JSON::Schema. So, if other JSON calls to encode() or to_json() were previously expected to die when fed blessed hashrefs, they will suddenly stop dying when JSON::Schema is imported.

It looks like this can safely be dropped, since JSON::Schema::Helper is supplying "convert_blessed=>1" to each to_json() call, anyway... does that sound correct? Or is there some other motivation behind using -convert_blessed_universally?

tobyink commented 10 years ago

Do you have a demonstration of that? This issue seems to contradict how -convert_blessed_universally is documented to behave, and contradicts my reading of the JSON.pm source code.

-convert_blessed_universally isn't at all equivalent to supplying convert_blessed=>1 to each to_json() call.

tobyink commented 10 years ago

PS: I agree that -convert_blessed_universally is global in effect, but it seems to me the cases where its globalness could cause problems are pretty minimal.

matthof commented 10 years ago

Yeah, the impact is minimal - it is basically breaking some unit tests my team has. We have use cases where we are explicitly serializing blessed objects to JSON using wrappers on JSON encode, so we are hoping to catch places where developers might inadvertently circumvent the wrapper methods.

I misread the documentation for -convert_blessed_universally, you are right that it is not equivalent to applying convert_blessed=>1 everywhere; so, the only use case I have is that we actually expect to catch cases where TO_JSON is not supplied. For example, we're expecting this to die:

myEncodeJsonWrapper( bless({}, 'foo') );

to die, but not this:

myEncodeJsonWrapper( ObjWithToJSON->new() );

where ObjWithToJSON has sub TO_JSON {...} defined, and myEncodeJsonWrapper is calling JSON->new->allow_blessed->convert_blessed->encode(...)