Implement graveler condition functionality #9566

guy-har · 2025-10-05T22:25:45Z

Changes for Condition Support

This PR introduces infrastructure for conditional writes with preconditions:

Graveler Layer:

Added ConditionFunc type for validating values before write operations
Added WithCondition(ConditionFunc) helper to integrate conditions into Set operations
The Set method now validates conditions before committing changes

Catalog Layer:

Added EntryCondition adapter that converts Entry-level conditions to graveler.ConditionFunc
Enables Entry-based validation logic (e.g., ETag checks) to work with graveler's conditional operations

Factory Layer:

Added BuildConditionFromParams(apigen.UploadObjectParams) function that builds conditions from API parameters
Currently returns nil (no-op) but provides extension point

Controller Layer:

Updated upload handlers to use factory.BuildConditionFromParams
Applies conditions via graveler.WithCondition when present

github-actions · 2025-10-05T22:26:26Z

📚 Documentation preview at https://pr-9566.docs-lakefs-preview.io/

(Updated: 10/8/2025, 1:52:05 PM - Commit: cc7ea2b)

itaigilo

Good solution for a tricky problem.

Requesting changes mainly since this is a delicate part of the code and I'd be happy to make it more readable to someone who isn't deep in the details -
But most of my comments are about it's about style or nit.

itaigilo · 2025-10-14T07:56:17Z

pkg/api/controller.go

 	"github.com/go-openapi/swag"
 	"github.com/gorilla/sessions"
 	authacl "github.com/treeverse/lakefs/contrib/auth/acl"
+	"github.com/treeverse/lakefs/modules/api/factory"


The convention is:
apifactory "github.com/treeverse/lakefs/modules/api/factory"

itaigilo · 2025-10-14T08:00:57Z

pkg/graveler/graveler.go

 	}
 }

+type ConditionFunc func(currentValue *Value) error


Suggested change

type ConditionFunc func(currentValue *Value) error

type ConditionFunc func(currentValue *Value) error

itaigilo · 2025-10-14T09:21:18Z

modules/api/factory/build.go

+
+// BuildConditionFromParams creates a graveler.ConditionFunc from upload params.
+// Returns nil if no precondition is specified in the params.
+func BuildConditionFromParams(params apigen.UploadObjectParams) *graveler.ConditionFunc {


Why it's in the api factory and not in the catalog factory?

Asking since it creates a graveler dependency here, where in the catalog package it already exists.

controller (pkg not just module) is exposed to graveler, on the other hand catalog is not exposed to apigen due to abstraction.
There's no perfect solution here but I believe this is the better one

itaigilo · 2025-10-14T09:29:07Z

pkg/catalog/catalog.go

+// EntryCondition adapts an Entry-level condition function to a graveler.ConditionFunc.
+// It converts graveler Values to Entries before applying the condition, enabling Entry-based
+// validation logic (e.g., Object metadata checks) to work with graveler's conditional operations.
+func EntryCondition(conditionFunc func(*Entry) error) graveler.ConditionFunc {


Nit: name the param condition instead of conditionFunc.

Its type is already func, so it's redundant.

If you insist naming it conditionFunc, so EntryCondition should also be called EntryConditionFunc, since it's basically a wrapper doing a similar action, so should have a similar name.

itaigilo · 2025-10-14T09:33:17Z

pkg/graveler/graveler.go

 	log := g.log(ctx).WithFields(logging.Fields{"key": key, "operation": "set"})
 	err = g.safeBranchWrite(ctx, log, repository, branchID, safeBranchWriteOptions{MaxTries: options.MaxTries}, func(branch *Branch) error {
-		if !options.IfAbsent {
+		// Check if Condition is provided


Nit: you're not actually checking it here.

itaigilo · 2025-10-14T09:33:48Z

pkg/graveler/graveler.go

+		// Check if Condition is provided
+		hasCondition := options.Condition != nil
+
+		if !options.IfAbsent && !hasCondition {


Why IfAbsent is related to hasCondition?

itaigilo · 2025-10-14T09:36:17Z

pkg/graveler/graveler.go

+		// update stage with new value respecting the ifAbsent and condition
+		return g.StagingManager.Update(ctx, branch.StagingToken, key, func(stagingValue *Value) (*Value, error) {
+			var valueToCheck *Value
+			noValue := stagingValue == nil || stagingValue.Identity == nil


Why this is a var, and not part of the case?

itaigilo · 2025-10-14T09:37:38Z

pkg/graveler/graveler.go

+			switch {
+			case noValue:
+				// Nothing in staging, check against committed value
+				valueToCheck = curValue


Why not return here?
Is options.Condition() relevant for nil values?

itaigilo · 2025-10-14T09:44:45Z

pkg/graveler/graveler.go

-		return g.StagingManager.Update(ctx, branch.StagingToken, key, func(currentValue *Value) (*Value, error) {
-			if currentValue == nil || currentValue.Identity == nil {
-				return &value, nil
+		// update stage with new value respecting the ifAbsent and condition


Suggested change

// update stage with new value respecting the ifAbsent and condition

// update stage with new value respecting the IfAbsent and Condition

(The nittest nit, ofc.)

itaigilo · 2025-10-14T09:49:27Z

pkg/graveler/graveler.go

+			}
+
+			// Run condition if provided
+			if hasCondition {


It would be clearer to me if this was part of the switch, right after checking the IfAbsent. It would be a list of conditions to check.

N-o-Z

Thanks!
Added my comments - did not go over tests as I believe the changes required are going to modify them

N-o-Z · 2025-10-14T14:39:21Z

pkg/api/controller.go


+	var setOpts []graveler.SetOptionsFunc
+	// Handle If-Match precondition
+	if condition := factory.BuildConditionFromParams(params); condition != nil {


BuildConditionFromParams should return an error.
Since the logic and contents of params is an opaque for the controller the method should also validate the params and return error in case of conflict, bad values, etc.

N-o-Z · 2025-10-14T14:39:47Z

pkg/api/controller.go

 	}

+	var setOpts []graveler.SetOptionsFunc
+	// Handle If-Match precondition


This comment is irrelevant for this code and will quickly become obsolete and confusing.

N-o-Z · 2025-10-14T14:40:22Z

pkg/api/controller.go

 	entry := entryBuilder.Build()

-	err = c.Catalog.CreateEntry(ctx, repo.Name, branch, entry, graveler.WithIfAbsent(!allowOverwrite), graveler.WithForce(swag.BoolValue(params.Force)))
+	// Combine all set options


Redundant comment - the code is pretty clear

N-o-Z · 2025-10-14T14:47:37Z

modules/api/factory/build.go

+
+// BuildConditionFromParams creates a graveler.ConditionFunc from upload params.
+// Returns nil if no precondition is specified in the params.
+func BuildConditionFromParams(params apigen.UploadObjectParams) *graveler.ConditionFunc {


controller (pkg not just module) is exposed to graveler, on the other hand catalog is not exposed to apigen due to abstraction.
There's no perfect solution here but I believe this is the better one

N-o-Z · 2025-10-14T14:49:08Z

pkg/catalog/catalog.go

+// EntryCondition adapts an Entry-level condition function to a graveler.ConditionFunc.
+// It converts graveler Values to Entries before applying the condition, enabling Entry-based
+// validation logic (e.g., Object metadata checks) to work with graveler's conditional operations.
+func EntryCondition(conditionFunc func(*Entry) error) graveler.ConditionFunc {


I believe this should also handled the IfNoneMatch and consolidated the logic of condition handling

N-o-Z · 2025-10-14T14:50:27Z

pkg/graveler/graveler.go

+		hasCondition := options.Condition != nil
+
+		if !options.IfAbsent && !hasCondition {


Suggested change

hasCondition := options.Condition != nil

if !options.IfAbsent && !hasCondition {

hasCondition := options.Condition != nil

if !options.IfAbsent && !hasCondition {

N-o-Z · 2025-10-14T14:52:49Z

pkg/graveler/graveler.go

-			if currentValue == nil || currentValue.Identity == nil {
-				return &value, nil
+		// update stage with new value respecting the ifAbsent and condition
+		return g.StagingManager.Update(ctx, branch.StagingToken, key, func(stagingValue *Value) (*Value, error) {


This whole logic should be refactored to support all conditions under the condition callback. adding explicit conditions and a condition callback is making this code messy and unreadable

Implement graveler condition functionality

2212ccc

Minor fixes

cc7ea2b

guy-har marked this pull request as ready for review October 8, 2025 13:52

guy-har requested review from a team and N-o-Z October 8, 2025 13:53

itaigilo requested changes Oct 14, 2025

View reviewed changes

N-o-Z requested changes Oct 14, 2025

View reviewed changes

	type ConditionFunc func(currentValue *Value) error
	type ConditionFunc func(currentValue *Value) error

	// update stage with new value respecting the ifAbsent and condition
	// update stage with new value respecting the IfAbsent and Condition

		hasCondition := options.Condition != nil

		if !options.IfAbsent && !hasCondition {

Implement graveler condition functionality #9566

Are you sure you want to change the base?

Implement graveler condition functionality #9566

Uh oh!

Conversation

guy-har commented Oct 5, 2025

Uh oh!

github-actions bot commented Oct 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

itaigilo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

N-o-Z left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Oct 5, 2025 •

edited

Loading