feat: add error when using an owner field as a sort key field #517

danielleadams · 2022-06-06T22:37:08Z

Description of changes

As discussed, there's a breaking change when using owner @auth field as the @primaryKey's sort key. Therefore, the transformer will raise an error and abort the transform process when using the sub::username identity claim in which the owner attribute in the database is being used as the sort key.

Docs PR to follow.

Issue #, if available

Description of how you validated changes

Checklist

PR description included
yarn test passes
Tests are changed or added
Relevant documentation is changed or added (and PR referenced)

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

sundersc · 2022-06-07T00:36:40Z

packages/amplify-graphql-index-transformer/src/graphql-primary-key-transformer.ts

+    if (!validateNotOwnerAuth(sortKeyFieldName, config, ctx)) {
+      throw new InvalidDirectiveError(
+        `The primary key's sort key type '${sortKeyFieldName}' cannot be used as an owner @auth field too. Please user another field for the sort key.`
+      );
+    }
+


Could adding this validation now affect any existing customer schema?

@sundersc no - the feature flag has to be enabled OR they have to be using the new feature (which is behind a feature flag) to see this (https://github.com/aws-amplify/amplify-category-api/pull/517/files#diff-5f8c05424d524274f208c275f81ac9d5b2f6fbf6a878f0bb1daa27f65f4f4289R101). Also, this will go out with a semver major, so anyone already using the feature and using the owner as a sort key will know this is a breaking change.

sachscode

LGTM, just a few comments

sachscode · 2022-06-07T00:58:07Z

packages/amplify-graphql-index-transformer/src/utils.ts

+
+  if (!authDir) return true;
+
+  const dirRules = (authDir.arguments?.find(arg => arg.name.value === 'rules')?.value as ListValueNode).values;


nit: can we ensure that dirRules is non-null before the ownerRules is initialized.

packages/amplify-graphql-index-transformer/src/utils.ts

danielleadams · 2022-06-07T16:52:45Z

@sachscode @sundersc this is ready to be re-reviewed/approved.

sachscode

LGTM

danielleadams · 2022-06-08T20:40:57Z

(Just rebased, so ready for rereview)

danielleadams · 2022-06-08T23:16:09Z

packages/amplify-graphql-auth-transformer/src/__tests__/owner-auth.test.ts

+        "The primary key's sort key type 'myOwnerField' cannot be used as an owner @auth field too. Please use another field for the sort key.",
+      );
+    });
+  });


Originally I had put these tests in the index transformer because the validation seemed to fit better in the sort key validations that run. There was a problem with circular dependencies though during the build, so I moved these tests to the auth transformer to prevent the circular dependency (I believe it was index transformer -> auth -> relational -> index). I can create a follow up task to organize these tests within their module, or move them to the velocity tests.

alharris-at · 2022-06-09T00:08:26Z

packages/amplify-graphql-index-transformer/src/utils.ts

+      }
+    });
+
+    const featureFlagEnabled = ctx.featureFlags.getBoolean('useSubUsernameForDefaultIdentityClaim');


Do we want a default value here set to false?

If this value isn't set in the project, .getBoolean will return the default value as configured between "existing" and "new" Amplify apps. (https://github.com/aws-amplify/amplify-cli/blob/master/packages/amplify-cli-core/src/feature-flags/featureFlags.ts#L218)

(Also, the feature flags module always errors out if the feature flag is missing from the instance because it acts as a singleton, so it always needs to be set in tests too.)

alharris-at · 2022-06-09T00:20:23Z

packages/amplify-graphql-index-transformer/src/utils.ts

+
+    (rule as ObjectValueNode).fields?.forEach(field => {
+      const name: string = field.name.value;
+      const { value } = field?.value as StringValueNode;


Hmm, I'm wondering if this can throw an npe if value is not defined? As written it seems like you want this to kind of behave like field?.value?.value, but I think that w/ the destructing and typecasting here it's more like field?.value!!.value.

If this is really desireable we may need to do (field?.value as (StringValueNode | undefined))?.value;

I'll change this to set value as a string and have handle undefined.

alharris-at · 2022-06-09T00:23:11Z

packages/amplify-graphql-index-transformer/src/utils.ts

+  if (!authDir) return true;
+
+  const dirRules = (authDir.arguments?.find(arg => arg.name.value === 'rules')?.value as ListValueNode)?.values || [];
+  const hasOwnerFieldAsSortKey = dirRules.some(rule => {


I might pull this rule validation out into it's own helper for readability, then it's clear that you're just reducing that state, i.e return dirRules.some(ruleHasOwnerFieldAsSortKey);

alharris-at · 2022-06-09T00:28:23Z

packages/amplify-graphql-index-transformer/src/utils.ts

+    let isOwner = false;
+    let identityClaimIsSet = false;
+    let ownerFieldIsSet = false;
+    let usesMultiClaim = false;
+    let sortKeyFieldIsAuthField = false;


I'll OOS for this PR, since this isn't yet an established pattern, but I prefer to avoid let wherever possible, and rely instead on returning consistent shapes, and reducing state where necessary. In general for something like this I'd probably refactor to have your inner loop (i.e. rule.fields?.forEach turn into a rule.fields.map(doesRuleHaveOwnerForSortKeyField), then just return a .some for that mapped value. As written, I'm not sure if it's possible to have sortKeyFieldIsAuthField be set to true for a single rule, and usesImplicitOwnerField for another unrelated rule, and result in a false positive here.

As written, there's some chance for state interaction between rules, which I don't think is by design. If we need rules to 'see' each other, we should make that super explicit, and either preprocess the necessary cross-rule state, or otherwise allow the rule checker to look at it's neighbors rather than relying on global state.

I'm fine with that. This was the best way I could configure for my own readability since the GQL objects can be messy to parse (I always wanted to work on some GQL adapters to integrate the code into the transformers), but I can change the pattern. I'll refactor this.

sachscode

LGTM

sundersc · 2022-06-10T20:21:57Z

packages/amplify-graphql-relational-transformer/src/__tests__/test-helpers.ts

    if (value === 'useSubUsernameForDefaultIdentityClaim') {
      return true;
    }


Can we move the default values to a new hash map and use that here? In that way, if we have to check for any new flags in the future, we don't have to add multiple if statements in the method.

We can potentially OOS for this change. I actually added a simple createFeatureFlagProviderWithOverrides util in another PR that can just become our standard to simplify behavior and keep us from reimplementing on a per-test basis. Your call if you think it's blocking or not.

I agree with this in theory and am happy to follow up, but since this isn't an established pattern (and there's a mix of using these mock objects in the current tests) is it okay if consolidating the tests is a follow up? If this is blocking I can fix it, but the reason I had to add this feature flag to each test is because this is how the other tests do it.

sundersc · 2022-06-10T20:24:56Z

packages/amplify-graphql-schema-test-library/src/__tests__/index.test.ts

+    getBoolean: (value: string, defaultValue: boolean): boolean => {
+      if (value === 'useSubUsernameForDefaultIdentityClaim') {
+        return true;
+      }
+      return defaultValue;
+    },


Same comment as the previous. If possible, please move this to a hash map.

sundersc · 2022-06-10T20:26:22Z

packages/graphql-transformers-e2e-tests/src/__tests__/IndexWithAuthV2WithFF.e2e.test.ts

      $parent: ID
-      $child: ModelIDKeyConditionInput


Is there a reason to remove this field from an existing test?

Yes, $child is no longer part of the primary key's query.

…in mock

* chore: upgrade api category dependencies * chore: reflect test updates from aws-amplify/amplify-category-api#517 in mock

danielleadams requested a review from a team June 6, 2022 22:37

sundersc reviewed Jun 7, 2022

View reviewed changes

sachscode previously approved these changes Jun 7, 2022

View reviewed changes

danielleadams dismissed sachscode’s stale review via 129086d June 7, 2022 13:26

danielleadams force-pushed the raise-error-with-owner-auth-and-sort-key branch from 0eac49c to 5728ec4 Compare June 7, 2022 16:28

sachscode previously approved these changes Jun 7, 2022

View reviewed changes

danielleadams dismissed sachscode’s stale review via e9821bf June 7, 2022 18:09

danielleadams mentioned this pull request Jun 8, 2022

test: update listX tests to account for compile error #522

Closed

5 tasks

danielleadams added 7 commits June 8, 2022 16:30

feat: add error when using an owner field as a sort key field

e668f8f

test: move auth dep into devDependencies

d50b55c

refactor: refactor validation function for sort key

c14d567

chore: update yarn.lock file

0afa394

test: move unit tests to auth transformer

dafa5ab

test: fix relational test

8298f8c

test: fix velocity auth tests

a66a7ce

danielleadams force-pushed the raise-error-with-owner-auth-and-sort-key branch from ac1b8e8 to a66a7ce Compare June 8, 2022 20:35

test: update listX tests to account for compile error

8eb758b

danielleadams force-pushed the raise-error-with-owner-auth-and-sort-key branch from 8ea0dd5 to 8eb758b Compare June 8, 2022 23:09

danielleadams commented Jun 8, 2022

View reviewed changes

alharris-at reviewed Jun 9, 2022

View reviewed changes

danielleadams mentioned this pull request Jun 9, 2022

doc: add section about breaking @primaryKey/@auth changes aws-amplify/docs#4377

Closed

danielleadams added 2 commits June 9, 2022 15:44

test: fix unit test when feature flag is off

c57bb03

refactor: separate out methods for finding owner fields in auth rules

bc9f2a0

alharris-at previously approved these changes Jun 9, 2022

View reviewed changes

sachscode previously approved these changes Jun 10, 2022

View reviewed changes

test: add subusername feature flag to transformer unit tests

fe018a2

danielleadams dismissed stale reviews from sachscode and alharris-at via fe018a2 June 10, 2022 19:13

sundersc reviewed Jun 10, 2022

View reviewed changes

alharris-at approved these changes Jun 10, 2022

View reviewed changes

sundersc approved these changes Jun 10, 2022

View reviewed changes

alharris-at merged commit 201032d into aws-amplify:main Jun 11, 2022

danielleadams mentioned this pull request Jun 11, 2022

doc: add section about breaking @primaryKey/@auth changes aws-amplify/docs#4387

Closed

danielleadams deleted the raise-error-with-owner-auth-and-sort-key branch June 12, 2022 23:32

alharris-at added a commit to aws-amplify/amplify-cli that referenced this pull request Jun 13, 2022

chore: reflect test updates from aws-amplify/amplify-category-api#517 …

31cb8ab

…in mock

danielleadams mentioned this pull request Jun 14, 2022

doc: add section about breaking @primaryKey/@auth changes aws-amplify/docs#4391

Merged

danielleadams pushed a commit to aws-amplify/amplify-cli that referenced this pull request Jun 16, 2022

chore: reflect test updates from aws-amplify/amplify-category-api#517 …

5f36928

…in mock

sachscode pushed a commit to aws-amplify/amplify-cli that referenced this pull request Jun 16, 2022

update api category packages 2 (#10587)

2497189

* chore: upgrade api category dependencies * chore: reflect test updates from aws-amplify/amplify-category-api#517 in mock

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add error when using an owner field as a sort key field #517

feat: add error when using an owner field as a sort key field #517

danielleadams commented Jun 6, 2022 •

edited

Loading

sundersc Jun 7, 2022 •

edited

Loading

danielleadams Jun 7, 2022 •

edited

Loading

sachscode left a comment

sachscode Jun 7, 2022

danielleadams commented Jun 7, 2022

sachscode left a comment

danielleadams commented Jun 8, 2022

danielleadams Jun 8, 2022

alharris-at Jun 9, 2022

danielleadams Jun 9, 2022

danielleadams Jun 9, 2022

alharris-at Jun 9, 2022

alharris-at Jun 9, 2022

danielleadams Jun 9, 2022

alharris-at Jun 9, 2022

alharris-at Jun 9, 2022

alharris-at Jun 9, 2022

danielleadams Jun 9, 2022

sachscode left a comment

sundersc Jun 10, 2022

alharris-at Jun 10, 2022

danielleadams Jun 10, 2022

sundersc Jun 10, 2022

sundersc Jun 10, 2022

danielleadams Jun 10, 2022


		if (!authDir) return true;

		const dirRules = (authDir.arguments?.find(arg => arg.name.value === 'rules')?.value as ListValueNode).values;

feat: add error when using an owner field as a sort key field #517

feat: add error when using an owner field as a sort key field #517

Conversation

danielleadams commented Jun 6, 2022 • edited Loading

Description of changes

Issue #, if available

Description of how you validated changes

Checklist

sundersc Jun 7, 2022 • edited Loading

Choose a reason for hiding this comment

danielleadams Jun 7, 2022 • edited Loading

Choose a reason for hiding this comment

sachscode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielleadams commented Jun 7, 2022

sachscode left a comment

Choose a reason for hiding this comment

danielleadams commented Jun 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sachscode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielleadams commented Jun 6, 2022 •

edited

Loading

sundersc Jun 7, 2022 •

edited

Loading

danielleadams Jun 7, 2022 •

edited

Loading