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

Refactor: avoid double parsing of mutation string in ACL #3494

Merged
merged 1 commit into from Jun 1, 2019

Conversation

@mangalaman93
Copy link
Contributor

commented May 31, 2019

This change is Reviewable

@mangalaman93 mangalaman93 requested review from gitlw, manishrjain and dgraph-io/team May 31, 2019
@mangalaman93 mangalaman93 force-pushed the aman/acl_refactor branch from fee6274 to d153e45 May 31, 2019
@mangalaman93 mangalaman93 force-pushed the aman/acl_refactor branch from d153e45 to 6f3c807 May 31, 2019
Copy link
Member

left a comment

:lgtm:

Reviewed 3 of 3 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @gitlw and @manishrjain)

@gitlw
gitlw approved these changes May 31, 2019
Copy link
Contributor

left a comment

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @gitlw and @manishrjain)

@gitlw
gitlw approved these changes May 31, 2019
Copy link
Contributor

left a comment

Thanks for the refactoring!

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @gitlw and @manishrjain)

Copy link
Member

left a comment

I don't get this refactoring. It changes the sequence of mutation handling quite significantly. For e.g., we check for context, then check if the request is empty, only then we try to parse the mutation. This change reorders all of that.

What's the rationale?

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @manishrjain)

Copy link
Contributor Author

left a comment

Short answer:
We were calling parseMutation already inside authorizeMutation. I have only moved it outside and reused it in both authorizeMutation and doMutate/Mutate functions

Long answer:
Earlier, we were calling parseMutation function two places. Following is the old sequence of operations -
-> Call authorizeMutation
-> Check whether ACL feature is enabled
-> Call parseMutation
-> More code
-> Call doMutate
-> More code (somewhere here we call parseMutation again)

I modified this sequence to -
-> Call parseMutation [This is copied from below]
-> Call authorizeMutation
-> Check whether ACL feature is enabled
-> [Call parseMutation (This is removed in the new code and moved up]
-> More code
-> Call doMutate
-> More code (somewhere here we call parseMutation again)
^ [No need to call parseMutation here any more]

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Member

left a comment

:lgtm: I see.

Reviewed 3 of 3 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@mangalaman93 mangalaman93 merged commit f2d404b into master Jun 1, 2019
5 checks passed
5 checks passed
Blockade (dgraph) TeamCity build finished
Details
CI (dgraph) TeamCity build finished
Details
GolangCI No issues found!
Details
code-review/reviewable 3 files reviewed
Details
license/cla Contributor License Agreement is signed.
Details
@mangalaman93 mangalaman93 deleted the aman/acl_refactor branch Jun 1, 2019
dna2github added a commit to dna2fork/dgraph that referenced this pull request Jul 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.