openapi-to-graphql added stricter scalar type checks

I have been researching a number of ways to increase the strictness of graphql scalar types (which are loose by default - with no stricter checks) and I came unto a novel set of packages that do just this and I wanted to suggest these changes that will allow for stricter type/validation checks at runtime on a graphql server using the pattern and minimum, maximum and nullable attributes of the Schema Object.

May 09 '20 01:05 isocroft

@isocroft Wow! These are some seriously cool changes! I would like to get @ErikWittern opinion on these changes as well.

May 12 '20 05:05 Alan-Cha

@isocroft I think this is a really nice PR, thank you very much! I love the idea of having stricter checks for scalars, and also making use of the information about the nature values that could already be defined in an OpenAPI document.

I wanted to ask about the choice of https://github.com/joonhocho/graphql-scalar as a new dependency, though. There seem to be more popular GraphQL scalar libraries, which seem more actively maintained, like https://github.com/Urigo/graphql-scalars. Would they offer the same functionality required by this PR?

The current dependency also introduces a peerDependency for https://github.com/Microsoft/tslib. tslib itself has no dependencies, and comes from the makers of TypeScript, so I don't have many concerns about it. Just would like to understand the choice of https://github.com/joonhocho/graphql-scalar better.

May 12 '20 08:05 ErikWittern

@ErikWittern I was also a bit concerned about the popularity of this library and came across graphql-scalars. I do not think graphql-scalars can be used as a replacement, however. The purpose of this PR is to validate values against the schema. graphql-scalars doesn't provide the flexibility to do that. Instead, it just provides a list of predefined scalars.

~~I am also wondering if this PR is entirely necessary as I think we should assume that the OAS is true. I do not think our users expect us to validate the API itself. If the API does not follow the OAS exactly, I think we should just return the response to the best of our ability. Do we really want to throw an error? Do we really want to expose information about the GraphQL backend to users? Do not take this the wrong way though. I think this PR is really cool and I am simply wondering if it should be added as an option rather the default behavior.~~

Edit: Sorry for the previous comment. I misunderstood the PR to a degree.

May 12 '20 12:05 Alan-Cha

You are right @Alan-Cha . I couldn't work with graphql-scalars as it didn't fit. graphql-scalar is such a nice fit for this because of the flexibility and adaptability it brings for the use-case in this PR @ErikWittern . I had to go for a middle ground option. Please find updates to my PR here.

I am happy that you welcome these changes. I am of the opinion that GraphQL has a lot to benefit from the OpenAPI way to documenting endpoints and this package is well on its way to show that.

Also, thank you for such an awesome project!!

May 12 '20 23:05 isocroft

@isocroft Thank you for this awesome PR!!! It's always so nice to see people using and even trying to improve OtG! I was aware that there was a lot of data that we weren't taking advantage of in the OAS but I wasn't sure how we could. That's why I think this PR is super cool.

May 13 '20 13:05 Alan-Cha

@Alan-Cha @ErikWittern please re-review and merge when you are free. Thanks for everything

May 13 '20 17:05 isocroft

@isocroft Thanks for the update. I left a few comments in the code, mostly about formatting / comments.

Besides that, as @Alan-Cha also suggested, this PR should include tests. Not only to make sure it does what it intends to do, but also as another means of documenting the new functionality. Would you be willing to add them?

May 19 '20 15:05 ErikWittern

@isocroft: PR needs rebase.