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

Prevent dropping or altering of reserved predicates. #2967

Merged
merged 12 commits into from
Feb 8, 2019

Conversation

martinmr
Copy link
Contributor

@martinmr martinmr commented Feb 1, 2019

There is already logic to prevent certain internal predicates (ACL and
type preds) from being dropped by a DropAll request. This change adds
similar logic to prevent DropAttr and Schema requests from modifying or
dropping reserved predicates.


This change is Reviewable

@martinmr martinmr requested a review from a team February 1, 2019 23:01
There is already logic to prevent certain internal predicates (ACL and
type preds) from being dropped by a DropAll request. This change adds
similar logic to prevent DropAttr and Schema requests from modifying or
dropping reserved predicates.
edgraph/server.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 1 of 2 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @golangcibot and @martinmr)


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

	// Reserved predicates cannot be dropped.
	if _, ok := x.InitialPreds[op.DropAttr]; len(op.DropAttr) > 0 && ok {

Let's keep the flow of code the same. We don't need to do this upfront. Move this to under op.DropAll.


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

	updates, schemaErr := schema.Parse(op.Schema)
	if schemaErr == nil {
		for _, update := range updates {

Move this to under the previous schema.Parse section.

Copy link
Contributor Author

@martinmr martinmr 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: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @golangcibot and @manishrjain)


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

Previously, golangcibot (Bot from GolangCI) wrote…

File is not goimports-ed (from goimports)

Done.


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

Previously, manishrjain (Manish R Jain) wrote…

Let's keep the flow of code the same. We don't need to do this upfront. Move this to under op.DropAll.

Done.


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

Previously, manishrjain (Manish R Jain) wrote…

Move this to under the previous schema.Parse section.

Done.

@martinmr martinmr dismissed manishrjain’s stale review February 4, 2019 22:18

Addressed comments. Ready for another review.

@martinmr
Copy link
Contributor Author

martinmr commented Feb 6, 2019

I didn't realized that the test was broken after I addressed the comments in the previous review. I have fixed that and this is ready for another round of review.

@martinmr martinmr requested a review from a team February 6, 2019 02:07
Copy link
Contributor

@codexnull codexnull left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @golangcibot and @manishrjain)

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:

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @golangcibot and @manishrjain)

@martinmr martinmr merged commit 7ace75b into master Feb 8, 2019
@martinmr martinmr deleted the martinmr/reserved-preds branch February 8, 2019 18:31
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 19, 2019
There is already logic to prevent certain internal predicates (ACL and
type preds) from being dropped by a DropAll request. This change adds
similar logic to prevent DropAttr and Schema requests from modifying or
dropping reserved predicates.
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.

5 participants