make it a compile error to accept more than one body extractor #556

davepacheco · 2023-01-10T00:34:45Z

See #555 for motivation. We currently verify at runtime that you don't have two extractors that both try to read the body. We could make this a compile-time error instead.

This currently depends on #554 so I'm going to leave this as "draft". Once #554 lands, I will update the base branch of this PR. We may also want to wait for #557 (which implements #555), and then also wait for a prototype of that in Omicron, before landing this. Until we have all that, this could all be a bust.

Status:

get consensus that this seems promising
confirm it's not a disaster in consumers like Omicron. (It was very simple. There's just one custom extractor and it's easily converted to a SharedExtractor.) I'm happy to check any other consumers people would like.
get consensus on the rest of this path (remove hyper::Request from RequestContext #557 at least, and maybe remove Arc around RequestContext #558)
polish up this PR
- changelog update
- fix error messages so they're as useful as they were before when your endpoint function arg isn't an extractor
- make sure we have tests
- any other XXXs
fix up websocket argument order (and changelog for that)

ahl

looks good

ahl · 2023-01-11T01:33:30Z

dropshot/src/extractor/common.rs

@@ -0,0 +1,198 @@
+// Copyright 2022 Oxide Computer Company


2023 here and elsewhere?

Done in 8ee3aa9.

ahl · 2023-01-11T01:38:50Z

dropshot/src/extractor/common.rs

+pub trait ExclusiveExtractor: Send + Sync + Sized {
+    /// Construct an instance of this type from a `RequestContext`.
+    async fn from_request<Context: ServerContext>(
+        rqctx: &RequestContext<Context>,


I assumed this would be either the actual RequestContext<Context> or a &mut to it; perhaps that's in a subsequent PR...

Counter-intuitively, this works with just &RequestContext<Context>. That's because what exclusive extractors need mutable access to is the hyper::Request, which is in the RequestContext but wrapped with Arc<Mutex<...>>. So you can take the lock to access a &mut hyper::Request.

After #558, the hyper::Request won't even be in the RequestContext any more.

So given that it's not necessary to have a &mut RequestContext and that the caller is going to pass the same RequestContext to the request handler function after calling this one, I prefer giving the extractor just a &RequestContext. Hopefully that all makes sense.

dropshot/src/extractor/common.rs

ahl · 2023-01-11T01:56:17Z

dropshot/tests/fail/bad_endpoint3.stderr

-  --> tests/fail/bad_endpoint3.rs:17:12
-   |
-17 |     param: String,
-   |            ^^^^^^ the trait `Extractor` is not implemented for `String`


this seems helpful and I'm not sure the parts that remain are as helpful

Totally. Since you're on board with this general idea, the next step is to figure out how to ensure good error messages when (1) an argument is not an extractor at all, or (2) you attempt to use two exclusive extractors. I think (1) is pretty easy because I made SharedExtractor impl ExclusiveExtractor, so we can change the existing check to check ExclusiveExtractor. I have not yet figured out a good way to do (2) but Rain suggested an approach I'm going to look at.

Done. The basic approach is to verify that:

the first argument is a RequestContext (like we did before)

the last argument is an ExclusiveExtractor (slight variation on what we did before)

all other arguments are SharedExtractor

This has the side effect of verifying that you don't have two exclusive extractors because they don't impl SharedExtractor.

The naming of the tests is a little weird. I kept the existing dense integer naming but it's a little confusing that test 19 is a slight variant of test 3. (That is, the behavior tested by the old bad_endpoint3.rs is now tested by a combination of bad_endpoint3.rs and bad_endpoint19.rs.) The alternatives seemed like separating into 3a and 3b or renaming everything to be more semantic. 🤷

ahl · 2023-01-11T01:58:57Z

dropshot_endpoint/src/lib.rs

            sig.output =
                syn::parse2(quote!(-> dropshot::WebsocketEndpointResult))?;


huh.. had not seen this construction.

ahl · 2023-01-11T02:00:49Z

dropshot_endpoint/src/lib.rs

-            const _: fn() = || {
-                fn need_extractor<T>()
-                where
-                    T: ?Sized + dropshot::Extractor,


could this change to RequestExtractor?

... or maybe SharedExtractor?

I think it could change to ExclusiveExtractor (shared extractors can be treated as exclusive, but not the other way around). (see my other comment -- I do intend to restore a helpful message for this case, one way or another)

ahl · 2023-01-11T02:02:33Z

dropshot_endpoint/src/lib.rs

@@ -173,7 +173,8 @@ fn do_channel(
        ChannelProtocol::WEBSOCKETS => {
            // here we construct a wrapper function and mutate the arguments a bit


Suggested change

// here we construct a wrapper function and mutate the arguments a bit

// Here we construct a wrapper function and mutate the arguments a bit

Done in fb31831.

davepacheco · 2023-01-12T19:24:50Z

dropshot_endpoint/src/lib.rs

+            // Historically, we required that the `WebsocketConnection` argument
+            // be first after the `RequestContext`.  However, we also require
+            // that any exclusive extractor (which includes the
+            // `WebsocketUpgrade` argument that we put in its place) appears
+            // last.  We replaced the type above, but now we need to put it in
+            // the right spot.
+            sig.inputs = {
+                let mut input_pairs =
+                    sig.inputs.clone().into_pairs().collect::<Vec<_>>();
+                let second_pair = input_pairs.remove(1);
+                input_pairs.push(second_pair);
+                input_pairs.into_iter().collect()
+            };


I'm curious what people think about this. The comment basically summarizes it: we previously said that a WebsocketConnection has to be the first extractor. As part of this PR, I decided that exclusive extractors (which would include this one) have to be last. I did that because I think it's natural for the extractors to appear in the order they look at parts of the HTTP request, and exclusive extractors generally look at the request body (the last thing).

There's not actually a conflict here because this macro already translates the user's argument list into what the endpoint macro expects, so we can shuffle the arguments around like this. But it's kind of annoying that we have to do this, and a user might notice the weirdness that their WebsocketConnection has to appear first, but other exclusive extractors have to appear last.

We could:

do nothing (what we're doing here). Downside: this ugliness, and maybe slightly confusing for people?

make WebsocketConnection appear last. Downside: breaking change, though trivial to fix.

make exclusive extractors appear first instead. Downside: less intuitive for the much more common case

Thoughts @ahl @lifning ?

I like making WebsocketConnection appear last -- it's a breaking change but I think it's justified by the overall direction of the PR.

likewise, i would by far prefer consistency over just leaving things as-is. and being that the only code using it now is this one thing, now would be the time to make such a change

davepacheco · 2023-01-12T19:25:50Z

I've sync'd this change up with "main", cleaned up the tests and other XXXs, and written migration instructions in the changelog. I think it's ready to go except maybe for this one question.

@ahl Unfortunately I think the way I sync'd this up will make it annoying to do an incremental review. Sorry. Let me know if I can help.

davepacheco added 7 commits January 9, 2023 13:22

switch block-style to line-style comments

b38f14c

do not update rustfmt.toml too

3ac8c39

fix websocket.rs (thanks @ahl!)

3b48121

move extractors and generic schema utilities out of handler.rs

9df46f7

Merge remote-tracking branch 'origin/main' into extractor-move

d9307c4

fix style

db9770f

WIP: making it work, need to rebase

41a1cd6

davepacheco changed the base branch from main to extractor-move January 10, 2023 00:34

davepacheco mentioned this pull request Jan 10, 2023

provide access to raw hyper::Request via an extractor #555

Closed

update changelog with some todo items

833920f

davepacheco mentioned this pull request Jan 10, 2023

remove hyper::Request from RequestContext #557

Merged

7 tasks

ahl reviewed Jan 11, 2023

View reviewed changes

fix copyrights

8ee3aa9

Base automatically changed from extractor-move to main January 11, 2023 18:35

davepacheco added 16 commits January 11, 2023 10:43

review feedback

fb31831

WIP: making it work, need to rebase

1dff466

update changelog with some todo items

1c7642b

fix copyrights

1673519

review feedback

f77a40d

Merge branch 'merge-tmp' into exclusive-extractors

e962e2b

WIP: making it work, need to rebase

f4dbd20

update changelog with some todo items

ec810ea

fix copyrights

29c6b47

review feedback

16d7ac3

merge with "main"

c228e40

Merge branch 'merge-with-main' into exclusive-extractors

f68c794

fix style

08c9a9a

update tests

38324b4

remove TODO

b200e10

rename tests

5bb93e7

davepacheco added 6 commits January 11, 2023 12:28

fix output

aa6f16e

copyright update was over-eager

0f956a5

Merge remote-tracking branch 'origin/main' into exclusive-extractors

95afcad

update CHANGELOG

5979394

bump for major version

0c727b9

clean up docs, XXXs

5ec8a45

davepacheco commented Jan 12, 2023

View reviewed changes

davepacheco added 2 commits January 18, 2023 08:54

websocket args should be last, too

185f7f8

Merge remote-tracking branch 'origin/main' into exclusive-extractors

0f78f7d

davepacheco marked this pull request as ready for review January 18, 2023 16:57

found one straggler

00193d1

This was referenced Jan 18, 2023

Add a new ChunkedBody extractor #541

Closed

[draft] straw proposal to remove the request from RequestContext #548

Closed

davepacheco merged commit 538e99b into main Jan 18, 2023

davepacheco deleted the exclusive-extractors branch January 18, 2023 22:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make it a compile error to accept more than one body extractor #556

make it a compile error to accept more than one body extractor #556

davepacheco commented Jan 10, 2023 •

edited

Loading

ahl left a comment

ahl Jan 11, 2023

davepacheco Jan 11, 2023

ahl Jan 11, 2023

davepacheco Jan 11, 2023

ahl Jan 11, 2023

davepacheco Jan 11, 2023

davepacheco Jan 11, 2023

ahl Jan 11, 2023

ahl Jan 11, 2023

ahl Jan 11, 2023

davepacheco Jan 11, 2023

ahl Jan 11, 2023

davepacheco Jan 11, 2023

davepacheco Jan 12, 2023

sunshowers Jan 17, 2023

lifning Jan 18, 2023

davepacheco commented Jan 12, 2023

		sig.output =
		syn::parse2(quote!(-> dropshot::WebsocketEndpointResult))?;

		@@ -173,7 +173,8 @@ fn do_channel(
		ChannelProtocol::WEBSOCKETS => {
		// here we construct a wrapper function and mutate the arguments a bit

make it a compile error to accept more than one body extractor #556

make it a compile error to accept more than one body extractor #556

Conversation

davepacheco commented Jan 10, 2023 • edited Loading

ahl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davepacheco commented Jan 12, 2023

davepacheco commented Jan 10, 2023 •

edited

Loading