feat(playstation): Emit outcome for skipped fields #4862
Conversation
| impl IntoResponse for BadMultipart { | ||
| fn into_response(self) -> Response { | ||
| let status_code = match self { | ||
| BadMultipart::Multipart( | ||
| multer::Error::FieldSizeExceeded { .. } | multer::Error::StreamSizeExceeded { .. }, | ||
| ) => StatusCode::PAYLOAD_TOO_LARGE, | ||
| _ => StatusCode::BAD_REQUEST, | ||
| }; | ||
|
|
||
| (status_code, ApiErrorResponse::from_error(&self)).into_response() | ||
| } | ||
| } |
There was a problem hiding this comment.
This copies the behaviour found here:
relay/relay-server/src/extractors/remote.rs
Lines 61 to 70 in 25f09a4
| @@ -177,7 +215,10 @@ where | |||
| let content_type = field.content_type().cloned(); | |||
| let field = LimitedField::new(field, config.max_attachment_size()); | |||
| match field.bytes().await { | |||
| Err(multer::Error::FieldSizeExceeded { .. }) if ignore_large_fields => continue, | |||
| Err(multer::Error::FieldSizeExceeded { limit, .. }) if ignore_large_fields => { | |||
There was a problem hiding this comment.
Cheating here a bit by putting the consumed size into the error rather than the limit that is breached such that we can use that value to generate the outcome.
| |quantity| { | ||
| outcome_aggregator.send(TrackOutcome { | ||
| timestamp: request_meta.received_at(), | ||
| scoping: request_meta.get_partial_scoping(), | ||
| outcome: Outcome::Invalid(DiscardReason::TooLarge( | ||
| DiscardItemType::Attachment(DiscardAttachmentType::Attachment), | ||
| )), | ||
| event_id: None, | ||
| remote_addr: request_meta.remote_addr(), | ||
| category: DataCategory::Attachment, | ||
| quantity, | ||
| }) | ||
| }, |
There was a problem hiding this comment.
I tried making this a method of UnconstrainedMultipart but that did not work since we also need the self.multiparts when invoking multipart_items hence it is like this.
| config: &Config, | ||
| ignore_large_fields: bool, | ||
| ) -> Result<Items, multer::Error> | ||
| where | ||
| F: FnMut(Option<&str>, &str) -> AttachmentType, | ||
| G: FnMut(u32), |
There was a problem hiding this comment.
I would suggest making this an FnMut(Outcome, u32) and generating the Outcome::Invalid(DiscardReason::TooLarge(…)) outcome in the if ignore_large_fields branch below (and don't pass it in UnconstrainedMultipart::items).
The reason is admittedly kind of aesthetic—here you have a function called emit_outcome that only handles u32 values.
| pub multipart: Multipart<'static>, | ||
| pub outcome_aggregator: Addr<TrackOutcome>, | ||
| pub request_meta: RequestMeta, |
There was a problem hiding this comment.
This fields probably shouldn't be public, this is now implementation detail territory, if you need access to the multipart, I'd just add a into_multipart() or From impl instead.
Currently there is no communication to the user about a field being skipped. This PR adds the logic for emitting outcomes for skipped fields such that the users are aware of these skipped fields.
Follow up to: #4793