-
Notifications
You must be signed in to change notification settings - Fork 79
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
Add checks for parsing validate directive #3125
base: feature/server-side-validation
Are you sure you want to change the base?
Add checks for parsing validate directive #3125
Conversation
…rsing validate directive
packages/amplify-graphql-validate-transformer/src/__tests__/validators.test.ts
Outdated
Show resolved
Hide resolved
`, | ||
}, | ||
{ | ||
name: 'accepts length values of extremely large numbers beyond 64-bit range', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a surprising "pass" case to me. I assume that we will validate it because it parses into a number
, but it will fail at runtime because it exceeds Java's Long.MAX_VALUE
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me check what the maximum/minimum value our resolver can accept
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This falls under a rare use case. We will use isNaN(parseInt(x))
until a customer complains about is.
describe('Valid usage', () => { | ||
test.each([ | ||
{ | ||
name: 'accepts -Infinity value', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will also double check if Infinity is accepted in resolver
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This falls under a rare use case. We will use isNaN(parseFloat(x))
until a customer complains about is.
packages/amplify-graphql-validate-transformer/src/__tests__/numeric-validation-types.test.ts
Outdated
Show resolved
Hide resolved
packages/amplify-graphql-validate-transformer/src/__tests__/numeric-validation-types.test.ts
Outdated
Show resolved
Hide resolved
...plify-graphql-validate-transformer/src/__tests__/validation-field-type-compatibility.test.ts
Outdated
Show resolved
Hide resolved
* Validates that length validation values (minLength, maxLength) are valid non-negative integers. | ||
*/ | ||
const validateLengthValue = (config: ValidateDirectiveConfiguration): void => { | ||
const value = parseInt(config.value, 10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommend we use the Number()
constructor rather than parseInt
, since I assume we want to catch invalid constructions like "10z.0"
. parseInt("10z.0", 10)
returns 10
, but Number("10z.0")
returns NaN
, which I think is what we want?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Circling back to this conversation as I noticed that in default transformer, we do the following (link)
const validateInt = (x: string): boolean => !Number.isNaN(parseInt(x, 10));
const validateFloat = (x: string): boolean => !Number.isNaN(parseFloat(x));
This is currently the only place where we parse String
to Number
. I suggest that we keep numeric parsing consistent across both @default
and @validate
, so that it will not be a surprise to the customers that their parsing behaviors are different.
…sitive & negative floats
Description of changes
@validate
to be repeatable in both definition and snapshot testvalidators.ts
that checks for invalid use of validate directive, including:minLength
maxLength
is non-negative integer__tests__
correctlyjest.config.js
to ignoretypes.ts
which has no executable codes, and to use globally defined coverage thresholdDescription of how you validated changes
minLength
maxLength
is valid non-negative integersChecklist
yarn test
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.