Skip to content
New issue

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

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

Already on GitHub? # to your account

feat(replays): Add looser type requirements for the user.id field #1443

Merged
merged 6 commits into from
Sep 1, 2022

Conversation

cmanallen
Copy link
Member

@cmanallen cmanallen commented Aug 31, 2022

Skip validation of the user.id field. We will validate on the consumer side with the intent of adding stricter validation to relay in the future.

@cmanallen cmanallen requested a review from a team August 31, 2022 17:11
@@ -147,7 +147,7 @@ struct VersionedMeta {

#[derive(Debug, Default, Deserialize, Serialize)]
struct User {
id: Option<String>,
id: Option<Value>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a quick question: is User ID can be anything or is there an agreed upon definition of how the user should looks like?

Copy link
Member Author

Choose a reason for hiding this comment

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

String or number. Mostly I'm just not concerned with validating it here. At least not yet. I want to allow anything for now and let the snuba consumer sort it out.

I want to use relay_general::protocol::user::User but it was a little difficult to integrate and its something I want to come back to. If you have any tips for utilizing it let me know.

Copy link
Member

@jjbayer jjbayer left a comment

Choose a reason for hiding this comment

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

Please add a PR description.

@cmanallen cmanallen merged commit 6423481 into master Sep 1, 2022
@cmanallen cmanallen deleted the replays-add-coverage-for-failure branch September 1, 2022 15:39
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants