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

Add support for *API* required for Multiple Mutation #3839

Merged
merged 4 commits into from
Aug 23, 2019
Merged

Conversation

mangalaman93
Copy link
Contributor

@mangalaman93 mangalaman93 commented Aug 20, 2019

Related to #3817


This change is Reviewable

@mangalaman93 mangalaman93 requested review from manishrjain and a team as code owners August 20, 2019 13:56
edgraph/access_ee.go Outdated Show resolved Hide resolved
Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

Reviewed 17 of 17 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mangalaman93)


edgraph/server.go, line 762 at r1 (raw file):

		return s.doMutate(ctx, req, true)
	}

Why not do the authorization here as was being done before? I'm not following the need for change in doQuery func.

Copy link
Contributor Author

@mangalaman93 mangalaman93 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 13 of 18 files reviewed, 2 unresolved discussions (waiting on @golangcibot and @manishrjain)


edgraph/access_ee.go, line 381 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

File is not gofmt-ed with -s (from gofmt)

				{Set: createUserNQuads},

Done.


edgraph/server.go, line 762 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Why not do the authorization here as was being done before? I'm not following the need for change in doQuery func.

Because we need to call authorizeMutate or authorizeQuery depending upon whether it is a mutation or it is a query. I just added code to do authorization for query in Mutation. I don't want to refactor this code in this PR, will do it when I implement Multiple Mutation.

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

Reviewable status: 11 of 18 files reviewed, 3 unresolved discussions (waiting on @golangcibot, @mangalaman93, and @manishrjain)


dgraph/cmd/alpha/http.go, line 316 at r2 (raw file):

		mu := &api.Mutation{}
		req = &api.Request{Mutations: []*api.Mutation{mu}}

Move this closer to usage. So a few lines below.


edgraph/server.go, line 762 at r1 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Because we need to call authorizeMutate or authorizeQuery depending upon whether it is a mutation or it is a query. I just added code to do authorization for query in Mutation. I don't want to refactor this code in this PR, will do it when I implement Multiple Mutation.

You know that information here (whether it is a query or a mutation). Call the authorization code here. What am I missing?

Copy link
Contributor Author

@mangalaman93 mangalaman93 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 11 of 18 files reviewed, 3 unresolved discussions (waiting on @golangcibot, @mangalaman93, and @manishrjain)


edgraph/server.go, line 762 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

You know that information here (whether it is a query or a mutation). Call the authorization code here. What am I missing?

I didn't want to refactor code in this PR. There will be a lot of refactoring work that I'd do in the PR for multiple mutation. Anyway, I can get authorization part in in this PR.

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

Reviewable status: 11 of 18 files reviewed, 3 unresolved discussions (waiting on @golangcibot, @mangalaman93, and @manishrjain)


edgraph/server.go, line 762 at r1 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

I didn't want to refactor code in this PR. There will be a lot of refactoring work that I'd do in the PR for multiple mutation. Anyway, I can get authorization part in in this PR.

But, you introduced a bool in doQuery API. The comment is asking about removing it. How is that refactoring?

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

:lgtm: Make the changes and merge it.

Reviewed 7 of 7 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @golangcibot and @mangalaman93)


edgraph/server.go, line 422 at r2 (raw file):

}

func (s *Server) doMutate(ctx context.Context, req *api.Request, authorize bool) (

Switch authorize to an enum.


edgraph/server.go, line 775 at r2 (raw file):

	}

	ctx, span := otrace.StartSpan(ctx, methodQuery)

Move it back to wherever it was.

Copy link
Contributor Author

@mangalaman93 mangalaman93 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 15 of 18 files reviewed, 4 unresolved discussions (waiting on @golangcibot and @manishrjain)


dgraph/cmd/alpha/http.go, line 316 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Move this closer to usage. So a few lines below.

I think we discussed this, req is defined outside the switch. And putting mu inside req makes more sense to do as soon as possible.


edgraph/server.go, line 422 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Switch authorize to an enum.

Done.


edgraph/server.go, line 775 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Move it back to wherever it was.

Done.

@mangalaman93 mangalaman93 force-pushed the aman/multi_mut branch 2 times, most recently from 400d537 to 1ccd20e Compare August 22, 2019 09:49
Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

:lgtm: Got to merge!

Reviewed 4 of 4 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @golangcibot)

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

I meant good to merge.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @golangcibot)

mangalaman93 and others added 2 commits August 23, 2019 14:36
This will be removed once we migrate to Go Modules, which will happen
once Badger v2 is released.
@campoy campoy merged commit abc64ce into master Aug 23, 2019
@campoy campoy deleted the aman/multi_mut branch August 23, 2019 22:22
MichaelJCompton added a commit that referenced this pull request Sep 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants