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

Formatting with multi_json ? #2486

Open
ericproulx opened this issue Jul 27, 2024 · 4 comments
Open

Formatting with multi_json ? #2486

ericproulx opened this issue Jul 27, 2024 · 4 comments
Labels

Comments

@ericproulx
Copy link
Contributor

ericproulx commented Jul 27, 2024

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
Copy link
Member

dblock commented Jul 27, 2024

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
Copy link
Contributor Author

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
Copy link
Member

dblock commented Jul 29, 2024

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
Copy link
Contributor Author

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.

@dblock dblock added the question label Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants