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

Option to attach to body as key-value pairs #348

Merged
merged 7 commits into from Jun 8, 2022

Conversation

zone117x
Copy link
Contributor

@zone117x zone117x commented Jun 3, 2022

Adds an option attachFieldsToBody: 'valueOnly'. When this is option is enabled, the field values are attached to the body directly, similar to what is described in the readme:

  const body = Object.fromEntries(
    Object.keys(req.body).map((key) => [key, req.body[key].value])
  ) // Request body in key-value pairs, like req.body in Express

This would be a convenient feature to include so that json-schemas can be shared more easily between the content types application/json, application/x-www-form-urlencoded, and multipart/form-data.

Without this capability, this needs to be performed in awkward route handler transforms, e.g.:

fastify.post<{ Body: ServiceAuthRequest, Reply: ServiceAuthResponse }>('/request_key', {
  preValidation: (request, _reply, done) => {
    if (request.isMultipart()) {
      // 🤢
      const body = request.body as unknown as Record<string, { value: string } | { _buf: Buffer }>;
      for (let [k, v] of Object.entries(body)) {
        const value = 'value' in v ? v.value : v._buf.toString();
        (request.body as any)[k] = value;
      }
    }
    done();
  },
  schema: {
    body: ServiceAuthRequestSchema,
    headers: ServiceAuthRequestHeadersSchema,
    response: {
      200: ServiceAuthResponseSchema,
    }
  },
}, 
async (request, reply) => {
  ...
});

Please let me know if the the convention attachFieldsToBody: 'valueOnly' should be changed to something else.

Also, currently files are converted to a string via .toBuffer().toString(). It's possible to use the onFile handler to customize the file buffer decoding, which is performed in one of the new tests.

Let me know if this feature make sense, and I'm happy to add info to the docs/readme.

Checklist

@kibertoad
Copy link
Member

This makes sense to me.
@Uzlopak wdyt?

@Uzlopak
Copy link
Contributor

Uzlopak commented Jun 4, 2022

It looks kind of confusing that valuesOnly still creates key value pairs. Also is the for loop fast? I would do classical with Object.keys.
And lastly does it not potentially needs protection against prototype pollution?

@zone117x
Copy link
Contributor Author

zone117x commented Jun 6, 2022

It looks kind of confusing that valuesOnly still creates key value pairs.

I agree, the option name could be better. Any suggestions? Maybe something like attachFieldsToBody: 'keyValues'?

Also is the for loop fast? I would do classical with Object.keys.

Will do!

And lastly does it not potentially needs protection against prototype pollution?

Good catch. Is it adequate to use Object.fromEntries(...)? Or perhaps there's a more standard/typical way to handle this in Fastify projects?

@zone117x
Copy link
Contributor Author

zone117x commented Jun 6, 2022

And lastly does it not potentially needs protection against prototype pollution?

On second look, I don't think additional changes are need for this. The request body is using the same set of keys, and only re-assigning the value. Prototype pollution should already be handled for these keys by the time this path is reached, right?

index.js Show resolved Hide resolved
@Uzlopak
Copy link
Contributor

Uzlopak commented Jun 7, 2022

I think it is now good.

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.

Documentation is required.

@zone117x
Copy link
Contributor Author

zone117x commented Jun 7, 2022

Documentation added for the new attachFieldsToBody: 'valueOnly' option, and also how it improves JSON Schema validation.

@zone117x zone117x requested a review from climba03003 June 7, 2022 19:38
README.md Outdated Show resolved Hide resolved
Co-authored-by: Igor Savin <iselwin@gmail.com>
@kibertoad kibertoad merged commit 7aa5b75 into fastify:master Jun 8, 2022
@kibertoad
Copy link
Member

Thank you!

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

5 participants