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 warning regarding subscription and mutation redaction of relational fields #7768

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

chrisbonifacio
Copy link
Contributor

Description of changes:

Add warning regarding subscription and mutation redaction of relational fields

Related GitHub issue #, if available:

aws-amplify/amplify-category-api#2636

aws-amplify/amplify-category-api#2609

Instructions

If this PR should not be merged upon approval for any reason, please submit as a DRAFT

Which product(s) are affected by this PR (if applicable)?

  • amplify-cli
  • amplify-ui
  • amplify-studio
  • amplify-hosting
  • amplify-libraries

Which platform(s) are affected by this PR (if applicable)?

  • JS
  • Swift
  • Android
  • Flutter
  • React Native

Please add the product(s)/platform(s) affected to the PR title

Checks

  • Does this PR conform to the styleguide?

  • Does this PR include filetypes other than markdown or images? Please add or update unit tests accordingly.

  • Are any files being deleted with this PR? If so, have the needed redirects been created?

  • Are all links in MDX files using the MDX link syntax rather than HTML link syntax?

    ref: MDX: [link](https://docs.amplify.aws/)
    HTML: <a href="https://docs.amplify.aws/">link</a>

When this PR is ready to merge, please check the box below

  • Ready to merge

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

@chrisbonifacio chrisbonifacio marked this pull request as ready for review June 20, 2024 21:22
@chrisbonifacio chrisbonifacio requested a review from a team as a code owner June 20, 2024 21:22
@PeteDuncanson
Copy link

Lovely to see this documented somewhere so others can be pointed to it. I wonder if we could add something to the graphql response too as a sign post to the reason the nested types are suddenly null? Was wondering if something could be done with the deprecated directive or similar, adds a message that devs can find ideally without throwing an exception but if you keep the expection at least there is a reason, link to these docs and a possible one liner explaining what has happened, a delicious DX rather than what we currently have.

Alternatively/Additionally add to the error messages explaining that the auth differs so whats why its coming back empty?

As it currently stands I went down rabbit holes trying to find what might have changed to cause it even to the dread of "have I over written live data with dev data somehow?" which is never a good feeling. An explaination (and link to these new docs) would have do wonders.

Thoughts?

Copy link
Member

@lawmicha lawmicha left a comment

Choose a reason for hiding this comment

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

Thanks for kicking this off, I reused the warning you have in this PR in the Relational Models page and added some extra details for the lazy/eager loading. I cherry-picked the commit over here so the warning can be updated in this PR as we iterate on it.

@pr0g
Copy link

pr0g commented Jun 24, 2024

I'm curious is there a plan to update the autogenerated code for mutations so relational fields are not included? This would be really handy as otherwise the autogenerated mutations will continue to throw errors such as:

"graphQLErrors": [
        {
            "path": [
                "createModelOfSomeKind",
                "linkedModel",
                "id"
            ],
            "locations": null,
            "message": "Cannot return null for non-nullable type: 'ID' within parent 'LinkedModel' (/createModelOfSomeKind/linkedModel/id)"
        },
        ...

Or is the recommentation to create custom mutations for everything from now on?

Thanks!

@sundersc
Copy link
Contributor

@chrisbonifacio Could you please add info about the new feature flag graphqltransformer > subscriptionsInheritPrimaryAuth.

If enabled, subscriptions will inherit the primary model authorization rules for the relational fields.
If disabled, relational fields will be redacted in mutation response when there is a difference between auth rules between primary and related models.

palpatim
palpatim previously approved these changes Jun 27, 2024
@palpatim palpatim dismissed their stale review June 27, 2024 22:59

Review inadvertently satisfied documentation team requirement

@palpatim palpatim requested a review from a team June 27, 2024 23:00
@chrisbonifacio chrisbonifacio enabled auto-merge (squash) June 28, 2024 01:48
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.

None yet

6 participants