search

ruby-grape / grape

An opinionated framework for creating REST-like APIs in Ruby.
http://www.ruby-grape.org
MIT License
9.89k stars 1.22k forks source link

Formatting with multi_json ? #2486

Open ericproulx opened 3 months ago

ericproulx commented 3 months ago

Am I wrong to say that Grape::Formatter::JSON will never call Grape::Json::Dump since every object responds to to_json ? On the other hand, Grape::ErrorFormatter::JSON is not using to_json.

I'm just concerned about the multi_json usage which doesn't seem to be consistent. Should we keep it ?

dblock commented 3 months ago

We've relied on multi_json to swap JSON serializers between the default one and oj, so removing it would be backwards incompatible and definitely create problems for users. It's possible that the paths where it was meaningful were all with large amounts of JSON, so nobody noticed that the error formatter wasn't using it? Which means it's a bug.

ericproulx commented 3 months ago

To code must call Grape::Json::Dump or to_json when formatting JSON ? The docs says

Built-in formatters are the following.
    :json: use object's to_json when available, otherwise call MultiJson.dump

This means that even if you have multi_json, the latter is not used when formatting JSON. Grape::ErrorFormatter::Json and Grape::Parser::Json are ok since its using Grape::Json.dump and Grape::Json.load respectively.

Its true that Multijson facilitates the usage of Oj but anybody could simply call Oj.mimic_JSON() and Oj would override all to_json.

dblock commented 3 months ago

Its true that Multijson facilitates the usage of Oj but anybody could simply call Oj.mimic_JSON() and Oj would override all to_json.

It could be a relic of the past. A change like this is still breaking, so it's got to be worth it.

ericproulx commented 3 months ago

GitLab has their own JSON Formatter which is using dump instead of to_json. I think we should remove the to_json part and just keep the Grape::Json.dump.