Skip to content
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 assertions and type safety after #61 #69

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

danielfinke
Copy link
Contributor

Issue #, if available: relates to #61 (see #61 (comment))

Description of changes:

Also fix mistyped return annotation on resolveGraphDBQuery, my bad

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Also fix mistyped return annotation on `resolveGraphDBQuery`, my bad
@@ -9,7 +9,5 @@
"--create-update-aws-pipeline-neptune-endpoint", "<AIR_ROUTES_DB_HOST>:<AIR_ROUTES_DB_PORT>",
"--output-resolver-query-https"],
"host": "<AIR_ROUTES_DB_HOST>",
"port": "<AIR_ROUTES_DB_PORT>",
"testOutputFilesSize": ["output.resolver.graphql.js", "output.schema.graphql", "output.source.schema.graphql"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you're cleaning up test files with unused fields can you also do the same for Case03/case.json?

@@ -27,12 +28,17 @@ const schema = buildASTSchema(schemaDataModel, { assumeValidSDL: true });
export function resolveGraphDBQueryFromAppSyncEvent(event) {
const fieldDef = getFieldDef(event.field);

assert(fieldDef);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For troubleshooting would be nice to add a value for the optional message parameter to indicate what expected state was not satisfied ie. Field definition for ${event.field} not found.

const value = event.arguments[inputDef.name.value];

if (value) {
const inputType = typeFromAST(schema, inputDef.type);

assert(isInputType(inputType));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For troubleshooting would be nice to add a value for the optional message parameter to indicate what expected state was not satisfied ie. Type ${inputType} is not a valid input type.

for (const rootDef of rootTypeDefs) {
const fieldDef = rootDef.fields.find(def => def.name.value === fieldName);
for (const rootTypeDef of rootTypeDefs) {
const fieldDef = rootTypeDef.fields?.find(def => def.name.value === fieldName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the Case03 which is not currently asserting any validation after the utility is executed can be repurposed to verify error handling for scenarios addressed in this PR (app sync event fields do not match up with the graphQL schema). This could be done by using similar approach to your Case01.05.test.js tests except modifying the app sync event to reference unknown types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants