Gql preprocess #20

marcvberg · 2022-05-06T00:37:10Z

Description of changes

This begins to add support for transformer plugins to modify schemas outside the transformation process. Enables codegen to hook into these changes directly and use them

This is only for the purpose of adding support in right now and does not hook into any active workflows

Issue #, if available

Description of how you validated changes

Unit testing

Checklist

PR description included
yarn test passes
Tests are changed or added

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

lgtm-com · 2022-05-06T00:47:00Z

This pull request introduces 6 alerts when merging c2961fa into 12e0be7 - view on LGTM.com

new alerts:

6 for Unused variable, import, function or class

lgtm-com · 2022-05-25T14:52:16Z

This pull request introduces 2 alerts when merging bcb29b9 into a9bc64c - view on LGTM.com

new alerts:

2 for Unused variable, import, function or class

lgtm-com · 2022-05-25T19:02:52Z

This pull request introduces 2 alerts when merging fd1ada7 into 0b795c9 - view on LGTM.com

new alerts:

2 for Unused variable, import, function or class

packages/amplify-data-core/src/amplify-data-api.ts

packages/amplify-graphql-transformer-core/src/transformation/transform.ts

lgtm-com · 2022-05-26T20:37:54Z

This pull request introduces 3 alerts when merging d64c9c2 into b1ceecd - view on LGTM.com

new alerts:

3 for Unused variable, import, function or class

lgtm-com · 2022-05-27T14:02:12Z

This pull request introduces 4 alerts when merging 201eacc into 5a1b0b6 - view on LGTM.com

new alerts:

4 for Unused variable, import, function or class

lgtm-com · 2022-05-27T14:27:58Z

This pull request introduces 3 alerts when merging 6ed42b9 into 5a1b0b6 - view on LGTM.com

new alerts:

3 for Unused variable, import, function or class

lgtm-com · 2022-05-27T15:18:38Z

This pull request introduces 3 alerts when merging 09255fa into 5a1b0b6 - view on LGTM.com

new alerts:

3 for Unused variable, import, function or class

lgtm-com · 2022-06-02T16:37:49Z

This pull request introduces 3 alerts when merging 0e017e2 into 390bfd0 - view on LGTM.com

new alerts:

3 for Unused variable, import, function or class

sundersc · 2022-06-08T22:38:39Z

...ify-graphql-relational-transformer/src/__tests__/amplify-graphql-has-one-transformer.test.ts

+    doc?.definitions?.forEach(def => {
+      if ((def.kind === 'ObjectTypeDefinition' || def.kind === 'ObjectTypeExtension') && def.name.value === objectType) {
+        def?.fields?.forEach(field => {
+          if (field.name.value === fieldName) {
+            field?.directives?.forEach(dir => {
+              if (dir.name.value === 'hasOne') {
+                dir?.arguments?.forEach(arg => {
+                  if (arg.name.value === 'fields') {
+                    if (arg.value.kind === 'ListValue' && arg.value.values[0].kind === 'StringValue' && arg.value.values[0].value === generatedFieldName) {
+                      hasFieldArgument = true;
+                    }
+                    else if (arg.value.kind === 'StringValue' && arg.value.value === generatedFieldName) {
+                      hasFieldArgument = true;
+                    }
+                  }
+                });
+              }
+            });
+          }


nit: Is it possible to reduce the nesting here using filter and some?
May be like doc.definitions.filter(def => (def.kind === '' and so on) && def.fields.some(f => f.name.value === fieldName)).

sundersc · 2022-06-08T22:41:52Z

packages/amplify-graphql-relational-transformer/src/graphql-belongs-to-transformer.ts

+        if (def.kind === 'ObjectTypeExtension' || def.kind === 'ObjectTypeDefinition') {
+          def?.fields?.forEach(field => {
+            field?.directives?.forEach(dir => {
+              if (dir.name.value === directiveName) {
+                const relatedType = objectTypeMap.get(getBaseType(field.type));
+                if (relatedType) { // Validation is done in a different segment of the life cycle
+                  const relationTypeField = relatedType?.fields?.find(relatedField => {
+                    if (getBaseType(relatedField.type) === def.name.value && relatedField?.directives?.some(
+                      relatedDir => relatedDir.name.value === 'hasOne' || relatedDir.name.value === 'hasMany',
+                    )) {
+                      return true;
+                    }
+                    return false;
+                  });
+                  const relationTypeName = relationTypeField?.directives?.find(relationDir => relationDir.name.value === 'hasOne' || relationDir.name.value === 'hasMany')?.name?.value;
+                  if (relationTypeName === 'hasOne') {
+                    const connectionAttributeName = getConnectionAttributeName(def.name.value, field.name.value);
+                    if (!def?.fields?.some(defField => defField.name.value === connectionAttributeName)) {
+                      def?.fields?.push(
+                        makeField(
+                          connectionAttributeName, [], isNonNullType(field.type) ?
+                            makeNonNullType(makeNamedType('ID')) : makeNamedType('ID'), [],
+                        ) as WritableDraft<FieldDefinitionNode>,
+                      );
+                    }
+                  }
+                }
+              }
+            });
+          });


Same as the previous comment. Please see if nesting can be reduced using filter and some.

sundersc · 2022-06-08T22:43:20Z

packages/amplify-graphql-relational-transformer/src/graphql-has-many-transformer.ts

+      draftDoc?.definitions?.forEach(def => {
+        if (def.kind === 'ObjectTypeExtension' || def.kind === 'ObjectTypeDefinition') {
+          def?.fields?.forEach(field => {
+            field?.directives?.forEach(dir => {
+              if (dir.name.value === directiveName) {
+                const baseFieldType = getBaseType(field.type);
+                const connectionAttributeName = getConnectionAttributeName(def.name.value, field.name.value);
+                const newField = makeField(connectionAttributeName, [], isNonNullType(field.type) ? makeNonNullType(makeNamedType('ID')) : makeNamedType('ID'), []);
+                connectingFieldsMap.set(baseFieldType, newField as WritableDraft<FieldDefinitionNode>);
+              }
+            });
+          });
+        }
+      });


Can we use filter instead of foreach and if?

sundersc · 2022-06-08T22:44:36Z

packages/amplify-graphql-relational-transformer/src/graphql-has-one-transformer.ts

+      draftDoc.definitions.forEach(def => {
+        if (def.kind === 'ObjectTypeDefinition' || def.kind === 'ObjectTypeExtension') {
+          def?.fields?.forEach(field => {
+            field?.directives?.forEach(dir => {
+              if (dir.name.value === directiveName) {
+                const connectionAttributeName = getConnectionAttributeName(def.name.value, field.name.value);
+                let hasFieldsDefined = false;
+                let removalIndex = -1;
+                dir?.arguments?.forEach((arg, idx) => {
+                  if (arg.name.value === 'fields') {
+                    if ((arg.value.kind === 'StringValue' && arg.value.value) || (arg.value.kind === 'ListValue' && arg.value.values && arg.value.values.length > 0)) {
+                      hasFieldsDefined = true;
+                    } else {
+                      removalIndex = idx;
+                    }
+                  }
+                });
+                if (removalIndex !== -1) {
+                  dir?.arguments?.splice(removalIndex, 1);
+                }
+                if (!hasFieldsDefined) {
+                  // eslint-disable-next-line no-param-reassign
+                  dir.arguments = [makeArgument('fields', makeValueNode(connectionAttributeName)) as WritableDraft<ArgumentNode>];
+                  def?.fields?.push(
+                    makeField(
+                      connectionAttributeName, [], isNonNullType(field.type) ?
+                        makeNonNullType(makeNamedType('ID')) : makeNamedType('ID'), [],
+                    ) as WritableDraft<FieldDefinitionNode>,
+                  );
+                }
+              }
+            });


Same as the previous comment. Please see if we can use filter to simply the logic.

sundersc · 2022-06-08T22:45:31Z

packages/amplify-graphql-relational-transformer/src/graphql-many-to-many-transformer.ts

+      draftDoc?.definitions?.forEach(def => {
+        if (def.kind === 'ObjectTypeDefinition' || def.kind === 'ObjectTypeExtension') {
+          def?.fields?.forEach(field => {
+            field?.directives?.forEach(dir => {
+              if (dir.name.value === directiveName) {
+                const relationArg = dir?.arguments?.find(arg => arg.name.value === 'relationName');
+                if (relationArg?.value?.kind === 'StringValue') {
+                  const relationName = relationArg.value.value;
+                  const manyToManyContext: ManyToManyPreProcessContext = {
+                    model: def,
+                    field: field,
+                    directive: dir,
+                    modelAuthDirectives: def?.directives?.filter(authDir => authDir.name.value === 'auth') ?? [],
+                    fieldAuthDirectives: def?.directives?.filter(authDir => authDir.name.value === 'auth') ?? [],
+                    relationName: relationName,
+                  };
+                  if (!manyToManyMap.has(relationName)) {
+                    manyToManyMap.set(relationName, []);
+                  }
+                  manyToManyMap.get(relationName)?.push(manyToManyContext);
+                }
+              }
+            });
+          });
+        }
+      });


Consider use of filter.

packages/amplify-graphql-relational-transformer/src/schema.ts

sundersc

Left few comments to check whether the nested foreach and if statements can be avoided using filter method. Otherwise the changes look good.

lgtm-com · 2022-06-10T14:12:19Z

This pull request introduces 1 alert when merging 37fe785 into eee67ed - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

sundersc

LGTM

alharris-at · 2022-06-13T15:48:33Z

packages/amplify-graphql-relational-transformer/package.json

@@ -36,7 +36,10 @@
    "@aws-cdk/core": "~1.124.0",
    "graphql": "^14.5.8",
    "graphql-mapping-template": "4.20.5",
-    "graphql-transformer-common": "4.23.2"
+    "graphql-transformer-common": "4.23.0",


this line is a dup of 41

lgtm-com · 2022-06-13T16:08:05Z

This pull request introduces 1 alert when merging 544eb3e into 5a8c75a - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

marcvberg added 2 commits May 5, 2022 20:32

WIP: Adding preProcess support to V2 transformer

8f9fc88

Resolve merge conflict: two added tests

c2961fa

marcvberg added 4 commits May 9, 2022 19:38

Feature: Add preprocessing to belongsTo plugin

9bcb494

Feat: adding preprocessing support manyToMany

c1bea31

Remove DataAPI, we will use a different single source read

6df2b71

Merge branch 'main' into gql-preprocess

bcb29b9

marcvberg marked this pull request as ready for review May 25, 2022 14:41

marcvberg requested a review from a team May 25, 2022 14:41

marcvberg requested a review from a team as a code owner May 25, 2022 14:41

Tests: add manyToMany preprocess tests

fd1ada7

alharris-at reviewed May 26, 2022

View reviewed changes

Fix: Add test and corrct field type renaming manyToMany

d64c9c2

Fix: make schema mutate more functional

201eacc

Fix: remove use of let

6ed42b9

Docs: update docstring for preprocessing with more detail

09255fa

Merge branch 'main' into gql-preprocess

0e017e2

alharris-at previously approved these changes Jun 2, 2022

View reviewed changes

sundersc reviewed Jun 8, 2022

View reviewed changes

packages/amplify-graphql-relational-transformer/src/schema.ts Outdated Show resolved Hide resolved

sundersc reviewed Jun 8, 2022

View reviewed changes

packages/amplify-graphql-relational-transformer/src/schema.ts Outdated Show resolved Hide resolved

sundersc reviewed Jun 8, 2022

View reviewed changes

Cut down on nesting in pre-processing

2646fc1

marcvberg dismissed alharris-at’s stale review via 2646fc1 June 10, 2022 03:38

marcvberg added 2 commits June 9, 2022 23:40

Add some semicolons in that should've been there

b623e0a

Merge branch 'main' into gql-preprocess

37fe785

sundersc previously approved these changes Jun 11, 2022

View reviewed changes

alharris-at suggested changes Jun 13, 2022

View reviewed changes

Fix extra package dependencies from merge

544eb3e

marcvberg dismissed sundersc’s stale review via 544eb3e June 13, 2022 15:57

alharris-at approved these changes Jun 13, 2022

View reviewed changes

marcvberg merged commit 15860fc into aws-amplify:main Jun 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gql preprocess #20

Gql preprocess #20

marcvberg commented May 6, 2022

lgtm-com bot commented May 6, 2022

lgtm-com bot commented May 25, 2022

lgtm-com bot commented May 25, 2022

lgtm-com bot commented May 26, 2022

lgtm-com bot commented May 27, 2022

lgtm-com bot commented May 27, 2022

lgtm-com bot commented May 27, 2022

lgtm-com bot commented Jun 2, 2022

sundersc Jun 8, 2022 •

edited

Loading

sundersc Jun 8, 2022 •

edited

Loading

sundersc Jun 8, 2022

sundersc Jun 8, 2022

sundersc Jun 8, 2022

sundersc left a comment

lgtm-com bot commented Jun 10, 2022

sundersc left a comment

alharris-at Jun 13, 2022

lgtm-com bot commented Jun 13, 2022

Gql preprocess #20

Gql preprocess #20

Conversation

marcvberg commented May 6, 2022

Description of changes

Issue #, if available

Description of how you validated changes

Checklist

lgtm-com bot commented May 6, 2022

lgtm-com bot commented May 25, 2022

lgtm-com bot commented May 25, 2022

lgtm-com bot commented May 26, 2022

lgtm-com bot commented May 27, 2022

lgtm-com bot commented May 27, 2022

lgtm-com bot commented May 27, 2022

lgtm-com bot commented Jun 2, 2022

sundersc Jun 8, 2022 • edited Loading

Choose a reason for hiding this comment

sundersc Jun 8, 2022 • edited Loading

Choose a reason for hiding this comment

sundersc Jun 8, 2022

Choose a reason for hiding this comment

sundersc Jun 8, 2022

Choose a reason for hiding this comment

sundersc Jun 8, 2022

Choose a reason for hiding this comment

sundersc left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Jun 10, 2022

sundersc left a comment

Choose a reason for hiding this comment

alharris-at Jun 13, 2022

Choose a reason for hiding this comment

lgtm-com bot commented Jun 13, 2022

sundersc Jun 8, 2022 •

edited

Loading

sundersc Jun 8, 2022 •

edited

Loading