[3/n] rename request_body_max_bytes to default_request_body_max_bytes #1173
Conversation
Created using spr 1.3.6-beta.1 [skip ci]
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1 [skip ci]
| #[derive(Clone, Debug, PartialEq, Serialize)] | ||
| pub enum InvalidConfig {} | ||
|
|
||
| fn deserialize_invalid_request_body_max_bytes<'de, D>( |
There was a problem hiding this comment.
do we prefer this to impl Serialize for InvalidConfig?
There was a problem hiding this comment.
This pattern allows a custom error message to be passed in -- InvalidConfig is a general marker value that shouldn't necessarily be tied with the specific error message.
I'll add a comment to this note.
There was a problem hiding this comment.
Oh also this returns an Option<InvalidConfig> which wouldn't be possible to do via a type implementation.
There was a problem hiding this comment.
Oh also this returns an
Option<InvalidConfig>which wouldn't be possible to do via a type implementation.
that's true... but I think impl Serialize for InvalidConfig would probably suffice since the implementation of Serialize for Option will call it.
There was a problem hiding this comment.
Hmm, InvalidConfig doesn't implement Serialize at all (the skip_serializing makes that moot). This code only does deserialization.
| #[serde(default)] | ||
| #[serde( | ||
| from = "DeserializedConfigDropshot", | ||
| into = "DeserializedConfigDropshot" |
There was a problem hiding this comment.
what's the point of the into?
There was a problem hiding this comment.
It is possible to derive Serialize on ConfigDropshot directly, and it would work today, but in case there's more divergence between the in-memory implementation and the serializable implementation over time, that may no longer hold true. I've seen this be a recurring source of bugs in other places, and making the types that directly interact with serde be the same for serialization and deserialization has always been helpful.
| default_handler_task_mode: HandlerTaskMode::Detached, | ||
| log_headers: Default::default(), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] |
There was a problem hiding this comment.
does this need to be Serialize?
There was a problem hiding this comment.
This is coupled with the into comment.
sunshowers
changed the base branch from
sunshowers/spr/main.3n-rename-request_body_max_bytes-to-default_request_body_max_bytes
to
main
Created using spr 1.3.6-beta.1
|
Going to go ahead and land this per chat message -- happy to address lingering comments in a followup. |
sunshowers
deleted the
sunshowers/spr/3n-rename-request_body_max_bytes-to-default_request_body_max_bytes
branch
In upcoming work (#1171), we're going to add optional per-endpoint size limits.
To make it clear that per-endpoint limits always override whatever's specified
in the configuration, rename this field.
(Without the rename, it can be confusing to determine what happens if the
per-endpoint limit is smaller than the configuration limit.)
Also add a deserialize-time check to fail if
request_body_max_bytesisspecified, guiding users to the new name.
Depends on: