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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose schema in request object #4149

Closed
2 tasks done
FlameWolf opened this issue Jul 22, 2022 · 13 comments 路 Fixed by #4216
Closed
2 tasks done

Expose schema in request object #4149

FlameWolf opened this issue Jul 22, 2022 · 13 comments 路 Fixed by #4216
Labels
feature request New feature to be added

Comments

@FlameWolf
Copy link

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the feature has not already been requested

馃殌 Feature Proposal

It would be great if we can access the schema from the request object so that a content parser can use it to correctly populate the request body instead of relying on hacks like you see in line 58 here.

Motivation

Need for having access to request schema for content parsing.

Example

https://github.com/FlameWolf/formzilla/blob/master/index.js

@Eomm
Copy link
Member

Eomm commented Jul 22, 2022

@Eomm Eomm closed this as completed Jul 22, 2022
@climba03003
Copy link
Member

climba03003 commented Jul 22, 2022

Since yesterday you can: https://www.fastify.io/docs/latest/Reference/Request/#getvalidationfunction

It is not the same request.

The request here is retrieve the route options, more particular schema for custom usage.

@climba03003 climba03003 reopened this Jul 22, 2022
@climba03003 climba03003 added the feature request New feature to be added label Jul 22, 2022
@mcollina
Copy link
Member

The schema is part of the route context:

schema,

@climba03003
Copy link
Member

climba03003 commented Jul 22, 2022

The schema is part of the route context:

Yes, it is. However, the document stated that context is for fastify internal usage.
Users should not use or modify it directly.

We need to deprecate the context properly and expose some useful inside context in public API.

@metcoder95
Copy link
Member

Not so sure if deprecate it will be the best approach, but at least expose some public API where the users can take use to understand the route settings

@climba03003
Copy link
Member

climba03003 commented Jul 22, 2022

Not so sure if deprecate it will be the best approach

If deprecation or removal is not possible.
I would

  1. refactor to a symbol property.
  2. expose the same context with Proxy to provide readonly usage.

In this case, user is safe to use context and wouldn't worry about accidental changes.

@mcollina
Copy link
Member

Allowing unvetted access to context enables quite a few plugins (even in our orgs). This was an escape-hatch to enable quick evolution of the framework for with flexibility.

I think we can design a new API to a address those cases. I would probably consider it semver-major and be done with it.

@Eomm
Copy link
Member

Eomm commented Jul 22, 2022

Meanwhile, the user can write:

fastify.addSchema({ $id: 'foo' ... })
fastify.get('/',
  { schema: fastify.getSchema('foo') }, 
  () => { const schema = fastify.getSchema('foo') })

@metcoder95
Copy link
Member

To recap, the agreed solution will be:

  1. Hide the context object through a symbol
  2. Create a new API that exposes such route internals that can be used by implementation and plugins
    • Here will be worth it to decide what we want to expose and what not

I think we can safely land the new API as a minor by exposing it but not hiding the context object through a symbol yet, just marking any not desired access to it as with a deprecation warning (not so sure if we can track all and distinguish but I'll try to make some assessment).

And leverage the context behind a symbol for the major.

Please let me know if my assumptions are correct, otherwise, let me know if I'm missing something 馃檪

@mcollina
Copy link
Member

Based on past experiences, I'd restrain in landing this change in a minor. This can have unpredictable consequences and unexpected performance drops.

There is no hurry to implement this so we can safely do it for the next major.

The PR can land in a next branch.

@metcoder95
Copy link
Member

Perfect, sounds good. Then everything remains the same but with the exception of aiming for next major 馃憤

@metcoder95
Copy link
Member

Is there a new next branch prepared for pointing the PR?

@mcollina
Copy link
Member

mcollina commented Aug 19, 2022

There is a next branch now

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

Successfully merging a pull request may close this issue.

5 participants