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

How to reference HttpRequest body in hooks #207

Closed
ejizba opened this issue Jan 3, 2024 · 4 comments · Fixed by #209
Closed

How to reference HttpRequest body in hooks #207

ejizba opened this issue Jan 3, 2024 · 4 comments · Fixed by #209
Assignees
Milestone

Comments

@ejizba
Copy link
Contributor

ejizba commented Jan 3, 2024

@ejizba I understand the design choice and looking forward to stream support. However, it would be beneficial to have an option to cache the request body. I'm writing a hook that validates the request body. The hook consumes it using .json(). Now the function has no way to access the request body. Because everything is readonly in HttpRequest I also don't see an easy option to reset/replace the consumed stream. Can you provide guidance how a preInvocation hook and a function can use the request body?

Update: I've tried to use pipeThrough, but that still causes the bodyUsed flag to be true. Looks like the only option is to copy the entire request:

  const textBody = await request.text()
  parsedBody = JSON.parse(textBody)

  // TODO: validate body here

  const headerCopy: Record<string, string> = {}
  origRequest.headers?.forEach((value, key) => {
    headerCopy[key] = value
  })

  const queryParams: Record<string, string> = {}
  origRequest.query?.forEach((value, key) => {
    queryParams[key] = value
  })

  request = new HttpRequest({
    method: origRequest.method,
    url: origRequest.url,
    body: { string: textBody },
    headers: headerCopy,
    query: queryParams,
    params: origRequest.params,
  })

Originally posted by @restfulhead in #79 (comment)

@ejizba
Copy link
Contributor Author

ejizba commented Jan 3, 2024

@restfulhead that's an interesting scenario. Other than your existing workaround, the only other one I'm aware of is for the the hook to read the body and then set the value on the context object for the function to read later. That being said, I think I like your suggested workaround better (at least for small requests where you're not concerned about performance).

I've considered adding a method HttpRequest.clone(), which would essentially be short-hand for what you're doing in your workaround. Would you be satisfied with that as the primary way to address your scenario?

@restfulhead
Copy link

restfulhead commented Jan 4, 2024

I've considered adding a method HttpRequest.clone(), which would essentially be short-hand for what you're doing in your workaround. Would you be satisfied with that as the primary way to address your scenario?

Yes, it would make my code a little easier and you'd have a recommendation on what to do in this scenario. By the way, I have the same issue regarding the response, too. Usually the response isn't consumed after my hook, but if there were other hooks that also need the response body, then they'd run into the same issue.

Unfortunately though copying means we consume the stream and then create it again only for the function (or other hooks) to consume it again. As an alternative, could there be an option to cache the result, so that subsequent calls to .json() would just read from memory? The downside is of course that the request body stays in memory, so maybe this option could be off by default? Perhaps it could be part of the signature, e.g. json(cache = false).

Just to mention, I've commented over at undici asking why pipeThrough would cause the body to be no longer consumable. I could see middleware functionality such as decompressing or decoding wanting to make use of it. In my case, I need the entire request body, so I guess this would have the same effect (consuming and then creating the stream again).

Also thought about using tee to create two copies of the stream. One copy could be consumed by the caller (i.e. by my hook) and the other stream could replace the original request's stream. But in my scenario I need to prevent further processing of the request if validation fails, so it would just have the same effect of consuming the body multiple times sequentially.

@ejizba ejizba added this to the January 2024 milestone Jan 4, 2024
@ejizba
Copy link
Contributor Author

ejizba commented Jan 4, 2024

Yes we can do clone for both request and response. It turns out the fetch/undici "clone" is a bit smarter than a simple duplication, so hopefully that covers most of the concerns.

Like the underlying ReadableStream.tee api, the body of a cloned Response will signal backpressure at the rate of the faster consumer of the two bodies, and unread data is enqueued internally on the slower consumed body without any limit or backpressure. Backpressure refers to the mechanism by which the streaming consumer of data (in this case, the code that reads the body) slows down the producer of data (such as the TCP server) so as not to load large amounts of data in memory that is waiting to be used by the application. If only one cloned branch is consumed, then the entire body will be buffered in memory. Therefore, clone() is one way to read a response twice in sequence, but you should not use it to read very large bodies in parallel at different speeds.

https://developer.mozilla.org/en-US/docs/Web/API/Response/clone

@ejizba ejizba self-assigned this Jan 4, 2024
@restfulhead
Copy link

Sounds good

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

Successfully merging a pull request may close this issue.

2 participants