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

Overwrite values for uid predicates #4883

Merged
merged 11 commits into from
Mar 12, 2020
Merged

Conversation

martinmr
Copy link
Contributor

@martinmr martinmr commented Mar 5, 2020

When the warning to delete and re-add the value was removed, non-list uid
predicates were not being overwritten. This fixes this by iterating over the list
and replacing theexisting postings with a copy with the operation set to a delete.

It also adds a test to prevent this from happening again.

Fixes #4879


This change is Reviewable

Docs Preview: Dgraph Preview

@martinmr martinmr requested review from manishrjain and a team as code owners March 5, 2020 01:27
posting/list.go Outdated Show resolved Hide resolved
Copy link
Contributor

@pawanrawal pawanrawal left a comment

Choose a reason for hiding this comment

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

@martinmr could you please see why the CI is failing?

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @manishrjain and @martinmr)

posting/list.go Outdated Show resolved Hide resolved
posting/list.go Outdated Show resolved Hide resolved
@martinmr martinmr force-pushed the martinmr/overwrite-uid-val branch from b5d1cd8 to bb470e9 Compare March 6, 2020 19:46
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.

Fixed the failing tests.

Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @golangcibot, @manishrjain, and @pawanrawal)


posting/list.go, line 343 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

Error return value of l.iterate is not checked (from errcheck)

Done.


posting/list.go, line 441 at r2 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

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

		pk.IsData() && mpost.Op == Set && mpost.PostingType == pb.Posting_REF

Done.


posting/list.go, line 441 at r3 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

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

		pk.IsData() && mpost.Op == Set && mpost.PostingType == pb.Posting_REF

Done.

@martinmr martinmr requested a review from pawanrawal March 9, 2020 20:14
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:

Reviewed 1 of 2 files at r1, 1 of 1 files at r5.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @golangcibot and @martinmr)


posting/list.go, line 339 at r5 (raw file):

	if singleUidUpdate {
		// This handles the special case when adding a value to predicates of type uid.

We have to do this, because the UID for a uid predicate would not be MaxUint64, as is the case for other non-list scalars. So, we need to delete the previous whatever UID is there and set this one.


posting/list.go, line 451 at r5 (raw file):

			hex.EncodeToString(l.key), mpost)
	}

Ludicrous mode return might have to be moved here.

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.

Add a bunch of comments.

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

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.

Added more comments.

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


posting/list.go, line 339 at r5 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

We have to do this, because the UID for a uid predicate would not be MaxUint64, as is the case for other non-list scalars. So, we need to delete the previous whatever UID is there and set this one.

Done.


posting/list.go, line 451 at r5 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Ludicrous mode return might have to be moved here.

Done.

@martinmr martinmr merged commit 9062d3b into master Mar 12, 2020
@martinmr martinmr deleted the martinmr/overwrite-uid-val branch March 12, 2020 23:41
martinmr added a commit that referenced this pull request Mar 17, 2020
When the warning to delete and re-add the value was removed, non-list uid
predicates were not being overwritten. This fixes this by iterating over the list
and replacing theexisting postings with a copy with the operation set to a delete.

It also adds a test to prevent this from happening again.

Fixes #4879
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.

List value returned for a non-list uid predicate
4 participants