added stricter scalar type checks #320

isocroft · 2020-05-09T01:31:43Z

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.

Signed-off-by: Ifeora Okechukwu <[email protected]>

Alan-Cha · 2020-05-12T05:41:56Z

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

packages/openapi-to-graphql/src/schema_builder.ts

ErikWittern · 2020-05-12T08:43:33Z

@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.

Alan-Cha · 2020-05-12T12:54:23Z

@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.

isocroft · 2020-05-12T23:41:38Z

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!!

…for input type Signed-off-by: Ifeora Okechukwu <[email protected]>

Signed-off-by: Ifeora Okechukwu <[email protected]>

Alan-Cha · 2020-05-13T13:52:15Z

@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.

Signed-off-by: Alan Cha <[email protected]>

…r provide default Signed-off-by: Ifeora Okechukwu <[email protected]>

isocroft · 2020-05-13T17:02:55Z

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

Alan-Cha

@ErikWittern and I reviewed this PR and in general, it looks really good and we are enthusiastic about the change, but we do have some questions.

In addition to the comments that I have left, is it possible to add some test cases? If not, I will happily look into adding them but because this is quite an extensive change, I do think we need test cases at some point.

Alan-Cha · 2020-05-15T12:53:13Z

packages/openapi-to-graphql/src/utils.ts

+
+export function isURL(s: string): boolean {
+ let res = null
+ const urlRegex = /(http(s)?:\/\/.)?(www\.)?[-a-zA-Z0-9@:%._\+~#=]{2,256}\.[a-z0-9]{2,6}\b([-a-zA-Z0-9@:%_\+.~#?&//=]*)/g


Is it possible to provide references to where these regexes are derived from?

The regex was gotten from a StackOverflow answer to a question andi took the regex and modified it and tested it properly using the famous Googler's URL Regex test samples - Mathias Bynens. It passed all the correct URL samples but failed a few of the incorrect URL samples. However, Deigo Perini's UL regex is the most successful albeit being very slow at runtime.

So, what do you think ?

@isocroft Sounds good, but let's add the link with the SO discussion in a comment on top of this function for reference.

@ErikWittern I have included the link

Alan-Cha · 2020-05-15T12:57:22Z

packages/openapi-to-graphql/src/utils.ts

+ */
+
+export function isEmail(s: string): boolean {
+ const emailRegex = /^[a-zA-Z0-9.!#$%&’*+/=?^_`{|}~-]+@[a-zA-Z0-9-]+(?:\.[a-zA-Z0-9-]+)*$/


Similar comment as above, it would be nice to know where these regexes come from.

They came from here - for EMAIL and here for UUID/GUID

@Alan-Cha the references to the regex definitions have been included as a comment

Alan-Cha · 2020-05-15T12:57:26Z

packages/openapi-to-graphql/src/utils.ts

+ * get the correct type of a variable
+ */
+export function strictTypeOf(value, type): boolean {
+ let result = false


In addition, for other check functions/helper functions, it would be nice to include a reference so we can easily learn more and also verify its completeness.

This function was created by me (I own the copyright - 😄😀) in 2015. The only references I have are the many codebases I have used it with resounding success. The function has also been tested in multiple instances and ways. Over the years, it has been re-written and updated for performance/correctness. It basically uses the variables' constructor name to determine what that type the variable is.

Below are example of its implementation:

const isURL = (variable) => strictTypeOf(variable, 'url') const isBlob = (variable) => strictTypeOf(variable, 'blob') const isSet = (variable) => strictTypeOf(variable, 'set') const isMap = (variable) => strictTypeOf(variable, 'map') const isBuffer = (variable) => strictTypeOf(variable, 'buffer') const isEmitter = (variable) => strictTypeOf(variable, 'eventemitter')

Also, I modified it to not inspect the prototype chain as such checks are not required for this project ( or are they ? could they be ? ). Here is the original version of the strictTypeOf implementation.

@isocroft I think it would be helpful to improve the documentation of this function. Both arguments are typed as any, but isn't at least the second supposed to be a string? The comment for the function states get the correct type of a variable. Could you elaborate on the context? It seems we are comparing a runtime value with what is defined in a JSON schema definition here, is that correct? I think it would also help to add some inline comments within this function - at least I am having issues following what it does.

@ErikWittern the version of the strictTypeOf function pasted above (see GitHub gist link) is not the same version used in the repo and in this PR. As it does a lot of things that is not needed for this project (e.g. looking up the prototype chain). Below is the slimmed-down version being used in the repo and in this PR - I have also added comments:

/** * check if a literal is falsy or not */ const isLiteralFalsey = (variable: unknown): boolean => { return variable === '' || variable === false || variable === 0 } /** * check if a variable contains a reference type (not a literal) */ const isPrimitive = (arg: unknown): boolean => { return typeof arg === 'object' || Boolean(arg) && typeof arg.apply === 'function' } /** * provide the name of primitive and/or reference types */ const checkTypeName = (target: unknown, type: string): boolean => { let typeName = '' // we need to separate checks for literal types and // primitive types so we can speed up performance and // keep things simple if (isLiteralFalsey(target) || !isPrimitive(target)) { // literal typeName = typeof target } else { // primitive/reference typeName = (Object.prototype.toString.call(target)).replace(/^\[object (.+)\]$/, '$1') } return Boolean(typeName.toLowerCase().indexOf(type) + 1) } /** * get the correct type of a variable */ export function strictTypeOf(value: unknown, type: string): boolean { // swagger/OpenAPI 'integer' type is converted // a JavaScript 'number' type for compatibility if (type === 'integer') { type = 'number' } type = type || '' return checkTypeName(value, type) }

examples:

// swagger/OPenAPI types: 'integer', 'string', 'array', 'object' strictTypeOf(variable, 'integer')

@ErikWittern No, that's not correct. The function isn't comparing a runtime value with what is defined in a JSON schema definition. It is checking that the data type of the variable is as intended/expected. I have added comments and references to this function for more clarity.

Alan-Cha · 2020-05-15T12:57:32Z

packages/openapi-to-graphql/src/types/strict_scalars.ts

+ config: TConfig
+}
+
+// may throw


Could you clarify the meaning of this comment?

This is an oversight. I will modify the comment to convey full meaning

@Alan-Cha I have reworded this comment

Alan-Cha · 2020-05-15T12:58:35Z

packages/openapi-to-graphql/src/utils.ts

+}
+
+/**
+ *


I see that there are a number of incomplete comments here. I can fill them in if you don't have the time.

I will fill them tomorrow morning 7:00a.m WAT

@Alan-Cha I have updated this comment

ErikWittern · 2020-05-19T15:05:31Z

packages/openapi-to-graphql/src/utils.ts

+/**
+ * get the correct type of a variable
+ */
+export function strictTypeOf(value: unknown, type: string): boolean {


@isocroft Maybe rename to isStrictTypeOf to indicate that the return value is boolean.

Also, I don't think the comment on top the function accurately explains what the function does. It seems to check whether the given value adheres to the type, which stems from a JSON schema from the OpenAPI, correct? Whereas the comment makes it seems as if the type (or at least the name of the type) of the given value is returned.

@ErikWittern I have resolved this by renaming the function and rewording the comments

ErikWittern · 2020-05-19T15:06:10Z

packages/openapi-to-graphql/src/utils.ts

+}
+
+/**
+ * get the correct type of a variable


@isocroft Minor: Can you capitalize the first word of comments to be in line with the rest of the codebase?

@ErikWittern I have done this

ErikWittern · 2020-05-19T15:06:42Z

packages/openapi-to-graphql/src/utils.ts

+/**
+ * check if a variable contains a reference type (not a literal)
+ */
+


@isocroft Minor: Please remove the empty line between the comment and the function it describes to be in line with the rest of the code base.

@ErikWittern I have sorted this

ErikWittern · 2020-05-19T15:08:32Z

packages/openapi-to-graphql/src/utils.ts

+/**
+ * provide the name of primitive and/or reference types
+ */
+const checkTypeName = (target: unknown, type: string): boolean => {


@isocroft Not sure the above comment describes what this function does accurately. It seems again to check that the given target (why not use "value" here?) is of the given type. Is that correct? The comment makes it seem as if a name of a primitive and/or type is returned.

@ErikWittern I have modified the comment to reflect the intent of the function.

ErikWittern · 2020-05-19T15:10:57Z

@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?

Signed-off-by: Ifeora Okechukwu <[email protected]>

ibm-ci-bot · 2021-07-15T17:49:06Z

@isocroft: PR needs rebase.