Verify that all the fields in a type exist in the schema. #4114

martinmr · 2019-10-02T00:27:20Z

Also, move the existing verification to the same place so that all type
verification is done at the same time.

This change is

Also, move the existing verification to the same place so that all type verification is done at the same time.

pullrequest

✅ A review job has been created and sent to the PullRequest network.

@martinmr you can click here to see the review status or cancel the code review job.

worker/mutation.go

pullrequest

Changes here look pretty good to me, left a few comments inline. One specific question is around the ctx.Background() being used. I just wanted to verify this wasn't supposed to extend from the ctx being passed in or have some sort of timeout to it.

Reviewed with ❤️ by PullRequest

worker/mutation.go

pawanrawal

Reviewable status: 0 of 7 files reviewed, 8 unresolved discussions (waiting on @manishrjain, @martinmr, and @pawanrawal)

worker/mutation.go, line 573 at r3 (raw file):

		fields := make([]string, len(t.Fields))
		for i, field := range t.Fields {
			fields[i] = field.Predicate

If some preds are supplied in the request, i.e. are in the preds map, then you don't really need to fetch the schema for them.

worker/mutation.go, line 575 at r3 (raw file):

			fields[i] = field.Predicate
		}
		schs, err := GetSchemaOverNetwork(ctx, &pb.SchemaRequest{Predicates: fields})

This network call could be optimized and be done only once after collecting fields from all types.

worker/mutation.go, line 593 at r3 (raw file):

			if !inSchema && !inRequest {
				return errors.Errorf(
					"Schema does not contain a matching predicate for field %s in type %s",

Do we have a test case for testing this scenario?

worker/mutation.go, line 605 at r3 (raw file):

			if field.ValueType == pb.Posting_OBJECT && len(field.ObjectTypeName) == 0 {
				return errors.Errorf(
					"Field with value type OBJECT must specify the name of the object type")

Error test cases for these scenarios would be good to have so that if we break them tomorrow, we know that we did.

martinmr

Reviewable status: 0 of 7 files reviewed, 8 unresolved discussions (waiting on @golangcibot, @manishrjain, @pawanrawal, and @pullrequest[bot])

worker/mutation.go, line 578 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

Error return value of errors.Errorf is not checked (from errcheck)

Done.

worker/mutation.go, line 561 at r2 (raw file):

Previously, pullrequest[bot] wrote…

you could potentially pass a size here to prevent having to re-size the map:
preds := make(map[string]struct{}, len(m.Schema))

Done.

worker/mutation.go, line 575 at r2 (raw file):

Previously, pullrequest[bot] wrote…

Should this extend from the ctx or is it supposed to be a disconnected context? I would consider leaving a comment inline if it is meant to be disconnected.

Done.

worker/mutation.go, line 582 at r2 (raw file):

Previously, pullrequest[bot] wrote…

Similar to above, could be worth passing the len(schs) here to initialize map with exact amount of space and save allocations.

Done.

worker/mutation.go, line 573 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

If some preds are supplied in the request, i.e. are in the preds map, then you don't really need to fetch the schema for them.

Done.

worker/mutation.go, line 575 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

This network call could be optimized and be done only once after collecting fields from all types.

Done.

worker/mutation.go, line 593 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Do we have a test case for testing this scenario?

Done.

worker/mutation.go, line 605 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Error test cases for these scenarios would be good to have so that if we break them tomorrow, we know that we did.

Done.

pullrequest

I've added an inline comment regarding error handling in the for loop.

Reviewed with ❤️ by PullRequest

worker/mutation.go

dgraph/cmd/alpha/run_test.go

systest/mutations_test.go

worker/mutation.go

systest/mutations_test.go

pawanrawal

apart for one change which seems to be missing

Reviewed 4 of 7 files at r1, 1 of 2 files at r3, 3 of 3 files at r4.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @golangcibot, @manishrjain, @martinmr, and @pullrequest[bot])

worker/mutation.go, line 575 at r3 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

Done.

Not sure if this is done, I was talking about collecting fields from all types and then making the network call outside after this for loop.

martinmr

Reviewable status: 5 of 8 files reviewed, 9 unresolved discussions (waiting on @golangcibot, @manishrjain, @pawanrawal, and @pullrequest[bot])

dgraph/cmd/alpha/run_test.go, line 1232 at r3 (raw file):

Previously, pullrequest[bot] wrote…

Not a trivial change here but this would be more readable as an enum if a refactoring comes up.

Leaving it like this for now.

systest/mutations_test.go, line 1672 at r4 (raw file):

Previously, pullrequest[bot] wrote…

This has come up in previous reviews, but I think the tick mark needs to be moved to the leftmost position. If that is not an issue, disregard.

Done. Moved tick to the previous line.

systest/mutations_test.go, line 1681 at r4 (raw file):

Previously, pullrequest[bot] wrote…

This has come up in previous reviews, but I think the tick mark needs to be moved to the leftmost position. If that is not an issue, disregard.

Done.

worker/mutation.go, line 562 at r3 (raw file):

Previously, pullrequest[bot] wrote…

What are sups? Schema updates? The variable name here could be a bit more descriptive. Not entirely clear what's being articulated.

Done.

worker/mutation.go, line 575 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Not sure if this is done, I was talking about collecting fields from all types and then making the network call outside after this for loop.

Sorry, I misinterpreted what you meant in your original comment.

worker/mutation.go, line 598 at r4 (raw file):

Previously, pullrequest[bot] wrote…

Could there be an expectation of continuing here or this all or nothing? The return after an error says yes. The way the code is written it looks like it could also be desirable to queue up several of these errors. This may result in a better experience instead of discovering the failures 1x1.

I don't think it's possible to queue multiple errors and trying to add all errors into a single message might make the message too big so I'll keep it as is for now.

golangcibot · 2019-10-07T23:57:49Z

worker/mutation.go

+		for _, field := range t.Fields {
+			fieldName := field.Predicate
+			if len(fieldName) > 0 && fieldName[0] == '~' {
+				fieldName = fieldName[1:len(fieldName)]


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

Suggested change

fieldName = fieldName[1:len(fieldName)]

fieldName = fieldName[1:]

manishrjain

A couple of comments.

Reviewed 3 of 7 files at r1, 1 of 2 files at r3, 1 of 3 files at r4, 3 of 3 files at r5.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @golangcibot, @martinmr, and @pullrequest[bot])

worker/mutation.go, line 575 at r2 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

Done.

schemas?

worker/mutation.go, line 562 at r5 (raw file):

func verifyTypes(ctx context.Context, m *pb.Mutations) error {
	// Create a set of all the predicates included in this schema request.
	preds := make(map[string]struct{}, len(m.Schema))

reqPreds

worker/mutation.go, line 569 at r5 (raw file):

	// Create a set of all the predicates already present in the schema.
	schemaSet := make(map[string]struct{})
	fields := make([]string, 0)

var fields []string

worker/mutation.go, line 577 at r5 (raw file):

		for _, field := range t.Fields {
			fieldName := field.Predicate
			if len(fieldName) > 0 && fieldName[0] == '~' {

if len(fieldName) == 0 {
continue
}

worker/mutation.go, line 590 at r5 (raw file):

	schs, err := GetSchemaOverNetwork(ctx, &pb.SchemaRequest{Predicates: fields})
	if err != nil {
		return errors.Errorf("Cannot retrieve predicate information")

Never lose the original error. Must be wrapf or similar.

worker/mutation.go, line 592 at r5 (raw file):

		return errors.Errorf("Cannot retrieve predicate information")
	}
	for _, schemaNode := range schs {

Declare schemaSet just above this. As close as possible to usage.

martinmr

Reviewable status: 7 of 8 files reviewed, 15 unresolved discussions (waiting on @golangcibot, @manishrjain, and @pullrequest[bot])

worker/mutation.go, line 575 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

schemas?

Done

worker/mutation.go, line 562 at r5 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

reqPreds

Done.

worker/mutation.go, line 569 at r5 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

var fields []string

Done.

worker/mutation.go, line 577 at r5 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

if len(fieldName) == 0 {
continue
}

The check already exists inside the typeSanityCheck method so I've changed the code so that it's called before this line.

worker/mutation.go, line 578 at r5 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

File is not gofmt-ed with -s (from gofmt)
				fieldName = fieldName[1:]

Done.

worker/mutation.go, line 590 at r5 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Never lose the original error. Must be wrapf or similar.

Done.

worker/mutation.go, line 592 at r5 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Declare schemaSet just above this. As close as possible to usage.

Done.

Verify that all the fields in a type exist in the schema.

0a74934

Also, move the existing verification to the same place so that all type verification is done at the same time.

pullrequest bot reviewed Oct 2, 2019

View reviewed changes

golangcibot reviewed Oct 2, 2019

View reviewed changes

worker/mutation.go Outdated Show resolved Hide resolved

Add missing return statement.

ae8a295

pullrequest bot reviewed Oct 2, 2019

View reviewed changes

worker/mutation.go Outdated Show resolved Hide resolved

worker/mutation.go Outdated Show resolved Hide resolved

worker/mutation.go Outdated Show resolved Hide resolved

martinmr added 2 commits October 2, 2019 12:46

address review comments.

9f84b3e

Merge remote-tracking branch 'origin/master' into martinmr/verify-types

6beed21

martinmr marked this pull request as ready for review October 2, 2019 19:48

martinmr requested review from manishrjain, pawanrawal and a team as code owners October 2, 2019 19:48

pawanrawal suggested changes Oct 3, 2019

View reviewed changes

Address review comments.

b1ca8a6

martinmr commented Oct 3, 2019

View reviewed changes

martinmr requested a review from pawanrawal October 4, 2019 16:57

pullrequest bot reviewed Oct 4, 2019

View reviewed changes

worker/mutation.go Outdated Show resolved Hide resolved

dgraph/cmd/alpha/run_test.go Show resolved Hide resolved

pullrequest bot reviewed Oct 4, 2019

View reviewed changes

systest/mutations_test.go Outdated Show resolved Hide resolved

worker/mutation.go Show resolved Hide resolved

systest/mutations_test.go Show resolved Hide resolved

pawanrawal approved these changes Oct 7, 2019

View reviewed changes

martinmr added 2 commits October 7, 2019 15:12

Address review comments.

d9a0f10

Merge remote-tracking branch 'origin/master' into martinmr/verify-types

69454e5

martinmr commented Oct 7, 2019

View reviewed changes

Properly handle reverse predicates in types.

da7f8ea

golangcibot reviewed Oct 7, 2019

View reviewed changes

manishrjain approved these changes Oct 8, 2019

View reviewed changes

Address Manish's comments.

bc26b6e

martinmr commented Oct 8, 2019

View reviewed changes

fix deepsource warnings.

c03f1ab

martinmr merged commit 11fe54c into master Oct 8, 2019

martinmr deleted the martinmr/verify-types branch October 8, 2019 17:09

martinmr mentioned this pull request Oct 29, 2019

Validate types when adding them. #4110

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify that all the fields in a type exist in the schema. #4114

Verify that all the fields in a type exist in the schema. #4114

martinmr commented Oct 2, 2019 •

edited by manishrjain

Loading

pullrequest bot left a comment

pullrequest bot left a comment

pawanrawal left a comment

martinmr left a comment

pullrequest bot left a comment

pawanrawal left a comment

martinmr left a comment

golangcibot Oct 7, 2019

manishrjain left a comment

martinmr left a comment

	fieldName = fieldName[1:len(fieldName)]
	fieldName = fieldName[1:]

Verify that all the fields in a type exist in the schema. #4114

Verify that all the fields in a type exist in the schema. #4114

Conversation

martinmr commented Oct 2, 2019 • edited by manishrjain Loading

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot left a comment

Choose a reason for hiding this comment

pawanrawal left a comment

Choose a reason for hiding this comment

martinmr left a comment

Choose a reason for hiding this comment

pullrequest bot left a comment

Choose a reason for hiding this comment

pawanrawal left a comment

Choose a reason for hiding this comment

martinmr left a comment

Choose a reason for hiding this comment

golangcibot Oct 7, 2019

Choose a reason for hiding this comment

manishrjain left a comment

Choose a reason for hiding this comment

martinmr left a comment

Choose a reason for hiding this comment

martinmr commented Oct 2, 2019 •

edited by manishrjain

Loading