Nested attributes cannot be returned in updateMutation

jo2 commented 3 months ago

How did you install the Amplify CLI?

npm

If applicable, what version of Node.js are you using?

18.20.3

Amplify CLI Version

12.12.2

What operating system are you using?

Windows

Did you make any manual changes to the cloud resources managed by Amplify? Please describe the changes made.

no

Describe the bug

I can't run update mutations in my code because the mutation does not return existing nested objects. Although the nested objects do exist, they are not returned in the mutation.

Expected behavior

Allthough the update of the mutation is executed, I'd also expect the nested fields to be returned.

Reproduction steps

These are the query MyQuery and the mutation MyMutation I use:

query MyQuery {
  getBooking(id: "d8d815f9-cf12-475d-b728-1447d301ebbd") {
    id
    staffing {
      id
      project {
        id
      }
    }
    activityDescription
    bookingStatus
    endTime
    startTime
    _version
  }
}

mutation MyMutation {
  updateBooking(input: {id: "d8d815f9-cf12-475d-b728-1447d301ebbd", activityDescription: "TEst", bookingStatus: DRAFT, endTime: "2024-06-10T09:00:00.000+02:00", projectId: "8d49c4e3-870c-4b0a-9ca6-17edab7ae5ab", staffingId: "5e5358cf-e047-4875-9df9-43f2ab5fd145", startTime: "2024-06-10T07:00:00.000+02:00", _version: 45}) {
    id
    staffing {
      id
      project {
        id
      }
    }
    activityDescription
    bookingStatus
    endTime
    startTime
    _version
  }
}

This is the data returned by MyQuery:

{
  "data": {
    "getBooking": {
      "id": "d8d815f9-cf12-475d-b728-1447d301ebbd",
      "staffing": {
        "id": "5e5358cf-e047-4875-9df9-43f2ab5fd145",
        "project": {
          "id": "8d49c4e3-870c-4b0a-9ca6-17edab7ae5ab"
        }
      },
      "activityDescription": "Test",
      "bookingStatus": "DRAFT",
      "endTime": "2024-06-10T09:00:00.000+02:00",
      "startTime": "2024-06-10T07:00:00.000+02:00",
      "_version": 46
    }
  }
}

This is the data returned by MyMutation:

{
  "data": {
    "updateBooking": {
      "id": "d8d815f9-cf12-475d-b728-1447d301ebbd",
      "staffing": null,
      "activityDescription": "Test",
      "bookingStatus": "DRAFT",
      "endTime": "2024-06-10T09:00:00.000+02:00",
      "startTime": "2024-06-10T07:00:00.000+02:00",
      "_version": 47
    }
  },
  "errors": [
    {
      "path": [
        "updateBooking",
        "staffing",
        "id"
      ],
      "locations": null,
      "message": "Cannot return null for non-nullable type: 'ID' within parent 'Staffing' (/updateBooking/staffing/id)"
    }
  ]
}

This is a part of the schema I use, I removed the auth rules and only included the parts of the schema I think are relevant for this issue:

type Booking
@model
@auth(
  rules: [
    ..
  ]
) {
  id: ID!
  bookingStatus: BookingStatus!
  bookingType: BookingType!
  activityDescription: String
  staffingId: ID! @index(name: "byStaffing")
  startTime: AWSDateTime!
  endTime: AWSDateTime
  companyId: ID! @index(name: "byCompany")
  note: String
  reviewerIds: [String!]!
  violations: [ViolationType]
  groupId: ID
  staffing: Staffing @belongsTo(fields: ["staffingId"])
  projectId: ID! @index(name: "byProject")
  employeeId: ID! @index(name: "byEmployee")
  readPermissions: [String!]!
  writePermissions: [String!]!
  lastCheckedBy: ID
  owner: String!
}

type Staffing
@model
@auth(
  rules: [
    ..
  ]
) {
  id: ID!
  staffingType: StaffingType!
  projectId: ID! @index(name: "byProject")
  employeeId: ID! @index(name: "byEmployee")
  bookings: [Booking] @hasMany(indexName: "byStaffing", fields: ["id"])
  companyId: ID! @index(name: "byCompany")
  project: Project @belongsTo(fields: ["projectId"])
  employee: Employee @belongsTo(fields: ["employeeId"])
  readPermissions: [String!]!
  updatePermissions: [String!]!
  writePermissions: [String!]!
}

I can't provide a minimal example project for this issue because this issue happens in only one of my environments. Other environments build in the same way, do not face this issue. The only difference was, that my working environment hat its last build on 2024-06-10 12:48 pm and the build for my failing environment was on the same day at 01:44 pm.

I apreceate any help I can get on this issue because this is blocking me in my project. If you need any more information, I'll gladly provide it.

Project Identifier

7a15e020679dc93ffd0710267d909f36

Log output

``` # Put your logs below this line ```

Additional information

No response

Before submitting, please confirm:

[X] I have done my best to include a minimal, self-contained set of instructions for consistently reproducing the issue.
[X] I have removed any sensitive information from my code snippets and submission.

silasmahler commented 3 months ago

I'm facing a similar issue

PeteDuncanson commented 3 months ago

I'm having the same issues as of a release last night a roll back isn't going to fix it either.

Only pattern we've seen is that non-mandatory child types are throwing this so this one (as per the OP example):

staffing: Staffing @belongsTo(fields: ["staffingId"])

Will error but fields defined as mandatory are coming back:

``staffing: Staffing! @belongsTo(fields: ["staffingId"])```

Still needs some more testing but its a hint at least.

PeteDuncanson commented 3 months ago

Seems that pattern I mentioned doesn't always hold true. I've got this multi nested setup that works for the first level fails on the deepest level (hand typed schema so might have typos):

type Car {
  id: ID!
  modelName: String
  manufacturerID: ID!
  manufacturer: Manufacturer @hasOne fields["manufacturerID"]  
}

type Manufacturer {
  id: ID!
  name: String!
  mainMarketID: ID!
  mainMarket: Market @hasOne fields["mainMarketID"]  
}

type Market {
  id: ID!
  name: String!
}

And then if I mutant this with the autogenerated mutations like so:

mutation UpdateCar($input: UpdateCarInput!, $condition: ModelCarConditionInput) {
  updateCar(input: $input, condition: $condition) {
    id
    modelName
    manufacturerID
    manufacturer { # <-- this returns with a value
      id
      name
      mainMarket {  # <-- this comes back as null
         id
         name
      }
    }
  }
}

So there goes that idea!

PeteDuncanson commented 3 months ago

As a horrid work around we've modified our code to simply requery (as queries don't seem to suffer from this error, only mutations) if we get back one of the offending error messages. Rolling this out to all touch points in our code now (luckily we have a DataLayer so only 20 files to touch).


// Something like this
/**
 * Updates a staff member with the given input
 * @param staff the updated staff member model input
 */
const update = async (staff: UpdateStaffMemberModel) => {
  if (staff._version === undefined) {
    console.error(
      "Trying to do an update but you haven't passed through the _version field for this StaffMember"
    );
  }

  const cleanStaff =
    DataAccessHelpers.generateUpdateObjMinusReservedFields(staff);

  const variables = {
    input: cleanStaff,
  };

  const fetchPromise = API.graphql(
    graphqlOperation(updateStaffMember, variables)
  );

  let response: GraphQLResult<UpdateStaffMemberMutation | null> | undefined =
    undefined;

  try {
    response =
      (await fetchPromise) as GraphQLResult<UpdateStaffMemberMutation | null>;

    if (response.errors) {
      logger.warn("Error updating staff", response.errors);
    }
  } catch (error) {
    if (response && response.errors) {
      if (
        response.errors[0].message.indexOf(
          "Cannot return null for non-nullable type"
        ) === 0
      ) {
        return StaffMemberRepository.get(staff.id);
      } else {

        throw error;
      }
    }
  }

  return response?.data?.updateStaffMember as StaffMember | undefined;
};

Helps that helps someone. There goes my day...

PeteDuncanson commented 3 months ago

Update this is also doing this on the generated create mutations not just the update ones as we initially thought. So any mutation with a nested type could be throwing this. Multi level nested types seem to work except for the leaf type which errors.

For completeness I'm using Gen 1 and running amplify v12.10.1

anuj-patel commented 3 months ago

This is probably related to:

PeteDuncanson commented 3 months ago

@anuj-patel I think both of those are separate issues to do with subscriptions; which will error if when you do your mutation you don't pull back all the fields in the return query so similar error message but not the same error. Red herring at this point, plus both of those are years old, this has cropped up only since our last release in the last week or so.

AnilMaktala commented 3 months ago

Hey @jo2, Thank you for bringing this to our attention and @PeteDuncanson for providing further context. I appreciate you taking the time to share the additional details. I am currently investigating this issue and will work on replicating the issue. Once I have more information, I will provide you with an update on the progress and any potential resolutions.

chrisbonifacio commented 3 months ago

Considering the mentioned CLI version and description of behavior, this issue is probably related to or a duplicate of https://github.com/aws-amplify/amplify-category-api/issues/2609

@PeteDuncanson can you confirm whether the environment affected by this issue is a dev/local or prod/hosted environment? You mentioned your CLI version is 12.10.1 but it's possible that a hosted environment could be using a newer version of the Amplify CLI to generate the resolvers with different logic.

You can also check and compare the resolver logic between the environments in the AppSync console to be sure.

PeteDuncanson commented 3 months ago

I've got the build to use the latest version

This is happening on both dev and live.

I'm updating my local version now to the latest which is 12.12.2 apparently. Should I do another push to dev to test?

chrisbonifacio commented 3 months ago

I've got the build to use the latest version

This is happening on both dev and live.

I'm updating my local version now to the latest which is 12.12.2 apparently. Should I do another push to dev to test?

The issue I mentioned is actually expected behavior for subscriptions and mutations using resolvers generated by Amplify CLI v12.12.2 or higher. If you update your local env to the latest it would generate resolvers with the new behavior.

If you were running into issues locally with an older version of the CLI then perhaps it's not the same issue.

I just wanted to point out that, if you are indeed experiencing the same issue, then please refer to https://github.com/aws-amplify/amplify-category-api/issues/2609#issuecomment-2168371400 for the details of the change and how you might be able to unblock yourself.

PeteDuncanson commented 3 months ago

Well. My initial thoughts is that is insane that you've made that the new default without any opt in or warnings. I've lost a whole day to this and none of our users have been able to use the system and we have a tonne of data in an unknown state as well.

Utter insanity. Have to walk away for a while as I'm done for the day after reading that.

silasmahler commented 3 months ago

I need to say that it's also blocking us from releasing anything on any env. The difference luckily appeared first in our stage app. We migrated from v12.7.0 to 12.12.0 and it appeared. As this is a breaking change the major version should definitely be increased. With this update an opt-out config should be expected. This really can't be just made a default, as it breaks functionality like described in the mentioned comments seemingly for a lot of users.

pr0g commented 3 months ago

I ran into this issue today too and was massively confused why this had suddenly stopped working 😅

A workaround is to create a custom query without any nested objects (anything connected by @hasMany, @hasOne etc...). The really annoying thing is all the auto-generated amplify queries in mutations.ts don't do this, so you have to write your own.

I'm wondering did this actually not work before and would just fail silently, and now the error is being reported, or did this used to work and is now broken?

It's possible to do a small mutation and then a query immediately after as a workaround, but this does seem like quite a significant breaking change if it was intentional.

PeteDuncanson commented 3 months ago

I'm wondering did this actually not work before and would just fail silently, and now the error is being reported, or did this used to work and is now broken?

It always works. Our app has been running for 3 years using this pattern. We've a lot of code to patch to work around it now we know what it is!

biller-aivy commented 3 months ago

https://github.com/aws-amplify/amplify-category-api/issues/1652

Maybe this could help here.

pr0g commented 3 months ago

I'm wondering did this actually not work before and would just fail silently, and now the error is being reported, or did this used to work and is now broken?

It always works. Our app has been running for 3 years using this pattern. We've a lot of code to patch to work around it now we know what it is!

I see, good to know. Our team likely have many issues now too, I just don't know how many call sites we have actually relying on a nested return value, hopefully not too many (and we can then add a query if needed), it is definitely a frustrating situation unfortunately 😞

jo2 commented 3 months ago

TL;DR: downgrade works, but be careful of the steps to do so.

Thanks to this comment I got the necessary hint to fix this problem, at least for now. It is only a temporary solution because we won't get any updates or security patches for our app until this is resolved. I used these steps to get my environment back up running:

Downgrade your local @aws-amplify/amplify-cli version to a version using the @aws-amplify/amplify-category-api in the version 5.11.4 or lower. For me it was @aws-amplify/amplify-cli@12.12.0 which uses @aws-amplify/amplify-category-api@5.11.3, according to the package.json @aws-amplify/amplify-cli@12.12.1 should also work, I didn't try it though.
Run amplify push from your local downgraded amplify cli version. Use the logs to verify that indeed your downgraded version is used.
Go to the amplify build settings of your app in your aws console, navigate to hosting --> build settings and set the amplify version used in the build to 12.12.0 (or your local cli version if you use a different one). Edit branch overrides if necessary.
Commit the changes from your amplify push. I've set up a ci so on a commit on a specific branch the amplify build and deploy will be triggered.

For me only completing these for steps in this order resolved the issue. Only completing some of those steps lead to some weird behaviour, for example:

only running the amplify push from a local downgraded cli version fixed the issue temporarily. Triggering the amplify build and deploy after that broke it again.
Only running the amplify build and deploy from a downgraded version in the aws console without doing an amplify push from the local downgraded version didn't fix anything at all which had me thinking that somehow maybe my dynamo db indexes were broken.

I also saw some differences in the generated resolver files between my resolvers build locally using @aws-amplify/amplify-cli@12.12.0 and thos from the deployment bucket from a broken build, but to be honest, I didn't really understood those changes.

As a conclusion, I like the sql analogy given in this comment, if I run a query (or mutation with a return value in this case) in any query language I'd expect to get the data specified in the query if it exists. I now have to prepare for discussions on why we should continue using amplify if it does not provide deterministic query (and mutation) responses on a given input.

Also, I don't consider this issue resolved, because a temporary fix for the cost of missing probably necessary updates is not a solution for me.

pr0g commented 3 months ago

I did a diff between the generated resolvers with amplify cli 12.12.0 and 12.12.2, I and I believe the issue is caused by this change:

## [Start] Check if subscriptions is protected. **
#if( $util.defaultIfNull($ctx.source.get("__operation"), null) == "Mutation" )
  $util.qr($ctx.stash.put("deniedField", true))
#end
## [End] Check if subscriptions is protected. **

This is present at the top of multiple .vtl files that describe relationships, e.g. Model.linkedModel.auth.1.req.vtl

It would be great if the previous functionality could be restored, or another workaround other than creating new custom mutation queries could be provided.

If @chrisbonifacio or @AnilMaktala could give an update on the situation it would be hugely appreciated, thank you! ❤️

PeteDuncanson commented 3 months ago

This is a breaking change. Simple as that.

I'm cross posting a bit from #2609 as want more eyes on it and not sure which should be the "main" issue to track these discussions.

I'm unsure how to unstick myself on this one. Seems I can:

Mess around with my auth settings although I'm happy with how they are and they seem to be setup as previously advised in the docs. But there are no pointers as to how to change the settings to allow for the nested content to come through. The docs linked to for Gen 1 above answer none of this.
Wrap a try/catch around every fetch I do to API.graphql which is what I've been doing but its error prone as we have a lot of them and I have to do it for every mutation, every CRUD operation. We've had new reports today of broken functionality that we've missed for instance so another day lost today patching which we hadn't scheduled for obvs.
Disable subscriptions on all my models - apparently thats an option (I didn't know about this one so I've learnt something). Trouble is I have a feature branch open where we are using subscriptions so I need them, is that work now scrapped too? Simply disabling subscriptions isn't really an option.
Roll back to a previous version and set my CI to use the same version to. This might work but in the past when I've done this I've been stuck on a frozen version for months while a fix is put in place, its not a fun place to be especially if there is a big conference coming up and an announcement of some new goodies that I can't use.
Roll my own mutations and remove any nested objects, this has the same problem as the try/catch, I still have to go through all my code and try to find all the references to anything nested and if needed refetch the nested objects. This is why I find it so frustrating that this change isn't an opt in, it just breaks everything in our app and leaves us with bugs if we haven't found and patched everything.
Ignore the returned values and always do a requery anyway which again is a massive refactor while taking heat.

My boss is going to be asking me questions and I don't have any answers and currently I can't defend Amplify with the way this one has been rolled out. I've got nothing to tell him from your end other than losing nearly 2 days is "by design" and "an improvement" somehow.

Its non-sensical and seems half baked. If you couldn't get auth into subscriptions then why break it for straight up mutations? Or go the other way and give people who need the "improvement" via subscriptions an way to opt in to use subscriptions like an event to say "Item X was updated" so folks could refetch item X rather than messing with sending it over the wire incomplete. This would fix about 80% of the usage problems people have with DataStore too as now it could go get the update rather than failing because someone hand rolled a mutation and didn't return every field!

Still brooding on this one and would like some pointers as to which way to go to get me out of this mess as pain free as possible.

DarylBeattie commented 3 months ago

This is a breaking change. Simple as that.

This. 👆

My boss is going to be asking me questions and I don't have any answers and currently I can't defend Amplify

This is true and scarier than it sounds. Because of this one issue alone, I've been asked to provide a detailed list of all the things we rely on Amplify for.........

DarylBeattie commented 3 months ago

@pr0g @PeteDuncanson downgrading Amplify-CLI and re-pushing fixed it for me, as I commented on #2609 , and @jo2 also did above. That should work as a stop-gap until this issue is repaired by Amplify.

PeteDuncanson commented 3 months ago

@DarylBeattie @jo2 thanks for the pointers on down grading, think that is the way we will go as still finding touch points in our code that aren't patched for the refetch logic. Back to a frozen version again for a few months I guess...

jo2 commented 3 months ago

@PeteDuncanson same's for me, it is a rather current version though so I'm not that worried. Also, I don't think we'll miss out on that many new features because I think most energy is invested into gen2 and the migration tools for gen1 customers.

It opened my eyes on the need of having a fixed version in the amplify build and deploy setup, too though. I'll probably stay on this version until the mentioned gen2 migration tools are available and kind of stable or we get a real fix on this.

JGamboa commented 3 months ago

@AnilMaktala So basically the amplify form generator is not working anymore because you made a change that affect everything? Whats the point of using the form generator now then ?

JGamboa commented 3 months ago

If you use the form generator and it doenst work, then the upgrade is a bug. And not an "improvement"

kekami commented 3 months ago

I can only agree that this was a really half-baked release. It completely broke data access from lambdas in gen 2, since you have to use the generated graphql client code for data access in lambdas. And the generated code for mutations tries to return all related objects, which throws errors.

What is more concerning is that core functionality can break like this, without being caught by tests and QA.

chrisbonifacio commented 2 months ago

For those running into issues with generated forms because of the nested relational fields being included in selection sets, a potential workaround might be to generate the graphql statements with a max depth of 1 using the following command to replace the statements generated by npx ampx generate forms

Gen 1

amplify codegen --maxDepth 1

Gen 2

npx ampx generate graphql-client-code --statement-max-depth=1 --out=ui-components/graphql

pr0g commented 2 months ago

Thanks for the suggestion @chrisbonifacio, I unfortunately ran into problems getting the above to run (on macOS).

One other idea is, might it be possible to update the Amplify extension settings in .graphqlconfig.yml to separate query depth and mutation/subscription depth:

Current

extensions:
  amplify:
    codeGenTarget: typescript
    generatedFileName: src/API.ts
    docsFilePath: src/graphql
    maxDepth: 4

Proposed

extensions:
  amplify:
    codeGenTarget: typescript
    generatedFileName: src/API.ts
    docsFilePath: src/graphql
    maxQueryDepth: 4
    maxMutationDepth: 1
    maxSubscriptionDepth: 1

Not sure how hard this would be, but otherwise everytime amplify codegen types/statements/models is run, the generated code will get updated and replaced again

chrisbonifacio commented 2 months ago

Hi @pr0g that would be a good feature request! Please open one if there isn't one already.

The workaround at the moment would be to run the generate graphql-client-code command twice - once with default depth, move the queries outside of the output directory. Then, re-run the command with a depth of 1 for mutations/subscriptions, and copy the queries back.

As for the issues you ran into while running the command - are you building an Amplify Gen 1 or Gen 2 app? I assume Gen 1 if you are using a .graphqlconfig.yml file.

For Gen 1, the command would be:

amplify codegen --maxDepth 1

pr0g commented 2 months ago

Hi @chrisbonifacio,

Thanks very much for getting back to me, it's good to know that doesn't sound like a crazy idea 😅 I'd be happy to create a feature request and I can cross-reference it here.

Thanks for the workaround suggestion, we are still on Gen 1 so the amplify codegen --maxDepth 1 option should do the trick. It'll do that for now to help get us unblocked (I've also been manually creating custom mutations in the src/graphql folder and manually removing any relations (as an example say we have a mutation called createPlayer that might have some relationships to other models, I've created a custom mutation called createPlayerNoRelations that omits them). It's not perfect but it's good enough for now.

Thanks for your time and I'll create a separate feature request issue shortly 👍

pr0g commented 2 months ago

@chrisbonifacio I created the feature request (see above)

Just to add one more thing, the solution you proposed won't work exactly as the API.ts file will be left in an incorrect state (without all the query responses with the right depth).

I believe to have this work as expected you'd need to run:

cd <root-amplify-project>
# ensure everything is in a good initial state
amplify codegen
# copy the queries.ts file you want to keep to the root temporarily
cp src/graphql/queries.ts .
# generate all queries/mutations/subscriptions with depth 1 (this will also update API.ts)
amplify codegen --maxDepth 1
# move existing queries.ts file back and overwrite the file that was just generated with a depth of 1
mv queries.ts src/graphql
# generate API.ts file again and populate it with the correct depth types for queries.ts
amplify codegen types

I think that should handle everything correctly

DarylBeattie commented 2 months ago

While better control over nested query depth is a good feature request, might I point out:

Disabling nested query depth is a stop-gap workaround to this bug, not a fix for it. We should still be focused on how to get this bug fixed.

DarylBeattie commented 2 months ago

It does look like this PR adds a feature-flag to disable the feature that causes this bug:

To use it, you add into your amplify/cli.json the following flag to the "graphqltransformer" section:

subscriptionsInheritPrimaryAuth: true

Though I have not tested it myself. I'm curious to know if it also fixes the update GQL statements, as it is labeled "subscriptions" in the name.

To be clear here: A new feature caused a bug, and instead of fixing the bug, a feature flag was added to disable the broken feature.

PeteDuncanson commented 2 months ago

This one does seem to be in a bit of a spiral of unintended consequences doesn't it?

Mutations to me should just be left alone as they where. The original changes seem to be for subscriptions only so should be targeted as such and leave Update,Delete,Create mutations alone. If its not possible to separate out the two then roll back until you can.

My worry is that this feature will hide behind this feature flag for a while until the flag gets retired at which point we will have to refactor anyway so are we just delaying the pain of refactoring to this new model of data access anyway? If that is the case then how do I know another such "improvement" won't be rolled out and we do this all again? I think we all need to learn about how this sort of stuff should be handled in the future so we don't have this mess next time. I can't keep justifying enforced refactoring and downtime to the boss! Pointing to call out boxes that are being hastily retro-actively added to the docs only seem to protect Amplify with a "but it says so in the docs" answer rather than those of us stuck with broken apps. It gives you something to point to when people complain its broken rather than helping us by not breaking it or making better changes to the code to unbreak it.

I was hoping to jump up to Gen 2 soon but my slack for doing that has been gobbled up by this one so we are all having knock on effects due to this one. Would love a proper reply from Amplify once you've got a united front answer for us about how this one came into being and how its been handled. Currently we are having to fill in the void of info ourselves by finding PRs and trying to work out what you are up to.

DarylBeattie commented 2 months ago

To use it, you add into your amplify/cli.json the following flag to the "graphqltransformer" section:
subscriptionsInheritPrimaryAuth: true

FYI for those wondering: this seems to work.

Though I must stress: Breaking core functionality with an incorrect permissions feature implementation, and then adding a config to disable that feature, is not the right way to code this.

The proper fix would be: Always return data if the requester asked for it and they have permission to get it, otherwise return null. (Don't just "always return nothing because it's easier".)

Edit: This was on Amplify CLI v12.12.4.

DeezNutz2 commented 2 months ago

@biller-aivy THANK YOU I'm having the same issue.

Looks like this ticket is further along.

Yeah, it's frustrating that they codegen a bunch of depth-2 Create and Update mutations that don't work. Anyone have any workarounds for using different-depth queries within the same app? There are only a few places anyone wants to have depth-3 queries in my app; usually they'd be happy to have just depth-1 or 2. But, to support the depth-3 query, EVERY SINGLE query in my app that isn't hardcoded has to be depth-3. It's a silly problem, even before this issue.

I've been trying to figure out why this issue is manifesting on one project and not the other -- maybe it's amplify-cli version.

So, we don't think this is an appsync issue?

pr0g commented 2 months ago

@DeezNutz2 I think this answer should help as a manual workaround, I've also created this feature request which might get some traction you never know 🙂

It also sounds like setting subscriptionsInheritPrimaryAuth: true in amplify/cli.json (as mentioned by @DarylBeattie) also works as a solution/workaround for now.

dpilch commented 2 months ago

@aws-amplify/cli@12.12.4 has been released with a feature flag to opt-out of the new behavior.

Please see the updates to the following docs:

dpilch commented 2 months ago

There is currently not a way to generate some GraphQL statements with different level of depth. We are investigating options so that the errors will not be present in the response.

github-actions[bot] commented 2 months ago

This issue is now closed. Comments on closed issues are hard for our team to see. If you need more assistance, please open a new issue that references this one.

jo2 commented 2 months ago

@dpilch as you've stated in the new docs, the subscriptionsInheritPrimaryAuth flag works for subscriptions. Does this also work for mutations which was the initial reason for this ticket? I don't want to break yet another environment testing this..

PeteDuncanson commented 2 months ago

I found the feature flag confusing too. It states that this feature flag will be set to true for existing setups/apps so in theory I should have to add anything just update amplify to the latest? But it also states in the other docs link that I should add the feature flag.

Which is it?

silasmahler commented 2 months ago

To use it, you add into your amplify/cli.json the following flag to the "graphqltransformer" section:
subscriptionsInheritPrimaryAuth: true
FYI for those wondering: this seems to work.

Though I must stress: Breaking core functionality with an incorrect permissions feature implementation, and then adding a config to disable that feature, is not the right way to code this.

The proper fix would be: Always return data if the requester asked for it and they have permission to get it, otherwise return null. (Don't just "always return nothing because it's easier".)

Edit: This was on Amplify CLI v12.12.4.

With quote to your reply @DarylBeattie I must say I support your opinion. I know for the maintainers it's frustrating to remove already written code, but in this case the feature should simply be removed, as it doesn't provide an improvement, unfortunately it adds a lot of unintuitive confusion for the amplify users, as we see within all the issues.

It's also not a good practice to have a feature flag added which is mandatory for everyone to implement, with the current version increment.

The upgrade process in the minor and patch releases should be kept minimal and clean for the users, as the semantic versioning suggests these changes to be backwards-compatible.

@chrisbonifacio please repoen this issue, as @jo2's request wasn't fully cleared, I'd also like to know if this concerns the mutations.

dpilch commented 2 months ago

@dpilch as you've stated in the new docs, the subscriptionsInheritPrimaryAuth flag works for subscriptions. Does this also work for mutations which was the initial reason for this ticket? I don't want to break yet another environment testing this..

This will set the mutations to the previous behavior. In AppSync mutation results and subscription results are inextricably linked. Please see an excerpt from this AppSync doc.

Subscriptions are triggered from mutations and the mutation selection set is sent to subscribers.

I found the feature flag confusing too. It states that this feature flag will be set to true for existing setups/apps so in theory I should have to add anything just update amplify to the latest? But it also states in the other docs link that I should add the feature flag.

This is a mistake. The feature flag will default to false for new and existing projects. We will fix the docs shortly.

pr0g commented 2 months ago

One quick thing regarding the above set of steps I mentioned in this comment, is this unfortunately doesn't really work, because the moment you run an amplify push, as part of that step, the src/graphql/mutations.ts and src/graphql/subscriptions.ts will be updated to use the default depth anyway (maybe there's a cli option to prevent this but I'm not aware of it unfortunately).

I'm going to enable subscriptionsInheritPrimaryAuth: true for our team, and will continue using custom queries where it makes sense and omit any relationships manually there.

I'm super interested about the solution @dpilch mentioned the team are looking at to avoid errors being generated when using the generated mutations with depth > 1.

pr0g commented 2 months ago

I tried enabling subscriptionsInheritPrimaryAuth: true, but strangely I'm still seeing the error reported when called from our Lambda functions using AWSAppSyncClient. It's possible the change hasn't propagated somehow yet? I'm not sure if there's a way to force that, is there something I'm missing?

dpilch commented 2 months ago

Have you made a successful push? You may need to add a newline to your schema to force an update.

pr0g commented 2 months ago

Thanks @dpilch, I have yes, tried a couple of times, I'll try and just make a whitespace change in case. I take it when running amplify status I should see the category API item with Update under the Operation column?

dpilch commented 2 months ago

Yes, that's correct.

aws-amplify / amplify-category-api