Repository navigation
[FSSDK-13349] Fix ODP event duplicate symbol/string key JSON error - #408
Conversation
…te-key JSON errors String-keyed custom_data (as received from parsed JSON callers) was being merged into the symbol-keyed default event data without normalization, producing a hash with duplicate-looking keys (e.g. both :data_source_type and "data_source_type"). Serializing that hash raises in newer json gem versions, silently dropping the ODP event instead of sending it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Farhan Anjum (FarhanAnjum-opti)
left a comment
There was a problem hiding this comment.
LGTM. The fix matches FSSDK-13349 and the root cause is solid. Full suite and rubocop pass locally.
Nit (non-blocking): transform_keys(&:to_sym) raises NoMethodError for non-string keys (e.g. {1 => 'x'} or a nil key). odp_data_types_valid? only checks values, not keys, so those inputs reach OdpEvent.new. On master they serialized fine, so this would now throw from send_odp_event into caller code. Unlikely in practice, but an easy hardening:
data.update(custom_data.transform_keys { |key| key.is_a?(String) ? key.to_sym : key })transform_keys(&:to_sym) raised NoMethodError for non-string keys (e.g. Integer, nil), which odp_data_types_valid? doesn't filter out since it only checks hash values. Only string keys can collide with the symbol-keyed defaults, so only those need normalizing; other key types are left as-is, matching prior behavior. Addresses review feedback from @FarhanAnjum-opti on PR #408. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Good catch, thanks Farhan Anjum (@FarhanAnjum-opti) — verified the |
Matjaz Pirnovar (Mat001)
left a comment
There was a problem hiding this comment.
This PR correctly fixes a real bug where caller-supplied string keys (from parsed JSON) failed to override the SDK's symbol-keyed defaults in OdpEvent#add_common_event_data, producing a duplicate-key hash that raised JSON::GeneratorError and silently dropped ODP events. The one-line fix (transform_keys symbolizing only string keys) restores the intended override semantics, is Ruby 3.0+ safe, and correctly leaves non-string keys alone since value-only validation lets Integer/nil keys through. Tests are meaningful regression guards (the dup-key spec fails pre-fix, passes after), scope is minimal and low-risk
Summary
OdpEvent#add_common_event_datamerged caller-suppliedcustom_data(string keys, as received from parsed JSON) directly into the SDK's default event data hash (symbol keys for the same field names:data_source_type,data_source,data_source_version). Ruby does not treat:data_source_typeand"data_source_type"as the same hash key, so the merge added a second, duplicate-looking key instead of overriding the default.OdpEventApiManager#send_odp_events→events.to_json) raisesJSON::GeneratorError("detected duplicate key ...") on newer versions of thejsongem, which was caught generically and logged as an ODP send failure — the event was silently dropped and never sent.custom_data's keys to symbols (custom_data.transform_keys(&:to_sym)) before merging, so a caller-supplied field correctly overrides the matching default instead of duplicating it.The "why": this was found while investigating a long-standing, seemingly-unrelated flaky CI check (
fullstack-sdk-compatibility-suite (ruby)) on an unrelated PR. The failure was traced through several incorrect theories (test-harness timing, Docker network/DNS race) before being confirmed directly from a CI container log, which showed the exactJSON::GeneratorErrorthis change fixes. Every affected FSC test scenario's payload happens to include the same three field names the SDK sets by default, which is why it reproduced consistently.Test plan
spec/odp/odp_event_spec.rb: verifies caller-supplied fields correctly override the matching defaults, that the resulting hash has no duplicate symbol/string keys, and that serialization doesn't raise. Verified the new spec fails against the pre-fix code and passes with the fix.Issues
FSSDK-13349
🤖 Generated with Claude Code