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 context issue in custom GraphQL query and mutation #5532

Merged
merged 4 commits into from Mar 20, 2020

Conversation

iicdii
Copy link
Contributor

@iicdii iicdii commented Mar 18, 2020

Description of what you did:

Fix #5531 issue. Make policiesMiddleware works in custom resolvers too.

@alexandrebodin
Copy link
Member

alexandrebodin commented Mar 18, 2020

@iicdii Hi, Can you please fix the DCO check.

@strapi strapi deleted a comment from codecov bot Mar 18, 2020
@alexandrebodin alexandrebodin self-requested a review March 18, 2020 08:48
@alexandrebodin alexandrebodin added source: plugin:graphql Source is plugin/graphql package issue: bug Issue reporting a bug labels Mar 18, 2020
@alexandrebodin alexandrebodin added this to the 3.0.0-beta.19.4 milestone Mar 18, 2020
Signed-off-by: harimkims <harimkims@gmail.com>
alexandrebodin
alexandrebodin previously approved these changes Mar 18, 2020
Copy link
Member

@alexandrebodin alexandrebodin left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks !

@alexandrebodin alexandrebodin dismissed their stale review March 18, 2020 09:32

Test breaking

@alexandrebodin
Copy link
Member

@iicdii The tests are breaking with your changes.

@iicdii
Copy link
Contributor Author

iicdii commented Mar 18, 2020

@alexandrebodin I'll check the code.

Signed-off-by: harimkims <harimkims@gmail.com>
@codecov
Copy link

codecov bot commented Mar 18, 2020

Codecov Report

Merging #5532 into master will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5532      +/-   ##
==========================================
- Coverage   17.51%   17.50%   -0.01%     
==========================================
  Files         700      700              
  Lines       10330    10332       +2     
  Branches     1687     1688       +1     
==========================================
  Hits         1809     1809              
- Misses       7089     7090       +1     
- Partials     1432     1433       +1     
Flag Coverage Δ
#front 12.88% <ø> (ø)
#unit 37.26% <0.00%> (-0.04%) ⬇️
Impacted Files Coverage Δ
...pe-builder/admin/src/components/ListRow/Wrapper.js 0.00% <ø> (ø)
packages/strapi-plugin-graphql/services/utils.js 34.14% <0.00%> (-1.76%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f3be5e...652fe9a. Read the comment docs.

@derrickmehaffy
Copy link
Member

Tests look to be passing now good catch @iicdii

Copy link
Member

@alexandrebodin alexandrebodin left a comment

Choose a reason for hiding this comment

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

This change feels like a patch rather than a clean fix to me.
I would like to know why graphqlCtx.context = ctx doesn't work and fix it instead of just patching on element of the context then another one etc

@iicdii
Copy link
Contributor Author

iicdii commented Mar 20, 2020

@alexandrebodin
This is the clue I've found why test fails.

// packages/strapi-plugin-users-permissions/test/graphql.test.e2e.js`

test('createUser is authorized for admins', async () => {
  const res = await authReq({
    url: '/graphql',
    method: 'POST',
    body: {
      query: /* GraphQL */ `
        mutation {
          createUser(
            input: {
              data: {
                username: "test"
                email: "test@strapi.io"
                password: "test"
              }
            }
          ) {
            user {
              id
              username
            }
          }
        }
      `,
    },
  });

  expect(res.statusCode).toBe(201);
  expect(res.body).toMatchObject({
    data: {
      createUser: {
        user: {
          id: expect.anything(),
          username: 'test',
        },
      },
    },
  });

  data.user = res.body.data.createUser.user;
});

res.statusCode is 200 when I graphqlContext.context = ctx so test fails.
Otherwise it's 201 when I graphqlContext.context.state = ctx.state so test passes.
It seems to need more digging the code, honestly I don't get the concept difference between graphqlContext and ctx.

@alexandrebodin
Copy link
Member

ctx is the context of the koa app and graphqlContext is the context inside of graphql.

When we receive a graphql query it calls koa middleware hence the need to passe the ctx to them. We just clone it to avoid pbl when running multiple queries at once.

if you look at the createUser mutation you will see it calls a controller with the context and inside of this you have a ctx.created() call. this is supposed to do ctx.status = 201 and ctx.body = data

But this is not copied back to the koa context. Which in my opinion isn't a big deal.

I think we should set graphqlContext.context =. ctx and just change the test to check for a 200 status. In graphql other status don't really make sense anyway.

@iicdii
Copy link
Contributor Author

iicdii commented Mar 20, 2020

Thank you for detailed explanation. I made PR again.

Copy link
Member

@alexandrebodin alexandrebodin left a comment

Choose a reason for hiding this comment

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

LGTM

@alexandrebodin
Copy link
Member

Thanks you for this PR 💯

@alexandrebodin alexandrebodin merged commit 40ea493 into strapi:master Mar 20, 2020
@iicdii iicdii deleted the fix/5531 branch March 21, 2020 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue: bug Issue reporting a bug source: plugin:graphql Source is plugin/graphql package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot access ctx.state.user in custom GraphQL resolver
3 participants