fix(user-report): Make all fields but event-id optional #886
Conversation
| @@ -8,9 +8,15 @@ pub struct UserReport { | |||
| /// The event ID for which this user feedback is created. | |||
| pub event_id: EventId, | |||
| /// The user's name. | |||
| // Empty strings are in fact stored in the database already | |||
There was a problem hiding this comment.
You can remove those comments.
Should this be Option<String>, instead, to support explicit null?
There was a problem hiding this comment.
I'm just reading the sourcecode of Sentry, but here's what I believe holds:
The "schema" for user report is "defined" is as the set of possible keyword-arguments to the Python model itself. Anything goes that the model constructor accepts. Explicit null was never really supported. Also changing to Option means that we would have to coerce to empty string on serialization precisely to avoid running UserReport(email=None).save() which would violate NotNullConstraint of the database.
There was a problem hiding this comment.
Sure, yet, we're designing the ingestion interface here. To keep it consistent with the main event schema, and to not invert expectations, I would prefer to allow null going forward, too.
There was a problem hiding this comment.
To add to the above, the API can and must abstract over how database access is or used to be handled in Sentry. This may change any time, and now that it occurs in a separate code base, we're no longer bound by implicit behavior that improperly tested code used to expose accidentally.
* master: feat: Enable use of multiple value types per field (#882) fix(user-report): Make all fields but event-id optional (#886) release: 20.12.1 release: 20.12.0 ci(release): Move to getsentry/publish for releases (#885) meta: Fix CODEOWNERS (#884) fix: Also build rdkafka with ssl if ssl feature is enabled (#881)
the exact schema has been really ill-defined, and it appears most SDK devs expect those fields to be optional now. We already store empty strings for all of those fields, so let's just make them all optional.
The "schema" for user report is "defined" is as the set of possible keyword-arguments to the Python model itself. Anything goes that the model constructor accepts. Explicit null was never really supported. Also changing to Option means that we would have to coerce to empty string on serialization precisely to avoid running UserReport(email=None).save() which would violate NotNullConstraint of the database.