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

preParsing hook payload manipulation and consistent Content Type Parser API #2286

Merged
merged 10 commits into from
May 20, 2020
Merged

preParsing hook payload manipulation and consistent Content Type Parser API #2286

merged 10 commits into from
May 20, 2020

Conversation

ShogunPanda
Copy link
Contributor

@ShogunPanda ShogunPanda commented May 18, 2020

Hi all!

This PR focuses on two main things: preParsing hook payload manipulation hook and consistent Content Type Parser API.
The description are below.

This PR solves #1986 and it is needed for fastify/fastify-compress#105 (via fastify/fastify-compress#110).

This PR supersedes #2278.

Hope this helps!
Paolo

preParsing hook payload manipulation

This PR add supports for a third parameter for the the preParsing hook before the callback, in order to allow manipulation of request body stream before attempting to parse.

The new hook signature is fn(request,reply,payload,[done]). Payload is the current request body stream. The done function argument (or the async return value) must be a stream itself.

The rationale for this is to being able to handle compressed or encrypted request payload.
At the moment this can be achieved only inside the handler, which forces the developer to give up on fastify's serialization or validation and reimplement them inside the handler itself.

Example of gzipped request payload:

fastify.addHook('preParsing', (request, reply, payload, done) => {
  if(request.headers['content-encoding'] !== 'gzip') {
    done(null, payload)
  }
  // Some code
  done(null, payload.pipe(zlib.createGunzip()))
})

The hook should also ideally set the receivedEncodedLength property on the returned stream. The property is used to correctly match the request payload with the Content-Length header value. Ideally, but completely optional, this property should be updated on each received chunk in order to allow earlier termination for large request payloads.

Consistent Content Type Parser API

The PR deprecates the fn(req, done) and async fn(req) style Content Type Parser in favor of fn(request, payload, done) and async fn(request, payload).

The rationale for this change is to have a consistent callback styles API across all Fastify API. This was the last one.

The newer forms are used in all cases:

  • If parseAs is not passed to addContentTypeParser, then payload is a stream.
  • If parseAs is passed to addContentTypeParser, then payload is a string or a buffer accordingly.

Note that request, unlike in the past is a Fastify request object, not an IncomingMessage.

The older forms are still supported but emit a warning when used.

TypeScript notes

For both changes, I did not keep existing types.
The reason for this are the following:

  1. Forbid use of deprecated forms
  2. For the preParsing hook, I was not able to provided non ambigous callback typings.

Checklist

  • run npm run test and npm run benchmark
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message and code follows Code of conduct

Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The migration guide should be updated.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

lib/warnings.js Outdated Show resolved Hide resolved
@ShogunPanda
Copy link
Contributor Author

@jsumners Ok, will update this tomorrow.

Copy link
Member

@Ethan-Arrowood Ethan-Arrowood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

types lgtm

@ShogunPanda
Copy link
Contributor Author

Migration guide and test symbols updated.

Copy link
Member

@delvedor delvedor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work!

@delvedor delvedor added feature v3.x Issue or pr related to Fastify v3 labels May 19, 2020
docs/Migration-Guide-V3.md Outdated Show resolved Hide resolved
docs/Migration-Guide-V3.md Outdated Show resolved Hide resolved
docs/Migration-Guide-V3.md Outdated Show resolved Hide resolved
docs/Migration-Guide-V3.md Outdated Show resolved Hide resolved
ShogunPanda and others added 4 commits May 19, 2020 14:26
Co-authored-by: James Sumners <[email protected]>
Co-authored-by: James Sumners <[email protected]>
Co-authored-by: James Sumners <[email protected]>
Co-authored-by: James Sumners <[email protected]>
@mcollina mcollina merged commit 620e96a into fastify:master May 20, 2020
@ShogunPanda ShogunPanda deleted the preparsing-manipulation branch May 20, 2020 08:42
sergejostir added a commit to sergejostir/fastify that referenced this pull request May 13, 2021
mcollina pushed a commit that referenced this pull request May 13, 2021
* Remove deprecation FSTDEP001

* Remove deprecation FSTDEP002

* Remove deprecation FSTDEP003

* fixup! preParsing hook payload manipulation and consistent Content Type Parser API  (#2286)

* Remove deprecation FSTDEP004
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 10, 2022
@Eomm Eomm added the semver-minor Issue or PR that should land as semver minor label Apr 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
semver-minor Issue or PR that should land as semver minor v3.x Issue or pr related to Fastify v3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants