mercurius-js / validation

Adds configurable validation support to Mercurius.
MIT License
30 stars 6 forks source link

Customizable inferJSONSchemaType #29

Closed sooxt98 closed 7 months ago

sooxt98 commented 2 years ago

Make it able to add custom scalar inferring types, so far it only support 3 types

GraphQLString <=> { type: 'string' } GraphQLInt <=> { type: 'integer' } GraphQLFloat <=> { type: 'number' }

mcollina commented 2 years ago

Definitely!

Would you like to send a Pull Request to address this issue? Remember to add unit tests.

ramonmulia commented 2 years ago

I believe to add custom scalar inferring types in inferJSONSchemaType it would need to re-think the way it is building the argument schema in json-schema-validator https://github.com/mercurius-js/validation/blob/main/lib/validators/json-schema-validator.js#L104.

I found a library graphql-2-json-schema that could be used to transform GraphQL schema into jsonSchema instead of looping schema type in json-schema-validator, but still, it needs to include the validation in jsonSchema generated and change the way it is generating the final jsonSchema, so I'm not sure if it is a good idea.

There are some grahql scalar types that are not included in inferJSONSchemaType like GraphQLID and GraphQLBoolean but for custom ones I believe it will need to define a definitions property in jsonSchema where are all the references to be used into jsonSchema.

do you guys have another idea to deal with this issue?

mcollina commented 2 years ago

I believe to add custom scalar inferring types in inferJSONSchemaType it would need to re-think the way it is building the argument schema in json-schema-validator https://github.com/mercurius-js/validation/blob/main/lib/validators/json-schema-validator.js#L104.

How would you do that?

I found a library graphql-2-json-schema that could be used to transform GraphQL schema into jsonSchema instead of looping in each schema type in json-schema-validator but still needs to include the validation in the jsonSchema generated and change the way it is generating the final jsonSchema so I'm not sure if it is a good idea.

I don't think such a library is useful here. It looks like a different library.

There are some grahql scalar types that is not included in inferJSONSchemaType like GraphQLID and GraphQLBoolean but for custom ones I believe it will need to define a definitions property in jsonSchema where are all the references to be used into jsonSchema.

I don't think so. You can define the custom parameter every time.

simoneb commented 2 years ago

@ramonmulia what's the status of the work on this issue?

ramonmulia commented 2 years ago

I didn't have time to back on this.

jonnydgreen commented 2 years ago

@ramonmulia @sooxt98 would you still be interested in submitting a PR to address this? I'm very happy to help out if needed :)

ramonmulia commented 2 years ago

hey @jonnydgreen sure! I wrote my findings on top, but I believe I missed something, could you give me some input about this issue?

jonnydgreen commented 2 years ago

Sure thing, I'll have a look later tonight!

jonnydgreen commented 2 years ago

Hey @ramonmulia have you got an API and associated use-case(s) in mind for this change? This is a good place to start as the solution may change depending on this. Once this is all good, we can iterate on the implementation in a draft PR :)

I believe to add custom scalar inferring types in inferJSONSchemaType it would need to re-think the way it is building the argument schema in json-schema-validator https://github.com/mercurius-js/validation/blob/main/lib/validators/json-schema-validator.js#L104.

Could you provide more details about what you meant by this?

ramonmulia commented 2 years ago

hey @jonnydgreen , the function we use to infer JSONSchema is this one:

function inferJSONSchemaType (type, isNonNull) {
  if (type === GraphQLString) {
    return isNonNull ? { type: 'string' } : { type: ['string', 'null'] }
  }
  if (type === GraphQLInt) {
    return isNonNull ? { type: 'integer' } : { type: ['integer', 'null'] }
  }
  if (type === GraphQLFloat) {
    return isNonNull ? { type: 'number' } : { type: ['number', 'null'] }
  }
  return {}
}

to add custom types in JSONSchema we need to have a definition level that will be used as type reference $ref. currently this is not current behavior, for custom types they will end up with type: 'object' in JSONSchema.

So, maybe instead of using the function to build the JSONSchema https://github.com/mercurius-js/validation/blob/main/lib/validators/json-schema-validator.js?rgh-link-date=2022-03-18T11%3A13%3A43Z#L13

it could use this library graphql-2-json-schema to transform a graphqlSchema in JSONSchema and with it in hands we can apply the validation schema on it.

jonnydgreen commented 2 years ago

@ramonmulia thanks for the clarification about what you meant. By custom types, I presume you mean custom scalar types?

I don't think we need to replace with this library, we just need to adjust the code to add this support. Feel free to start a draft PR for this!

It's also worth noting that, per the title, this issue is also about making the inferJSONSchemaType customisable through the plugin options