fix: serialize Array/Hash context variables as JSON instead of Ruby #to_s

[matthv]

Problem

ContextVariablesInjector.inject_context_in_value resolves a context variable and calls #to_s on it unconditionally. When the resolved value is a Ruby Array or Hash (e.g. {{currentUser.tags}}, the whole tags collection rather than a single tags.xxx key), #to_s produces Ruby's hash-rocket/inspect syntax (e.g. {"foo"=>"bar"}), which is not valid JSON. Any comparison against a json/jsonb column in a segment query then fails with an invalid input syntax error, the same way described in ForestAdmin/forest-rails#783.

inject_context_in_native_query is not affected and is left untouched by this PR: it binds the resolved value as a query parameter rather than serializing it into the query text, so it never hits this failure mode in the first place.

Fix

Mirrors the forest-rails fix, plus two follow-ups raised in review:

• When the filter value is exactly one {{variable}} reference (no surrounding text), inject_context_in_value now returns the resolved object as-is instead of a serialized string, so the datasource type-casts the real Array/Hash correctly instead of re-serializing an already-encoded string (which a naive to_json swap alone does not fix on the ActiveRecord path — see review discussion). A reference embedded in a larger string still gets JSON.generate'd, since it has to become part of a string either way.
• inject_context_in_value_custom's substitution now resolves every placeholder from the original string upfront and applies them in a single pass, instead of a mutating loop that re-scanned its own output — which could misinterpret {{...}}-looking text inside a resolved value as a new reference, or hang if that text matched an already-resolved key.

Test

Added a { key: 'tags', expected_value: user['tags'].to_json } case to the existing currentUser-variable table test, plus dedicated coverage for: the full-reference object-preserving path (Hash and Array), JSON serialization when embedded in a larger string, backslash preservation during substitution, and the corruption/hang scenario described above.

Note

Serialize Array/Hash context variables as JSON instead of Ruby to_s

In ContextVariablesInjector.inject_context_in_value, resolved context variable values that are Arrays or Hashes are now serialized with to_json instead of to_s. Scalar values continue to use to_s. Behavioral Change: any context variable that previously injected a Ruby object literal (e.g. {"key"=>"value"}) will now produce a valid JSON string (e.g. {"key":"value"})

Changes since #339 opened

• Modified ForestAdminAgent::Utils::ContextVariablesInjector.inject_context_in_value to return resolved values with their original types when the input is exactly a single variable reference matching {{variable}}, and to serialize Array and Hash values as JSON using JSON.generate when interpolated within larger strings [c66f829]
• Fixed ForestAdminAgent::Utils::ContextVariablesInjector.inject_context_in_value_custom to prevent interpretation of backslashes and dollar sign sequences in replacement strings during substitution [c66f829]
• Updated test expectations in packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/context_variables_injector_spec.rb and packages/forest_admin_agent/spec/lib/forest_admin_agent/services/permissions_spec.rb to verify that full variable references return native types and that Arrays/Hashes embedded in strings are JSON-serialized [c66f829]
• Rewrote placeholder substitution logic in ForestAdminAgent::Utils::ContextVariablesInjector.inject_context_in_value_custom to prevent infinite loops and secondary resolution [6525b5a]
• Added test cases to verify placeholder resolution does not reinterpret resolved values or hang on self-references [6525b5a]

📊 Macroscope summarized c66f829. 1 file reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted

🗂️ Filtered Issues

No issues evaluated.

[Scra3]

Verified the mechanism end to end, and the fix does not reach its target on the ActiveRecord path — details in the blocking comment. Requesting changes mainly to get the failing query, since this touches permission-scope semantics.

Three findings that sit outside the diff, so I could not attach them inline:

gsub! corrupts backslashes (context_variables_injector.rb:48). The two-argument form interprets escape sequences in the replacement string, and to_json emits \\ for a literal backslash. Reproduced:

to_json  : [{"key":"planet","value":"C:\\path"}]
injected : [{"key":"planet","value":"C:\path"}]
parsed   : {"value"=>"C:path"}      # backslash silently gone

Pre-existing (to_s had the same hazard), but this PR is what makes valid JSON the goal. The block form gsub!(regex) { yield(context_variable_key) } disables replacement-string interpolation.

Native queries are not fixed. The description lists them as affected, but inject_context_in_native_query (same file, line 32) passes get_value raw into the bindings map and is untouched. Bindings go to the driver so the invalid-JSON failure cannot arise there — the problem statement looks wider than the change.

Cross-SDK parity is now 2 of 5. forest-rails#783 is genuinely merged with the same diff, so this is half a coordinated change rather than a unilateral divergence — my mistake on that. Worth noting though that #783 was opened 09:08Z and self-merged 09:23Z with zero reviews, ~50 min before this one, so it provides no independent validation. agent-nodejs (String()), agent-python (str()) and agent-php (raw value) all still emit non-JSON.

One limit on my side: I could not run against a live PostgreSQL (pg is not in the bundle). The ActiveRecord conclusion comes from reading Type::Json#serialize and executing it directly, which is solid for the value handed to the driver, but I did not observe the final SQL.

[Scra3]

issue (blocking): inject_context_in_value_custom always returns a String, and ActiveRecord::Type::Json#serialize re-encodes any String handed to a jsonb column — so the comparison becomes a jsonb string scalar against a jsonb array and never matches, in both the old and the new form.

to_json then AR : "\"[{\\\"key\\\":\\\"planet\\\"}]\""   # jsonb string
to_s    then AR : "\"[{\\\"key\\\"=>\\\"planet\\\"}]\""   # jsonb string
the Array itself: [{"key":"planet"}]                    # jsonb array — the only one that matches

Two consequences: the claimed PG::InvalidTextRepresentation cannot reproduce through the AR datasource (AR quotes the string into valid JSON before PG sees it), and I found no issue or reproduction in this repo for it. Could you share the failing query? If an array-valued scope really has to work, string substitution is the wrong shape — the value would need to stay an object when it is exactly one {{variable}} reference, which changes the return-type contract and needs product sign-off.

[matthv]

Confirmed empirically — thanks for catching this. Type::Json#serialize re-encodes any String handed to a jsonb attribute (even one already produced by to_json), so neither to_s nor to_json could ever have matched here.

Fixed in c66f829: when the filter value is exactly one {{variable}} reference (no surrounding text), inject_context_in_value now returns the resolved object as-is instead of a string, so .where(field: value) type-casts the real Array/Hash correctly (single encode instead of double). Side benefit: this also fixes ConditionTreeLeaf#match's in-memory equality check, which was comparing a real object to a string and could never match either.

On the failing query — I tracked down the original report: it's a forest-rails production incident (a Qonto segment on {{currentUser.tags}} against a jsonb column, actively broken at the time). I don't have a confirmed reproduction on this repo's ActiveRecord datasource specifically — this PR is a parity fix, not driven by an agent-ruby incident. The object-preserving change above should make it correct here as well if/when the same pattern is hit.

[Scra3]

issue (non-blocking): to_json is called without require 'json' in this file, and nothing in the library loads json — Zeitwerk is lazy, so include ForestAdminAgent::Builder never loads agent_factory.rb and its require never runs; the spec only passes because spec_helper.rb:2 requires simplecov, which pulls json in. Non-Rails hosts (forest_admin_rpc_agent, bare Rack) hit NoMethodError.

Also worth pinning the encoder: ActiveSupport monkey-patches to_json with escape_html_entities_in_json = true, so < becomes \u003c and the same scope serializes two ways depending on whether AS is loaded. JSON.generate(resolved_value) fixes both the missing require and the non-determinism.

[matthv]

Fixed — added require 'json' and switched to JSON.generate instead of #to_json, so it doesn't depend on ActiveSupport's to_json monkey-patch (and its html-entity escaping) being loaded. (c66f829)

[Scra3]

issue (non-blocking): this row covers the Array branch only — the fixture is 'tags' => [{ 'key' => 'planet', ... }] — so || resolved_value.is_a?(Hash) can be deleted with the suite staying green, and no spec anywhere resolves a context variable to a Hash. A front-supplied variable (dropdown, multi-select) is the likely Hash producer, so that half deserves a row too.

[matthv]

This one's actually already covered, just not the way it looks — in this repo the tags fixture is a bare Hash ({ 'planet' => 'Death Star' }), not an Array like forest-rails' fixture. So this row exercises the Hash branch, not Array: removing || resolved_value.is_a?(Hash) would break it (falls back to .to_s → Ruby hash-rocket syntax, test would fail). I added a dedicated Array-branch test too in c66f829 (via a request_context_variables entry), since that genuinely had zero coverage before.

[Scra3]

You are right and my comment was wrong — apologies. The fixture is 'tags' => { 'planet' => 'Death Star' } on both main and this branch, so this row does exercise the Hash branch, and Array was the uncovered half.

The cause of my mistake is worth stating so it doesn't happen again: my working copy was checked out on an unrelated local branch whose fixture had been changed to an Array of Hashes, and I read that file instead of the branch's. forest-rails' fixture is an Array of Hashes (tags[0]['value']), which made the wrong reading look consistent. Two review agents I cross-checked with read the same local file and confirmed the same error, so the agreement was worthless — same bad source.

Your dedicated Array-branch test in c66f829 closes the gap that actually existed.

[qltysh]

All good ✅

[matthv]

Thanks for the thorough review — pushed c66f829 addressing the blocking issue plus the two inline non-blocking ones (replied on each thread).

Also addressed the three points raised outside the diff:

• gsub! backslash corruption: fixed, using the block form now (gsub!(regex) { replacement }) so a backslash in an injected value survives.
• Native queries: confirmed unaffected/untouched — agreed this PR's scope is narrower than the description implied, no change needed there.
• Cross-SDK parity / #783 self-merge: fair callout. I tracked down the original report — it's a real, actively-broken production segment for Qonto on forest-rails, not this repo. I don't have a confirmed failing query for agent-ruby's ActiveRecord datasource specifically; this PR is intentionally a parity fix rather than incident-driven. The updated approach (return the raw object when the filter value is exactly one {{variable}} reference) should make it correct here regardless — it also fixes the same class of bug for in-memory condition matching (ConditionTreeLeaf#match), not just the AR .where() path.

Full suite green (724 examples), rubocop clean.

[Scra3]

Approving — all four points are addressed, and the fixes are the right shape.

• The blocking one: FULL_REFERENCE_REGEX returning the resolved object when the value is exactly one reference is what makes the jsonb comparison work at all, and your observation that it also repairs ConditionTreeLeaf#match's in-memory equality is a real second win. Note it changes the return-type contract of inject_context_in_value — legitimate here, since a ConditionTreeLeaf with an Array value is exactly what in wants, and both callers (permissions.rb:147, charts.rb:70) pass it straight into a leaf.
• require 'json' plus JSON.generate — pins the encoder as well as the load, so the ActiveSupport html-entity escaping can no longer change the serialised form between hosts.
• The single-pass resolution kills both the corruption and the hang, and the Timeout(2) wrapper is the right call so a reintroduction fails loudly instead of hanging CI.
• I was wrong on the fixture shape; corrected in that thread.

Thanks for being straight about the reproduction: a forest-rails production incident with no confirmed repro on this datasource is a fair basis for a parity fix, and it is more useful stated than implied.

One thing left untouched, pre-existing and outside this PR's scope — inject_context_in_native_query still has the same class of bug you just fixed in inject_context_in_value_custom:

next if encountered_variables.value?(context_variable_key)

It compares the variable name against the hash's values, so the dedup can never fire; and if it ever did, next inside a while that hasn't advanced the string would spin forever. Same failure mode, other path. Worth a follow-up ticket rather than widening this PR.

Also still true, for the record: the PR description lists native queries as affected, and they are not covered by the change — the bindings path serialises via the driver's type map, never as JSON. Might be worth trimming that sentence so the next reader doesn't assume it was fixed.

[matthv]

Also updated the description (Problem section) — it was still claiming native queries were affected, but inject_context_in_native_query binds values as query parameters and was never actually exposed to this failure mode. Clarified that and refreshed the Fix section to reflect what actually landed across the three commits.