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

FIX Make graphql mutation template abstractions work #1498

Conversation

GuySartorelli
Copy link
Member

@GuySartorelli GuySartorelli commented Apr 12, 2023

The input parameters must be lowercase.
This has been required since https://github.com/silverstripe/silverstripe-admin/pull/1148/files#diff-c95be282611b632f7ce8c94d0f00c1c6768dc56b746f43e6bb066d076b5eeb9bR29-R32 which forces lowerCamelCase for field names for graphql v3 in the admin schema.
See also for example the equivalent asset-admin PR where all of the field names in queries/mutations are changed to lowerCamelCase because now they have to be.

I've checked and double checked that these aren't used anywhere in our code (which explains why they were never updated to be lowerCamelCase) but I'll run a CI with asset-admin and campaign-admin with this PR just in case.

Why these changes are required

Without changing Input to input we get the following error in with both graphql versions:

Uncaught (in promise) Error:
GraphQL error: Unknown argument "Input" on field "createNote" of type "Mutation". Did you mean "input"?
GraphQL error: Field "createNote" argument "input" of type "AppNoteCreateInputType!" is required but not provided.

Without dynamically setting the correct input argument types, in graphql 4 we get this error:

Uncaught (in promise) Error: GraphQL error: Missing graphql file for NoteCreateInputType

NOTE

This change was reverted once: #1488
The change being made in this new PR is a lot less invasive, and is only the changes that are absolutely necessary to get the docs working.

I've checked for anywhere that's using these abstractions, and can't find any that will be affected by the changes I've made here - but make sure to check react sections of the CMS to be sure they're working as expected, and if you're aware of any react-heavy modules, maybe test those with this PR as well.

Installer CI run

There are some behat failures related to toast messages - these will be resolved with silverstripe/silverstripe-asset-admin#1345

Issue

The input parameters must be lowercase.
@@ -76,3 +76,15 @@ export const getFragments = ({ availableFragments, fragments = [] }) => (
: capturedFragments
), '')
);

function isLegacy() {
return !!document.body.getAttribute('data-graphql-legacy');
Copy link
Member Author

Choose a reason for hiding this comment

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

See

const isLegacy = !!document.body.getAttribute('data-graphql-legacy');

@GuySartorelli GuySartorelli changed the title FIX Make graphql template abstractions work with gql3 FIX Make graphql mutation template abstractions work Apr 12, 2023
@GuySartorelli GuySartorelli force-pushed the pulls/1.13/graphql-template-abstractions branch from 6bcf55c to 171f652 Compare April 12, 2023 03:58
Copy link
Contributor

@sabina-talipova sabina-talipova left a comment

Choose a reason for hiding this comment

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

Test locally. Didn't see any problem.
LGFM.
Small question, why did you update only buildUpdateMutation.js and buildCreateMutation.js, but didn't change buildDeleteMutation.js buildReadMutation.js etc.?

@GuySartorelli
Copy link
Member Author

I didn't change the others for a couple of reasons:

  1. The primary purpose of this is to make sure the code can support the documentation - which doesn't currently include any query/mutation types other than READ and CREATE.
    • Without documentation for the other query/mutation types it's hard to know exactly how these templates should work - and setting up that documentation is well out of scope for the issue this PR is attached to
    • I did update the code for UPDATE as well, but that's because it's practically identical to CREATE so it was very obvious what needed to be done
  2. Last time I attempted this, I did alter more of the files, but it caused problems. This time I'm taking a more risk-averse approach, only touching what I need to for the documentation to be correct.

At some point it is likely that at least the buildDeleteMutation file will need to be amended, since it uses uppercase IDs instead of lowerCamelCase ids - but since that mutation template isn't documented currently it's not in scope for this issue.

Copy link
Contributor

@sabina-talipova sabina-talipova left a comment

Choose a reason for hiding this comment

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

Tests have been done in local environment with GQ3 and GQ4. All passed. Approve.

@sabina-talipova sabina-talipova merged commit f8b9e5b into silverstripe:1.13 Apr 17, 2023
@sabina-talipova sabina-talipova deleted the pulls/1.13/graphql-template-abstractions branch April 17, 2023 04:18
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.

2 participants