Skip to content

[Remote Config] Add Custom Signals - #1899

Open
a-maurice wants to merge 11 commits into
mainfrom
am-custom_signals
Open

[Remote Config] Add Custom Signals#1899
a-maurice wants to merge 11 commits into
mainfrom
am-custom_signals

Conversation

@a-maurice

Copy link
Copy Markdown
Contributor

Description

Provide details of the change, and generalize the change in the PR title above.

Add support for setting Custom Signals to Remote Config. This also adds some of the remote config unit tests back in.


Testing

Describe how you've tested these changes. Link any manually triggered Integration tests or CPP binary SDK Packaging Github Action workflows, if applicable.

Running integration tests and unit tests locally.


Type of Change

Place an x the applicable box:

  • Bug fix. Add the issue # below if applicable.
  • New feature. A non-breaking change which adds functionality.
  • Other, such as a build process or documentation change.

Notes

  • Bug fixes and feature changes require an update to the Release Notes section of release_build_files/readme.md.
  • Read the contribution guidelines CONTRIBUTING.md.
  • Changes to the public API require an internal API review. If you'd like to help us make Firebase APIs better, please propose your change in a feature request so that we can discuss it together.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces support for setting Custom Signals in Firebase Remote Config across Android, iOS, and Desktop platforms, along with corresponding tests. The review feedback highlights three critical issues regarding type handling: on Android and iOS, passing a null Variant fails to clear the custom signal and instead logs an error or gets omitted; on Desktop, converting numeric custom signals to string representations breaks backend targeting rules that expect numeric types, so the original Variant types should be preserved.

Comment thread remote_config/src/android/remote_config_android.cc Outdated
Comment thread remote_config/src/ios/remote_config_ios.mm
Comment thread remote_config/src/desktop/remote_config_desktop.cc
@a-maurice

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request adds support for setting Custom Signals in Remote Config across Android, iOS, and Desktop platforms, allowing developers to set custom key-value pairs for targeting. Feedback on the changes highlights JNI robustness issues on Android, such as missing null/OOM checks, improper exception handling, and inconsistent error handling for invalid Variant types. Additionally, it is recommended that the iOS implementation fail the future when encountering an invalid Variant type to maintain consistency with the Desktop platform's behavior.

Comment thread remote_config/src/android/remote_config_android.cc
Comment thread remote_config/src/ios/remote_config_ios.mm
@a-maurice

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request adds support for setting Custom Signals in Remote Config across Android, iOS, and Desktop platforms, including metadata serialization, REST request payload injection, and comprehensive testing. Feedback highlights two critical issues: first, RemoteConfigREST holds a copy of LayeredConfigs instead of a reference, which prevents updated custom signals from being propagated to the backend; second, the Desktop implementation merges custom signals while Android and iOS overwrite them, leading to platform inconsistencies and failing integration tests when clearing signals.

Comment thread remote_config/src/desktop/rest.h
Comment thread remote_config/src/desktop/remote_config_desktop.cc
@a-maurice

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request adds support for setting Custom Signals in Firebase Remote Config across Android, iOS, and Desktop platforms. It introduces the public APIs SetCustomSignals and SetCustomSignalsLastResult, implements platform-specific JNI, Objective-C, and REST bindings, handles metadata serialization, and adds comprehensive unit and integration tests. A critical issue was found in remote_config_request.cc where the logical condition for generate_status is inverted due to an incorrect comparison with nullptr, which would cause the function to assert and return early on success.

Comment thread remote_config/src/desktop/remote_config_request.cc
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.

1 participant