Skip to content
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

feat(unreal): Add check to ensure user info is not empty #4225

Merged
merged 4 commits into from
Nov 6, 2024

Conversation

tobias-wilfert
Copy link
Member

Seeing an issue with a customer where the login_id is set to an empty string. This tries to fix that by checking that the value we set the login_id to from the header is not an empty string and by overwriting the login_id value from the runtime_properties not only if the login_id is none but also if it is an empty string.

@tobias-wilfert tobias-wilfert self-assigned this Nov 6, 2024
@tobias-wilfert tobias-wilfert requested a review from a team as a code owner November 6, 2024 14:52
relay-server/src/utils/unreal.rs Outdated Show resolved Hide resolved
tobias-wilfert and others added 2 commits November 6, 2024 16:05
Co-authored-by: David Herberth <david.herberth@sentry.io>
@tobias-wilfert tobias-wilfert merged commit dbd2307 into master Nov 6, 2024
23 checks passed
@tobias-wilfert tobias-wilfert deleted the tobias-wilfert/feat/check-user-info branch November 6, 2024 15:42
@@ -201,7 +201,8 @@ fn merge_unreal_context(event: &mut Event, context: Unreal4Context) {

if let Some(login_id) = &runtime_props.login_id {
let id = event.user.get_or_insert_with(User::default).id.value_mut();
if id.is_none() {

if id.as_ref().map_or(true, |s| s.is_empty()) {
Copy link
Contributor

@loewenheim loewenheim Nov 8, 2024

Choose a reason for hiding this comment

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

I learned of it too late, but this seems a good usecase for Option::is_none_or.

Suggested change
if id.as_ref().map_or(true, |s| s.is_empty()) {
if id.as_ref().is_none_or(|s| s.is_empty()) {

Copy link
Member

Choose a reason for hiding this comment

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

Huh, I remember searching for this a while ago and being annoyed that it doesn't exist, but they added it with 1.82 now!

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.

3 participants