-
Notifications
You must be signed in to change notification settings - Fork 79
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
Nested attributes cannot be returned in updateMutation #2636
Comments
I'm facing a similar issue |
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):
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. |
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):
And then if I mutant this with the autogenerated mutations like so:
So there goes that idea! |
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).
Helps that helps someone. There goes my day... |
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 |
This is probably related to: |
@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. |
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. |
Considering the mentioned CLI version and description of behavior, this issue is probably related to or a duplicate of #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. |
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 #2609 (comment) for the details of the change and how you might be able to unblock yourself. |
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. |
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. |
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 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. |
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! |
Maybe this could help here. |
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 😞 |
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:
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:
I also saw some differences in the generated resolver files between my resolvers build locally using 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. |
I did a diff between the generated resolvers with amplify cli
This is present at the top of multiple 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! ❤️ |
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:
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. |
This. 👆
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......... |
@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. |
@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... |
@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. |
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:
Though I have not tested it myself. 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. |
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. |
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. |
@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? |
@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 |
Please see the updates to the following docs: |
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. |
This issue is now closed. Comments on closed issues are hard for our team to see. |
@dpilch as you've stated in the new docs, the |
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? |
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. |
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.
This is a mistake. The feature flag will default to false for new and existing projects. We will fix the docs shortly. |
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 I'm going to enable 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. |
I tried enabling |
Have you made a successful push? You may need to add a newline to your schema to force an update. |
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 |
Yes, that's correct. |
No joy unfortunately, it doesn't seem to be detecting the change. We're on Amplify Gen 1, V5 (we are using Transformer V2), not sure if that's related. If there's a trick other than making a whitespace change in |
Oh yes, I should have been more explicit. Make a newline change to your |
Ah I see! 😅 The penny's dropped 😝 I'll give that a shot tomorrow, thanks for the info! 🙂 |
That did the trick 🥳 Thank you @dpilch, wouldn't be the first time needing to make a whitespace change in |
Adding this (along with a schema whitespace change and adding some useless fields to be extra-sure)
with amplify version 12.12.4 is still not working for me if I just check it in and let console do a push. I have to run "amplify push" from a computer, like a moron. Is there a way to pass this variable that un-bricks my app to the "amplifyPush --simple" command in my amplify.yml? Also, since this undocumented breaking change seems to be breaking customer prod apps, will the COE be public? |
does setting |
You will need to do a push for each env. |
Has this been fixed? |
Not technically. |
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 mutationMyMutation
I use:This is the data returned by
MyQuery
:This is the data returned by
MyMutation
: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:
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
Additional information
No response
Before submitting, please confirm:
The text was updated successfully, but these errors were encountered: