Skip to content

fix(core): add additional checks in verifyUniqueWithinMutation for pruned mutations #9450

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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

matthewmcneely
Copy link
Member

@matthewmcneely matthewmcneely commented Jun 20, 2025

Description

This PR adds additional checks in edgraph/server.go::verifyUniqueWithinMutation for mutations that may have been conditionally pruned in updateMutations (called just before).

Also a basic test verifies not only the prevention of the panic but also assures unpruned mutations were applied. Note this is an improvement upon #9449 in that additional range and inner loop predicate checks were required (@gooohgb).

Assigning to @shivaji-kharse as the original author of this function.

Checklist

  • Code compiles correctly and linting passes locally
  • For all code changes, an entry added to the CHANGELOG.md file describing and linking to
    this PR
  • Tests added for new functionality, or regression tests for bug fixes added as applicable

@Copilot Copilot AI review requested due to automatic review settings June 20, 2025 20:17
@matthewmcneely matthewmcneely requested a review from a team as a code owner June 20, 2025 20:17
@github-actions github-actions bot added area/testing Testing related issues go Pull requests that update Go code labels Jun 20, 2025
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enhances mutation verification by skipping pruned entries, enriches error context in binary copying, and adds a regression test to prevent panics when unique predicates are conditionally pruned.

  • Skip out-of-bounds or nil entries in verifyUniqueWithinMutation to handle pruned mutations.
  • Include source and destination paths in the copyBinary error message.
  • Add TestWithConditionallyPrunedMutations to verify no panic and correct UID assignments.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
edgraph/server.go Added bounds checks and updated duplicate-value logic in verifyUniqueWithinMutation.
dgraphtest/image.go Augmented copyBinary error wrap to include toPath and fromPath.
dgraph/cmd/alpha/run_test.go Introduced a test to cover conditionally pruned mutations and ensure stability.
Comments suppressed due to low confidence (1)

edgraph/server.go:2173

  • [nitpick] Consider using %q or %s verbs instead of %v when formatting strings to ensure proper quoting and improve readability, e.g., "could not insert duplicate value %q for predicate %q".
					pred1Value, pred1.Predicate)

Copy link

trunk-io bot commented Jun 20, 2025

Static BadgeStatic BadgeStatic BadgeStatic Badge

View Full Report ↗︎Docs

@mangalaman93 mangalaman93 disabled auto-merge June 23, 2025 09:13
@mangalaman93 mangalaman93 enabled auto-merge (squash) June 23, 2025 09:14
@mangalaman93 mangalaman93 disabled auto-merge June 23, 2025 09:15
@mangalaman93 mangalaman93 enabled auto-merge (squash) June 23, 2025 09:15
@matthewmcneely matthewmcneely force-pushed the matthewmcneely/test-for-panic-with-conditionally-pruned-mutations branch from ecff40b to 908de7a Compare June 23, 2025 16:12
@matthewmcneely matthewmcneely force-pushed the matthewmcneely/test-for-panic-with-conditionally-pruned-mutations branch from c699011 to 672bca4 Compare June 25, 2025 16:38
@matthewmcneely matthewmcneely force-pushed the matthewmcneely/test-for-panic-with-conditionally-pruned-mutations branch from 672bca4 to 1003aa2 Compare June 25, 2025 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Documentation related issues. area/testing Testing related issues go Pull requests that update Go code
Development

Successfully merging this pull request may close these issues.

3 participants