Skip to content

fix: beforeSend overriding default values even if they were not set #376

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

buenaflor
Copy link
Contributor

@buenaflor buenaflor commented Apr 23, 2025

📜 Description

We need to keep the KMP event model in sync with the native one. Users mainly edit events in KMP hooks like beforeSend, so we must track whether each field was actually modified:
If a field was changed, propagate the new value to the native event.• If it was untouched, leave the native default in place.
Previously we overwrote native defaults with null values by default e.g., Cocoa’s auto‑detected release was not working since we set a null release

💡 Motivation and Context

Fixes #337

💚 How did you test it?

TBD unit tests

📝 Checklist

You have to check all boxes before merging:

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.

🔮 Next steps

Copy link

codecov bot commented Apr 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.32%. Comparing base (ff8969f) to head (de816fd).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #376      +/-   ##
==========================================
- Coverage   79.92%   79.32%   -0.61%     
==========================================
  Files          44       11      -33     
  Lines         812      324     -488     
  Branches      108       49      -59     
==========================================
- Hits          649      257     -392     
+ Misses        107       39      -68     
+ Partials       56       28      -28     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

@buenaflor buenaflor marked this pull request as ready for review April 23, 2025 13:55
@buenaflor buenaflor requested a review from romtsn as a code owner April 23, 2025 13:55
@buenaflor buenaflor marked this pull request as draft April 23, 2025 13:55
@buenaflor buenaflor marked this pull request as ready for review April 24, 2025 22:10
@buenaflor
Copy link
Contributor Author

buenaflor commented May 2, 2025

I'll update this PR, let's fix it only for the release field now. I'm not even sure if the other fields need to be fixed like that as well


// init with a SentryLevel as a workaround for:
// Unable to call non-designated initializer as super constructor
private class FakeSentryEvent : CocoaSentryEvent(Internal.Sentry.kSentryLevelFatal) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

can only test cocoa like this since the SentryEvent in java is final and we don't use a mocking framework yet

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.

On iOS default release version is cleared when no release is set on the Kotlin Multiplatform side.
2 participants