Skip to content

fix: avoid singleton mutation in NewFromJsonBytesContext#126

Merged
afrittoli merged 2 commits into
cdevents:mainfrom
davidlemme-usmobile:fix/singleton-mutation-data-bleed
Apr 9, 2026
Merged

fix: avoid singleton mutation in NewFromJsonBytesContext#126
afrittoli merged 2 commits into
cdevents:mainfrom
davidlemme-usmobile:fix/singleton-mutation-data-bleed

Conversation

@davidlemme-usmobile
Copy link
Copy Markdown
Contributor

Fixes #125

NewFromJsonBytesContext unmarshals JSON directly into the singleton pointers stored in CDEventsByUnversionedTypes. Since json.Unmarshal doesn't zero omitempty fields absent from the input, values from a previous call leak into subsequent calls for the same event type. This also causes a data race when called concurrently.

The fix uses reflect.New to create a fresh instance of the concrete type before unmarshalling, so the map entries are only used for type lookup and never mutated.

@davidlemme-usmobile davidlemme-usmobile force-pushed the fix/singleton-mutation-data-bleed branch from a0ba7e6 to e7b4b42 Compare April 1, 2026 20:24
Copy link
Copy Markdown
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

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

Thanks for catching and fixing this. The change looks good.

@afrittoli
Copy link
Copy Markdown
Member

A pin was missing in CI: #127

@afrittoli
Copy link
Copy Markdown
Member

@davidlemme-usmobile the PR looks good, but you'll need to rebase it so CI can succeed and then be merged.

Signed-off-by: David Lemme <david.lemme@usmobile.com>
@davidlemme-usmobile davidlemme-usmobile force-pushed the fix/singleton-mutation-data-bleed branch from e7b4b42 to 1f1ad29 Compare April 8, 2026 16:44
@davidlemme-usmobile
Copy link
Copy Markdown
Contributor Author

My pleasure. Rebased :)

@afrittoli
Copy link
Copy Markdown
Member

It looks like your fix exposed a unit test that used to pass only because of the bug - the implicit_json_custom_data.json test fixture is missing some of the fields that are set in the expected result.
Would you mind fixing that as part of your PR as well?

@davidlemme-usmobile
Copy link
Copy Markdown
Contributor Author

It looks like your fix exposed a unit test that used to pass only because of the bug - the implicit_json_custom_data.json test fixture is missing some of the fields that are set in the expected result. Would you mind fixing that as part of your PR as well?

Will do working on it shortly

Signed-off-by: David Lemme <david.lemme@usmobile.com>
@davidlemme-usmobile davidlemme-usmobile force-pushed the fix/singleton-mutation-data-bleed branch from 58ae009 to 050efc4 Compare April 8, 2026 20:16
@davidlemme-usmobile
Copy link
Copy Markdown
Contributor Author

davidlemme-usmobile commented Apr 8, 2026

added the fix to the tests

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 9, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.14%. Comparing base (9a15314) to head (050efc4).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #126      +/-   ##
==========================================
+ Coverage   85.12%   85.14%   +0.01%     
==========================================
  Files         149      149              
  Lines       13097    13098       +1     
==========================================
+ Hits        11149    11152       +3     
+ Misses       1631     1629       -2     
  Partials      317      317              
Files with missing lines Coverage Δ
pkg/api/bindings.go 83.33% <100.00%> (+0.15%) ⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@afrittoli
Copy link
Copy Markdown
Member

Thank you!

@afrittoli afrittoli added this pull request to the merge queue Apr 9, 2026
Merged via the queue into cdevents:main with commit 00173ce Apr 9, 2026
8 checks passed
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.

bug: NewFromJsonBytesContext mutates singleton map entries, causing data bleed and data races

2 participants