-
Notifications
You must be signed in to change notification settings - Fork 35
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
Include "required" and "additionalProperties" keys to the object definitions #13
Comments
@valentinpalkovic I saw your comment on the commit that removed the required functionality that you had implemented earlier. I have a suggested approach to address your concern. I use Organization Model
Schema should look like this when
|
In principle a good idea. But you only treat the cases of creating or editing a new entity from the frontend. How should the schema validation looks like, if the user wants to fetch an entity with some specific fields (Graphql case). The frontend client can fetch any fields and does not have to fetch all of them at once. Therefore marking fields as required might only be true for (in REST language) PUT/POST requests. Therefore it is impossible at build/compile time to know, which fields are actually fetched by the client. Secondly, you cannot be sure, which fields have to be sent by the client and which can be delivered by other microservices or even be extracted from the user session information (information saved in JWT). The Prisma schema doesn't answer these types of questions and I have the feeling that your provided solution would cover one small use-case out of a bunch of possible use-cases. Also, in the use-case of a backend-for-frontend solution, where most of the business logic lays in the backend, additional fields, which are not persisted in the database, might be sent to the frontend (persisted ui states, session related information,...). Marking |
I use graphql not rest api. In my opinion, all cases will be covered by setting The current generated schema is of no use as it will pass any object
How do you propose to use the current state of generated schema for input validation in the resolver? |
To be a bit more specific to my previous comment (because you haven't answered my concerns yet):
query getOrganization() {
organization {
id
address
name
}
}
query getOrganization() {
organization {
id
address
name
someComputedValueWhichIsNotSavedIntoDatabase
}
}
mutation deleteOrganization($input: DeleteOrganizationInput) {
id
} Your proposed solution: "And when id or cuid is not null then it will be treated as an update request, hence uses anyOf to enforce at least one of the fields to be present for updation." wouldn't work. But maybe the JSON schema validation is only used for client-data validation. In this case, my concerns mentioned in 1. and 2. don't matter. I just don't know currently, how the schema is used to validate data. Maybe you could provide some examples and use-cases which could show the usage of the generated JSON schema. |
Currently, that's true. Only known fields are validated and checked, whether the right type is passed or not. Unknown fields or empty objects are not validated. |
Point 1. The solution can be enhanced to include all the fields in the Point 2. Is addressed by setting Point 3. Why anyone would like to do an explicit schema validation for delete mutation. The |
Here's my solution to this problem, a simple utility function: export const requireSchema = <T>(
schema: { type: string; properties: T },
required: (keyof T)[]
) => {
return { ...schema, required }
} It takes a schema definition with a list of required params. Here's an example using it with fastify: server.route<{ Body: Lead }>({
method: "POST",
url: "/leads",
schema: {
body: requireSchema(schema.definitions.Lead, [
"first_name",
"last_name",
"email",
]),
},
handler: async (req, reply) => {
// ...
},
}) This allows to enforce which fields are necessary on a route-by-route basis instead of just based on the database. Since it makes use of typescript generics, the array of required fields is validated against the schema, so its type-safe. With this method you could make the mistake of leaving out a field required by the database, but at that point Prisma will throw an error anyway. For me this is a decent middle-ground. |
This library isn't usable without required |
I think we have to talk about the abilities of the Prisma schema and its main purpose. Per docs, the Prisma schema consists of three parts:
The Prisma model definitions "represent the entities of your application domain, map to the tables (relational databases like PostgreSQL) or collections (MongoDB) in your database, form the foundation of the queries available in the generated Prisma Client API and when used with TypeScript, Prisma Client provides generated type definitions for your models and any variations of them to make database access entirely type-safe." - https://www.prisma.io/docs/concepts/components/prisma-schema/data-model By only parsing the Prisma model, it is not possible to figure out, which endpoints will be created, whether it is a GraphQL endpoint or a plain REST endpoint. The actual code of your backend defines, which data the user has to send to create, read, update or delete data. Already mentioned techniques like computed fields makes things even more unpredictable. It would be necessary to parse your endpoints to create JSON schema definitions for each of them. As @auderer already mentioned, you need validation on a route-by-route base, instead of purely relying on the type definition of the database models. The generated JSON schema can help you, but currently, it's still your responsibility to set the "required" flag on a route-by-route base. Let's take a look, what would happen if all the fields would be marked as required: ReadDepending on where the schema validation happens, it can cause huge problems, if all the fields are marked as required (except the fields which have the attributes UpdateUpdating a model can happen in a lot of different ways. The client might send only a subset of data (patch, a partial update) or it has to update all of the fields at once (put operation). The Prisma model doesn't define this behaviour, but your programming code does. It's impossible to guess, whether all the fields are required or only a subset without analyzing your endpoints. CreateCreating a new entry in your database requires all the field information for a model. But it doesn't mean, that all the defined model fields have to be sent by the client. Maybe some information is aggregated or fetched from a different source. This means, that the client might only send a subset of the needed information and the rest is calculated/aggregated/pulled from another source. As you can see, it is not possible to just mark all the fields as required, because a couple of cases are not covered. @ShivamJoker Nevertheless, if you have some smart ideas to tackle the mentioned issues, I would be happy to continue the discussion and maybe find a way of supporting your needed use case. Maybe you can start by describing your issue in detail with some examples to continue the discussion upon it. |
Feature Request
Include "required" and "additionalProperties" keys to the object definitions. The field list of "required" key should be as per the prisma model definition. The "required" is very much needed as the default behaviour of json schema specs is to allow empty objects to pass the validation.
For example:
The text was updated successfully, but these errors were encountered: