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

make id arg required in singular content type graphql request #15877

Merged
merged 2 commits into from Mar 7, 2023

Conversation

innerdvations
Copy link
Contributor

@innerdvations innerdvations commented Feb 22, 2023

What does it do?

Makes the "id" argument in Graphql queries required for singular content type requests (eg, "restaurant" but not "restaurants" or single types)

Despite changing the schema, I do not believe it should be considered a breaking change because currently the queries are already broken, so this only corrects the type of error that is returned.

Why is it needed?

Before this PR, failing to include an id results in an invalid query that goes all the way to the database layer:
Screenshot 2023-02-22 at 10 55 17

After this PR, we catch the missing ID with the graphql schema:
Screenshot 2023-02-22 at 10 54 45

How to test it?

Test that id argument is now required for singular content request types that previously had only an "id" arg that wasn't required.

It should not be required for single types or multiple request queries

@innerdvations innerdvations self-assigned this Feb 22, 2023
@innerdvations innerdvations added source: plugin:graphql Source is plugin/graphql package pr: fix This PR is fixing a bug labels Feb 22, 2023
@innerdvations innerdvations marked this pull request as ready for review February 22, 2023 10:05
@innerdvations innerdvations added the flag: don't merge This PR should not be merged at the moment label Feb 22, 2023
@codecov
Copy link

codecov bot commented Feb 22, 2023

Codecov Report

Patch coverage has no change and project coverage change: +5.61 🎉

Comparison is base (b9e92de) 60.68% compared to head (e47e7d0) 66.30%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #15877      +/-   ##
==========================================
+ Coverage   60.68%   66.30%   +5.61%     
==========================================
  Files        1495     1121     -374     
  Lines       36878    22792   -14086     
  Branches     7359     4191    -3168     
==========================================
- Hits        22380    15112    -7268     
+ Misses      12417     6800    -5617     
+ Partials     2081      880    -1201     
Flag Coverage Δ
back ?
front 66.30% <ø> (ø)
unit_back ?
unit_front 66.30% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...content-manager/server/controllers/single-types.js
...nager/server/services/utils/configuration/index.js
packages/core/utils/lib/parse-multipart.js
...ore/strapi/lib/core-api/service/collection-type.js
...ore/content-manager/server/services/utils/store.js
...core/strapi/lib/services/metrics/stringify-deep.js
...builder/server/controllers/validation/component.js
...ages/core/strapi/lib/services/content-api/index.js
...s/core/database/lib/validations/relations/index.js
...es/plugins/i18n/server/migrations/field/migrate.js
... and 364 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@innerdvations innerdvations removed the flag: don't merge This PR should not be merged at the moment label Feb 28, 2023
@Bassel17
Copy link
Member

This change looks good to me, I think you can update this with main and it should be fine to mover forward with this fix

@innerdvations innerdvations merged commit 2a22da6 into main Mar 7, 2023
@innerdvations innerdvations deleted the fix/graphql-findone-require-id branch March 7, 2023 15:28
@aaronLejeune
Copy link

Hi all,

Had some headaches after installing this update because this commit is causing my application to break.

I want to query based on SLUG instead of ID an followed this tutorial:
https://forum.strapi.io/t/strapi-v4-search-by-slug-instead-id/13469/37

In short, this is the code we used
`register({ strapi }) {
const extensionService = strapi.plugin("graphql").service("extension");

const extension = () => ({
  typeDefs: `
    type Query {
      faqQuestion(slug: String!): FaqQuestionEntityResponse
    }
  `,
  resolvers: {
    Query: {
      faqQuestion: {
        resolve: async (parent, args, context) => {
          const { toEntityResponse } = strapi.service(
            "plugin::graphql.format"
          ).returnTypes;
          const data = await strapi.services["api::faq-question.faq-question"].find({
            filters: { slug: args.slug },
          });
          const response = toEntityResponse(data.results[0]);
          return response;
        },
      },
    },
  },
});

extensionService.use(extension);

},`

Which of course is causing this error:
Field \"faqQuestion\" argument \"id\" of type \"ID!\" is required, but it was not provided.

Any idea on how to bypass the "ID is required" issue?

It seems like a lot of developers are using that tutorial to query their contentTypes, will keep an eye out for future issues because not everyone is reading updates. Thanks a lot!

@strapi-bot
Copy link

This pull request has been mentioned on Strapi Community Forum. There might be relevant details there:

https://forum.strapi.io/t/strapi-v4-search-by-slug-instead-id/13469/54

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: fix This PR is fixing a bug source: plugin:graphql Source is plugin/graphql package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants