-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
fix for deletion of reinserted causing a quad does not exist error #747
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for tracking this down. The change looks good, only a few minor comments:
graph/graphtest/graphtest.go
Outdated
for i := 0; i < 2; i++ { | ||
w.AddQuad(quad.Make("<bob>", "<follows>", "<sally>", nil)) | ||
err = w.RemoveQuad(quad.Make("<bob>", "<follows>", "<sally>", nil)) | ||
assert.Nil(t, err, "Remove quad failed") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert.Nil(t, err, "Remove quad failed") | |
require.NoError(t, err, "Remove quad failed") |
graph/graphtest/graphtest.go
Outdated
}) | ||
|
||
for i := 0; i < 2; i++ { | ||
w.AddQuad(quad.Make("<bob>", "<follows>", "<sally>", nil)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check an error when adding quads
graph/kv/indexing.go
Outdated
@@ -675,9 +675,10 @@ func (qs *QuadStore) hasPrimitive(ctx context.Context, tx BucketTx, p *proto.Pri | |||
if !get && unique { | |||
return p, nil | |||
} | |||
for _, x := range options { | |||
for ix := len(options) - 1; ix >= 0; ix-- { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using i
as a name is fine
for ix := len(options) - 1; ix >= 0; ix-- { | |
for i := len(options) - 1; i >= 0; i-- { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
When reinserting a previously deleted quad, and attempting to then delete it a quad does not exist error was given for the the kv stores, issue was retrieval of the primitive from the log was finding the prviously deleted, not the previously inserted quad. Change retrieval from the log to iterate in reverse to ensure the latest is retrieved when multiple hits. Added unit test.