Skip to content

Allow comments when reading the JSON cache files (fixes load failure on json 3.0) - #191

Open
bonjourjules wants to merge 1 commit into
Shopify:mainfrom
bonjourjules:fix-json3-comments
Open

Allow comments when reading the JSON cache files (fixes load failure on json 3.0)#191
bonjourjules wants to merge 1 commit into
Shopify:mainfrom
bonjourjules:fix-json3-comments

Conversation

@bonjourjules

Copy link
Copy Markdown

Problem

measured cannot be loaded at all when json 3.0 is in the bundle:

$ ruby -e 'require "measured"'
JSON::ParserError: unexpected token '//' at line 1 column 1

Measured::Cache::JsonWriter prefixes every cache file with a comment line:

# lib/measured/cache/json_writer.rb:5
f.write("// Do not modify this file directly. Regenerate it with 'rake cache:write'.\n")

That line is not valid JSON. Measured::Cache::Json#read worked because the json gem accepted JavaScript comments by default. json 3.0 turned that off:

  • The allow_comments parsing option now defaults to false.

Since lib/measured.rb requires units/length, units/weight and units/volume, and each declares cache Measured::Cache::Json, "<unit>.json", the table is read while the class body is evaluated — so this raises at require time, not on first conversion. Under Bundler it surfaces as Bundler::GemRequireError and the host application never boots.

This currently affects any app that pulls in measured on json 3.0. Because the gemspec declares no upper bound on json, Bundler resolves to 3.0 on its own.

The test suite is affected too — on main, with json 3.0, it does not even load:

$ bundle exec rake test
Gem Load Error is: unexpected token '//' at line 1 column 1

Fix

Opt back into comments when reading the cache.

allow_comments was introduced in json 2.20 and unknown options are silently ignored by earlier versions, which accept comments anyway. I verified the call on json 2.19.0, 2.21.2 and 3.0.0 — all three return the parsed table, so this stays compatible across every json version the gem supports (gemspec: Ruby >= 3.0).

I kept the writer as is, so cache files already on disk keep working. Dropping the comment header instead would have been the other option, but it breaks reading any previously generated cache.

Verification

With the patch, on json 3.0.0:

517 tests, 845 assertions, 0 failures, 0 errors, 0 skips

The existing #read test passes a payload without the comment header, which is why the suite never caught this. I added a test that uses the exact header JsonWriter writes, so the regression is covered — it reproduces the failure without the one-line change.

`Measured::Cache::JsonWriter` prefixes every cache file it writes with a
`// Do not modify this file directly.` line, which is not valid JSON.
`Measured::Cache::Json#read` relied on the json gem accepting JavaScript
comments by default. json 3.0 turned that off: `allow_comments` now
defaults to `false`, so requiring `measured` raises

    JSON::ParserError: unexpected token '//' at line 1 column 1

while loading `cache/length.json`, and the gem cannot be loaded at all.
Under Bundler this surfaces as a `Bundler::GemRequireError` at boot.

Opt back into comments when reading the cache. The option was introduced
in json 2.20 and is silently ignored by older versions, which accept
comments anyway, so this stays compatible with every json version
supported by the gem.

The existing `#read` test passes a payload without the comment header,
which is why the suite did not catch this; add a test that uses the exact
header `JsonWriter` produces.

@kmcphillips kmcphillips left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good fix that is backwards compatible.

I think it makes sense to not dump that comment at the top if it is technically invalid. It's important to flag that somehow, but maybe there's another way. But maybe the comment doesn't matter because this library should be the only thing that ever reads that file. 🤷‍♂️

decode(JSON.load(File.read(@path), nil, freeze: true))
# The cache files written by Measured::Cache::JsonWriter start with a `//` comment line, which is not
# valid JSON. json < 3.0 accepted comments by default, json >= 3.0 requires opting in. The option is
# ignored by json < 2.20, where comments are accepted anyway.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A three line comment is not required to add a single config option. LLMs dump this out, but it makes the codebase less legible, not more.

@bonjourjules

Copy link
Copy Markdown
Author

I have signed the CLA!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants