Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose 'local()' method to allow request content to be accessed in fairing #2575

Closed
wants to merge 0 commits into from

Conversation

martynp
Copy link
Sponsor

@martynp martynp commented Jul 9, 2023

Work-in-progress to expose "local()" method in Request module to enable body content to be pre-processed.

@martynp
Copy link
Sponsor Author

martynp commented Jul 9, 2023

#2576

@martynp martynp changed the title Add 'allow-data-mutation' feature flag to expose 'local()' method in … Expose 'local()' method to allow request content to be accessed in fairing Nov 28, 2023
@martynp martynp marked this pull request as ready for review November 28, 2023 10:17
@martynp
Copy link
Sponsor Author

martynp commented Nov 28, 2023

Example use case here:

https://github.com/martynp/rocket_hmac

If acceptable I will work up some documentation etc. for the local method

@SergioBenitez
Copy link
Member

SergioBenitez commented Nov 28, 2023

I don't think we should expose this. It encourages reading the request body data into memory, which is not something I'd like to do. Unless there's a significantly motivated reason to do this, I think we should find a better API which encourages a pass-through stream of the data.

For example, in your project, the ideal seems to be the ability to do something like the following given some &mut Data:

data.adapt_fold(|req| Hasher::new(), |hasher, req, chunk: &[u8]| hasher.hash(chunk), |req, n, hasher| {
    req.local_cache(|| Hash(hasher.finish()));
});

I think what we want is something functionally equivalent to the following trait:

trait DataAdapter {
    async fn adapt<'r, S: Stream<Item = Cow<'r, [u8]>>>(
        &mut self,
        req: &Request<'_>,
        stream: S
    ) -> impl Stream<Item = Cow<'r, [u8]>>;

    async fn finish(&mut self, n: N, req: &Request<'_>);
}

And then we can write helpers like the adapt_fold above. This allows streams to be adapted endlessly. IE, you could call data.adapt(adapter1).adapt(adapter2).adapt(adapter3) and so on. It also allows us to use all of the existing functionality for streams.

A similar API would be to instead pass in an AsyncRead and expect another AsyncRead. In either case, both of these APIs encourage streaming the data, perhaps the former more so. It does not seem particularly straightforward to implement such an API, however, but I'd be in favor of adding such functionality.

@martynp
Copy link
Sponsor Author

martynp commented Dec 5, 2023

Thanks for the feedback!

With the AsyncRead option would that sit in the vicinity of the open method in the Data module?

Given this is in the request, I assume this should not have the ability to mutate data in the steam?

I can see it would be a bit difficult to implement - I have used simple adaptors before for reading compresses & encrypted files, but always as separate entities implementing the much simpler std::io::read trait.

I will have a tinker.

@martynp martynp marked this pull request as draft December 5, 2023 19:05
@SergioBenitez
Copy link
Member

Yes that's right. I've implemented a draft of the idea. I can post it here for you later today, and perhaps you can test it out and take it across the finish line. How's that sound?

@martynp
Copy link
Sponsor Author

martynp commented Dec 6, 2023

That would be great, I definitely will look at it and do the docs etc.

@SergioBenitez
Copy link
Member

See #775 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants