Fix: Prevent SetDefaultEventParameters from clearing params with all-… #1747
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…invalid input Addresses code review feedback regarding the behavior of SetDefaultEventParameters when all provided parameters are of invalid types. Previously, if all parameters in the C++ map were invalid, an empty NSDictionary/Bundle would be passed to the native iOS/Android setDefaultEventParameters method. According to documentation, passing nil/null (for iOS/Android respectively) clears all default parameters. While passing an empty dictionary/bundle is a valid operation, it could lead to unintentionally clearing parameters if that was the state before, or it might be a no-op if parameters were already set. This change modifies the iOS and Android implementations to explicitly check if the processed native parameter collection (NSDictionary for iOS, Bundle for Android) is empty after filtering for valid types. If it is empty, the call to the native SDK's setDefaultEventParameters method is skipped. This ensures that providing a C++ map containing exclusively invalid parameters (or an empty map) to SetDefaultEventParameters will be a true no-op from the C++ SDK's perspective, preventing any accidental modification or clearing of existing default parameters on the native side.
jonsimantov
added
the
tests-requested: quick
github-actions
bot
added
tests: in-progress
✅ Integration test succeeded!Requested by @jonsimantov on commit 9681c18 |
firebase-workflow-trigger
bot
removed
the
tests: in-progress
…invalid input Addresses code review feedback regarding the behavior of SetDefaultEventParameters when all provided parameters are of invalid types. Previously, if all parameters in the C++ map were invalid, an empty NSDictionary/Bundle would be passed to the native iOS/Android setDefaultEventParameters method. According to documentation, passing nil/null (for iOS/Android respectively) clears all default parameters. While passing an empty dictionary/bundle is a valid operation, it could lead to unintentionally clearing parameters if that was the state before, or it might be a no-op if parameters were already set. This change modifies the iOS and Android implementations to explicitly check if the processed native parameter collection (NSDictionary for iOS, Bundle for Android) is effectively empty after filtering for valid types. If it is, the call to the native SDK's setDefaultEventParameters method is skipped. For iOS, this is done by checking `[ns_default_parameters count] == 0`. For Android, this is done by tracking if any parameter was successfully added to the Bundle using a boolean flag, as a direct `bundle.isEmpty()` check before the native call could miss JNI exceptions during bundle population. This ensures that providing a C++ map containing exclusively invalid parameters (or an empty map) to SetDefaultEventParameters will be a true no-op from the C++ SDK's perspective, preventing any accidental modification or clearing of existing default parameters on the native side.
jonsimantov
added
the
tests-requested: quick
github-actions
bot
added
tests: in-progress
firebase-workflow-trigger
bot
removed
the
tests: in-progress
jonsimantov
merged commit 9681c18
into
feat-default-event-params
48 of 54 checks passed
github-actions
bot
added
tests: in-progress
firebase-workflow-trigger
bot
removed
the
tests: in-progress
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…invalid input
Addresses code review feedback regarding the behavior of SetDefaultEventParameters when all provided parameters are of invalid types.
Previously, if all parameters in the C++ map were invalid, an empty NSDictionary/Bundle would be passed to the native iOS/Android setDefaultEventParameters method. According to documentation, passing nil/null (for iOS/Android respectively) clears all default parameters. While passing an empty dictionary/bundle is a valid operation, it could lead to unintentionally clearing parameters if that was the state before, or it might be a no-op if parameters were already set.
This change modifies the iOS and Android implementations to explicitly check if the processed native parameter collection (NSDictionary for iOS, Bundle for Android) is empty after filtering for valid types. If it is empty, the call to the native SDK's setDefaultEventParameters method is skipped.
This ensures that providing a C++ map containing exclusively invalid parameters (or an empty map) to SetDefaultEventParameters will be a true no-op from the C++ SDK's perspective, preventing any accidental modification or clearing of existing default parameters on the native side.
Description
Testing
Type of Change
Place an
xthe applicable box:Notes
Release Notessection ofrelease_build_files/readme.md.