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

Support firebase #548

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Support firebase #548

wants to merge 3 commits into from

Conversation

ThomasKientz
Copy link

Currently we can't use this plugin with serverless GCP (or Firebase).

As explained here fastify/fastify#946 (comment) and in the doc for firebase, we need to use req.rawBody in order to parse files from multipart.

Signed-off-by: Thomas Kientz <[email protected]>
@Uzlopak
Copy link
Contributor

Uzlopak commented Oct 8, 2024

the tests are not happy

Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

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

Decorate that new assignment to req please

Signed-off-by: Thomas Kientz <[email protected]>
index.js Outdated Show resolved Hide resolved
@ThomasKientz
Copy link
Author

the tests are not happy

@Uzlopak don't really understand why it's failing, looks good but return exit code 1

@Uzlopak
Copy link
Contributor

Uzlopak commented Oct 8, 2024

The test coverage is Not 100%

Copy link
Member

@climba03003 climba03003 left a comment

Choose a reason for hiding this comment

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

Where is payload.rawBody comes from?
I don't like the current idea to support one serverless environment and adding extra branching.

Instead one should provide a plugin that dedicated for the serverless environment and add all the necessary patching to match how Node.js works.

@Uzlopak
Copy link
Contributor

Uzlopak commented Oct 10, 2024

Well the problem is, that gcf is a glorified express router. So they preparse the body so that you dont have to. You get the body stream in rawBody. So maybe we need, like you suggest, a plugin which basically removes the default content type parsers and just assigns rawbody to body?!

@climba03003
Copy link
Member

climba03003 commented Oct 10, 2024

What's comes my mind is something like?

function gcp(fastify) {
  // add pass through content-type parser
  fastify.addContentTypeParser('application/json', {}, (req, body, done) => {
    done(null, body.body);
  });
  
  // add stream transform for multipart when neccessary
  if (fastify.hasContentTypeParser('multipart/form-data')) {
    fastify.addHook('preParsing', function(request, reply, payload, done) {
      if(request.headers['content-type'].startsWith('multipart/form-data')) {
        // we override the `.pipe` method and return rawBody as new payload
        const pipe = request.raw.pipe
        request.raw.pipe = function(stream) {
          Readable.from([rawBody]).pipe(stream)
        }
        request.raw.originalPipe = pipe
        done(null, payload.rawBody)
      } else {
        done(null, payload)
      }
    })
  }
}

@ThomasKientz
Copy link
Author

Should I pursue the current implementation of this pull request and add test coverage? @Uzlopak

@Fdawgs Fdawgs requested a review from Copilot November 20, 2024 10:54

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no suggestions.

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.

4 participants