Conversation
Oj's :indent option is an Integer count of spaces while the json gem's
indent is the String to indent with. Oj.dump in compat and custom mode
forwards its options hash to every to_json it calls, so an object
whose to_json comes from the json gem, such as a Time, handed the
Integer to JSON::Generator::State, which insists on a String:
Oj.dump(Time.at(0).utc, mode: :compat, use_to_json: true, indent: 2)
# TypeError: wrong argument type Integer (expected String)
Give to_json a copy of the hash with :indent as that many spaces. A
String or nil indent, and a hash with no indent at all, are passed
through as the same object, and the caller's hash is never modified.
A negative Integer becomes an empty String, matching how Oj itself
treats it.
Compat mode passes as_ok as true for every nested value, so
use_to_json only gates the top level object and a Time inside an
Array reached to_json without the option set. The conversion is
therefore decided by the mode: compat and custom mode, the ones that
call to_json, get the converted hash whether or not use_to_json is
set. The other modes never call to_json and are unchanged.
The new test/test_to_json_json_gem.rb loads the json gem before Oj so
Time#to_json is the json gem's, which is the case from the report. It
sets mode and use_to_json through Oj.default_options so the hash that
reaches to_json holds only the indent under test. It lives in its own
file because loading the json gem changes what every other compat test
dumps.
Closes ohler55#1105
ohler55
reviewed
Sep 8, 2026
ohler55
reviewed
Sep 8, 2026
Owner
|
Looks like some tests are failing. |
stringify_indent_option() allocated an uninitialized String and then filled it with spaces. oj_parse_options() has already raised on an Integer :indent above MAX_INDENT by the time the helper runs, so a static MAX_INDENT-character run of spaces covers every count that can reach it. The String handed to to_json is now a prefix of that run, which drops the memset. The count is still clamped to the run in case the two ever drift apart, and a negative count is still an empty String. Tests cover MAX_INDENT itself and a negative count.
Contributor
Author
|
I addressed both of your inline comments above. I’m pretty sure the test failures are unrelated to my patch and were instead triggered by the |
Owner
|
As with the other MR, better to have tests passing or skipped if it makes sense to skip. Maybe merge the other MR first then this will pass as well. |
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Oj's
:indentoption is an Integer count of spaces, while thejsongem'sindentis the String to indent with.Oj.dumpin:compatand:custommode forwards its options hash to everyto_jsonit calls, so an object whoseto_jsoncomes from the json gem (Time,Date,Symbol, and so on) handed the Integer toJSON::Generator::State, which insists on a String.This patch gives
to_jsona copy of the hash with:indentas that many spaces. A String or nil indent, or a hash with no indent at all, is passed through as the same object, and the caller's hash is never modified. A negative Integer becomes an empty String, which matches how Oj itself treats it. Only:compatand:custommodes (i.e. the ones that callto_json) take the copy.One thing I ran into while working on #1106:
:compatmode passesas_okastruefor every nested value, souse_to_jsononly gates the top level object and aTimeinside an Array reachedto_jsonwithout the option set. The conversion is therefore decided by the mode rather than by the flag.This patch is independent of #1106 and branches from
develop, but the two touch the same spot indump(), add the sameArgKeepertest class, create a test file of the same name, and both add a3.17.7 - unreleasedchangelog heading, so whichever lands second will need a small conflict resolution. The two helpers are separate functions, so combining them is a matter of calling both on the forwarded hash. I'm happy to rebase this onto #1106 instead if you'd prefer to review them as a stack.Similar to #1106, I’d expect the
rails_8.1jobs to fail on ActiveSupport 8.1.3.1 passing a positional options hash tojson3'sJSON.parse. Also, the Valgrind job will hit the #1104 error from the realworldcanada.jsondata once the new test file loads the json gem, because this branch doesn't include the fix in #1106.