Unable to implement an extractor the references my app-specific server context

[inickles]

I was looking to implement an ExclusiveExtractor that references data in my server context, and at first glance I thought I was going to be able to, but now I'm not seeing how I can specify my concrete type to be able to do so.
I want to do something like:

#[async_trait]
impl<T> ExclusiveExtractor for VerifiedGithubBody<T>
where
    T: Sync + Send,
{
    async fn from_request(
        rqctx: &RequestContext<MyContext>,
        request: hyper::Request<hyper::Body>,
    ) -> Result<Self, HttpError> {
        let context = rqctx.context();
        let hmac_secret = context.github_hmac_secret;

        ...
    }
...
}

where MyContext is:

pub(crate) struct MyContext{
    pub(crate) github_hmac_secret: secrecy::SecretString,
}

but that doesn't work.

I can do all the work of this in the endpoint handler, which is fine, but it would be my preference to do this kind of thing in an extractor. Alternatively, I could pass that access that data via other means, such as environment variables, but it is also my preference to not use that for secrets. The server context seems like an appropriate place to store that.

[jclulow]

Have you see what @augustuswm did in https://github.com/oxidecomputer/dropshot-verified-body/blob/main/src/services/github.rs ?

[inickles]

Have you see what @augustuswm did in https://github.com/oxidecomputer/dropshot-verified-body/blob/main/src/services/github.rs ?

I did, yeah, that's kind of how I got here. I noticed it was getting the key from env (https://github.com/oxidecomputer/dropshot-verified-body/blob/main/src/services/github.rs#L40) and him and I were discussing how we'd like for that to come from the context instead, so I thought I'd work up a PR for it. But then I got stuck here.

[davepacheco]

@inickles sorry that I missed this when it came in. How exactly does your example fail?

My guess is that in order to impl ExclusiveExtractor, you need to impl a from_request() that accepts any Context: ServerContext, which isn't necessarily yours. For this to work, I think ExclusiveExtractor itself (rather than its from_request method) would have to be parametrized by the context type and then we'd have to make sure you only use one having the same context type as the handler. I'd want to play with that a bit but it seems probably fine. Anybody who was impl'ing the old one can still impl the new one, I think -- it'd just be generic over the type parameter. But you'd be able to impl ExclusiveExtractor<YourContextType> and then only use it with request handlers that accept your context type.

This would probably be a breaking change, though, unless we went through the trouble of defining a new ContextSpecificExclusiveExtractor<Context: ServerContext> and then made the rules more complicated ("you can have at most one of {ExclusiveExtractor, ContextSpecificExclusiveExtractor} for each handler ... ").

We may want to do the same with SharedExtractor.

Sorry about the trouble.

[inickles]

@inickles sorry that I missed this when it came in. How exactly does your example fail?

No worries! I consider this a minor enhancement request, nothing that needs addressing soon.

My guess is that in order to impl ExclusiveExtractor, you need to impl a from_request() that accepts any Context: ServerContext, which isn't necessarily yours. For this to work, I think ExclusiveExtractor itself (rather than its from_request method) would have to be parametrized by the context type and then we'd have to make sure you only use one having the same context type as the handler. I'd want to play with that a bit but it seems probably fine. Anybody who was impl'ing the old one can still impl the new one, I think -- it'd just be generic over the type parameter. But you'd be able to impl ExclusiveExtractor<YourContextType> and then only use it with request handlers that accept your context type.

Yeah, that sounds right to me.

This would probably be a breaking change, though, unless we went through the trouble of defining a new ContextSpecificExclusiveExtractor<Context: ServerContext> and then made the rules more complicated ("you can have at most one of {ExclusiveExtractor, ContextSpecificExclusiveExtractor} for each handler ... ").

I wonder if we could set a default generic type parameter to the unit type and have it not break for existing users, so trait ExclusiveExtractor<T = ()>. dropshot would still need to know which implementation to use, as you say above, which I'm guessing it could just get from the handler?

Sorry about the trouble.

Really, no worries at all.

cc @sunshowers , as we'd discussed this issue a little bit ago.

[sunshowers]

Keeping API traits in mind, which are by definition generic over a variety of contexts -- if we decide to allow more restricted contexts then I think we'd have to do it with respect to a trait. So in the

trait MyContextTrait: ServerContext {
    fn github_hmac_secret(&self) -> secrecy::SecretString;
}

#[dropshot::api_description]
trait MyApi {
    type Context: MyContextTrait;

    // ...
}

Then you'd write impl<T: MyContextTrait> ExclusiveExtractor<T> for XYZ.

I'm around 90% sure the types will work out if this mechanism is followed.

[sunshowers]

I wonder if we could set a default generic type parameter to the unit type and have it not break for existing users, so trait ExclusiveExtractor<T = ()>. dropshot would still need to know which implementation to use, as you say above, which I'm guessing it could just get from the handler?

I think it would be best to pass in the actual server context uniformly here to minimize confusion.

made the rules more complicated ("you can have at most one of {ExclusiveExtractor, ContextSpecificExclusiveExtractor} for each handler ... ").

Oh interesting -- you might be able to do something like:

trait ContextSpecificExclusiveExtractor<T: ServerContext> { ... }

impl<E: ExclusiveExtractor, T: ServerContext> ContextSpecificExclusiveExtrator<T> for E { ... }

[davepacheco]

@sunshowers to be clear, you're saying that if we do what I proposed above (make dropshot::ExclusiveExtractor parameterized by T so that its from_request accepts RequestContext<T>), and if you're using the trait API, then you'd need to do what you suggested? That makes sense to me.

Or are you saying that it wouldn't work at all to parameterize dropshot::ExclusiveExtractor in that way because of the trait-based API?

[sunshowers]

@davepacheco The former. If we add a type parameter to ExclusiveExtractor, then to make it compatible with API traits we'd have to use this kind of trait scheme.