update max request size #64

jessfraz · 2020-12-03T05:31:22Z

Did one for #23, would you like all those fields to be the same way?

jessfraz · 2020-12-03T05:32:41Z

hmm my version and yours of rustfmt are not in sync

ahl · 2020-12-03T05:39:43Z

hmm my version and yours of rustfmt are not in sync

The struggle is real. This is how we attempted to work around this:
https://github.com/oxidecomputer/dropshot/blob/master/.vscode/settings.json

We pinned down a specific version because we found the spurious changes to be annoying:
https://github.com/oxidecomputer/dropshot/blob/master/.github/workflows/rust.yml#L20

ahl

@davepacheco looks good to me

davepacheco · 2020-12-03T18:58:12Z

Cool! Thanks for doing this.

What do you think about using the Default trait for ConfigDropshot instead of an Option here? I haven't used Default much myself, but I think it would be more clearly express what's going on. When I saw this was an Option, I expected that None would mean "no limit" not "default limit".

Yes, I think it would be great to add the other two fields here (definitely not a blocker for this PR).

jessfraz · 2020-12-04T01:04:07Z

Can’t implement default on a type like usize want me to make a new type for it

jessfraz · 2020-12-04T01:13:41Z

Did that :)

jclulow · 2020-12-04T01:14:53Z

I reckon we could provide a manual implementation of Default for ConfigDropshot which would return a copy of the struct with Some(1024) (or whatever) filled in as the default value for the new property?

Then you could...

let cfg = ConfigDropshot {
    bind_address: "127.0.0.1:0".parse().unwrap(),
    ..Default::default()
};

... and you'd be proofed against future additional fields.

davepacheco · 2020-12-07T16:53:34Z

Yeah, I was talking about implementing Default for ConfigDropshot, not a new type. (I don't have a feeling either way about using a new type here.) This would make it easy for us to provide defaults for other fields in the future. (I wonder if it would make it harder for us to force people to specify fields for which it doesn't make sense to have a default?)

Either way is okay right now but it looks like both style and the test suite aren't passing. Are there tests for the new change? I don't see them but maybe it's exercised by the tests that are failing?

jessfraz · 2020-12-07T17:23:16Z

Oh gotcha I’ll fix that! My bad!

…

On Mon, Dec 7, 2020 at 8:53 AM David Pacheco ***@***.***> wrote: Yeah, I was talking about implementing Default for ConfigDropshot, not a new type. (I don't have a feeling either way about using a new type here.) This would make it easy for us to provide defaults for other fields in the future. (I wonder if it would make it harder for us to force people to specify fields for which it doesn't make sense to have a default?) Either way is okay right now but it looks like both style and the test suite aren't passing. Are there tests for the new change? I don't see them but maybe it's exercised by the tests that are failing? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#64 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALA23G3ODKL7IALSHG5CV3STUCB7ANCNFSM4ULPBRDQ> .

jessfraz · 2020-12-07T17:27:06Z

Also those tests fail for me on master so I wonder if it's something with my setup..? Will look into it.

jessfraz · 2020-12-14T19:28:46Z

Okay re-did this so that it is Default on DropshotConfig.

Also sorry about the fmt changing....
And I am unsure why the tests fail for me on master as well, going to try to dive in

Signed-off-by: Jess Frazelle <[email protected]>

There was a warning: ``` warning: panic message contains a brace --> dropshot/src/router.rs:121:67 | 121 | "HTTP URI path segment variable missing leading \"{\"" | ^ | = note: `#[warn(panic_fmt)]` on by default = note: this message is not used as a format string, but will be in a future Rust edition help: add a "{}" format string to use the message literally | 121 | "{}", "HTTP URI path segment variable missing leading \"{\"" | ^^^^^ ``` This fixes that. Signed-off-by: Jess Frazelle <[email protected]>

Signed-off-by: Jess Frazelle <[email protected]>

jessfraz · 2020-12-14T19:48:47Z

Also I had to run the tests with TEST_OVERWRITE locally and sorry those are reflected in the diff. If I should not commit those files I can easily remove that commit.

jessfraz · 2020-12-14T19:51:08Z

also I am sure it's there for a reason but what do you think about removing the "version" line from the rustfmt file? that's how I was able to fmt locally, but I didn't commit that change

ahl · 2020-12-14T23:46:52Z

also I am sure it's there for a reason but what do you think about removing the "version" line from the rustfmt file? that's how I was able to fmt locally, but I didn't commit that change

That was how we ensured that we didn't wrestle over formatting. At least at the time we found that recent changes in rustfmt impacted our code. I think this is what we used previously:

rustup install nightly-2020-06-02
rustup run nightly-2020-06-02 rustfmt <file>

If you're using vscode, it should "do the right thing"

I think it would be very reasonable to update the version number in the rustfml.toml to reflect a more recent version; you'll want to update .vscode/settings.json and .github/workflows/rust.yml as well.

ahl · 2020-12-14T23:48:36Z

.. and to be clear: this all is pretty jank and we wish that there was some more reasonable way to ensure a consistent formatting of code. For example, it would be cool if the Cargo.toml could depend on a specific rustfmt version and use it. The version in rustfmt.toml which acts only to induce failures ... sucks.

jessfraz · 2020-12-14T23:49:09Z

Bleh I’ll just skip it I’m on nightly as well

…

On Mon, Dec 14, 2020 at 3:47 PM Adam Leventhal ***@***.***> wrote: also I am sure it's there for a reason but what do you think about removing the "version" line from the rustfmt file? that's how I was able to fmt locally, but I didn't commit that change That was how we ensured that we didn't wrestle over formatting. At least at the time we found that recent changes in rustfmt impacted our code. I think this is what we used previously: rustup install nightly-2020-06-02 rustup run nightly-2020-06-02 rustfmt <file> If you're using vscode, it should "do the right thing" I think it would be very reasonable to update the version number in the rustfml.toml to reflect a more recent version; you'll want to update .vscode/settings.json and .github/workflows/rust.yml as well. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#64 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALA23D7FLJVTWD5HLRTZUTSU2PXTANCNFSM4ULPBRDQ> .

davepacheco · 2020-12-22T16:33:15Z

Thanks for doing this and sticking with it! I like the Default impl and the expected test output changes look like the usual compiler churn (unfortunately), so that's expected too. Sorry it took so long for me to circle back on this.

jessfraz · 2020-12-22T16:37:31Z

No worries woohoo

…

On Tue, Dec 22, 2020 at 8:33 AM David Pacheco ***@***.***> wrote: Thanks for doing this and sticking with it! I like the Default impl and the expected test output changes look like the usual compiler churn (unfortunately), so that's expected too. Sorry it took so long for me to circle back on this. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#64 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALA23EJXKACI4ZAFV3GSETSWDC53ANCNFSM4ULPBRDQ> .

ahl requested a review from davepacheco December 3, 2020 05:39

ahl approved these changes Dec 3, 2020

View reviewed changes

jessfraz force-pushed the update-max-size branch from 8af802f to ee4c7fe Compare December 4, 2020 01:13

jessfraz force-pushed the update-max-size branch 2 times, most recently from 530cda7 to 503ad18 Compare December 4, 2020 21:40

jessfraz mentioned this pull request Dec 4, 2020

allow for bracketed, nested query params as Mailchimp does #65

Closed

jessfraz force-pushed the update-max-size branch 2 times, most recently from 9cdd63c to caeb818 Compare December 14, 2020 19:31

add request_body_max_bytes to DropshotConfig

9e312ba

Signed-off-by: Jess Frazelle <[email protected]>

jessfraz force-pushed the update-max-size branch from caeb818 to 9e312ba Compare December 14, 2020 19:36

jessfraz added 4 commits December 14, 2020 11:38

run TEST_OVERWRITE

3bc7963

Signed-off-by: Jess Frazelle <[email protected]>

fix test_config to add request_body_max_bytes

a80cde4

Signed-off-by: Jess Frazelle <[email protected]>

fix the rust docs examples

fb31766

Signed-off-by: Jess Frazelle <[email protected]>

jessfraz force-pushed the update-max-size branch from bbdaf68 to fb31766 Compare December 14, 2020 19:47

davepacheco merged commit bf4e950 into oxidecomputer:master Dec 22, 2020

davepacheco mentioned this pull request Jan 5, 2021

server tunables should be configurable #23

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update max request size #64

update max request size #64

jessfraz commented Dec 3, 2020

jessfraz commented Dec 3, 2020

ahl commented Dec 3, 2020

ahl left a comment

davepacheco commented Dec 3, 2020

jessfraz commented Dec 4, 2020

jessfraz commented Dec 4, 2020

jclulow commented Dec 4, 2020

davepacheco commented Dec 7, 2020

jessfraz commented Dec 7, 2020 via email

jessfraz commented Dec 7, 2020

jessfraz commented Dec 14, 2020

jessfraz commented Dec 14, 2020

jessfraz commented Dec 14, 2020

ahl commented Dec 14, 2020

ahl commented Dec 14, 2020

jessfraz commented Dec 14, 2020 via email

davepacheco commented Dec 22, 2020

jessfraz commented Dec 22, 2020 via email

update max request size #64

update max request size #64

Conversation

jessfraz commented Dec 3, 2020

jessfraz commented Dec 3, 2020

ahl commented Dec 3, 2020

ahl left a comment

Choose a reason for hiding this comment

davepacheco commented Dec 3, 2020

jessfraz commented Dec 4, 2020

jessfraz commented Dec 4, 2020

jclulow commented Dec 4, 2020

davepacheco commented Dec 7, 2020

jessfraz commented Dec 7, 2020 via email

jessfraz commented Dec 7, 2020

jessfraz commented Dec 14, 2020

jessfraz commented Dec 14, 2020

jessfraz commented Dec 14, 2020

ahl commented Dec 14, 2020

ahl commented Dec 14, 2020

jessfraz commented Dec 14, 2020 via email

davepacheco commented Dec 22, 2020

jessfraz commented Dec 22, 2020 via email