gofr-dev / gofr-dev/gofr

Dgraph: Client.Mutate never commits, so a mutation without CommitNow is silently dropped

Open
#4,157 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
20.9k
Forks
1.8k
Avg merge
5d 18h
Merged PRs (30d)
39

Description

### Summary

`Client.Mutate` opens a transaction, mutates, and returns — it never calls `Commit` or `Discard`. Whether the write lands is decided entirely by a field the **caller** sets on the `*api.Mutation`. If `CommitNow` is not set, the mutation is staged into a transaction that is then abandoned, the call returns `err == nil` with a non-nil response, and **nothing is persisted**.

`pkg/gofr/datasource/dgraph/dgraph.go:220`:

```go
func (d *Client) Mutate(ctx context.Context, mu any) (any, error) {
...
resp, err := d.client.NewTxn().Mutate(tracedCtx, mutation)
...
}
```

No `Commit`. No `Discard`.

### Why the behaviour depends on the caller

Verified against the pinned `github.com/dgraph-io/dgo/v210 v210.0.0-20230328113526-b66f8ae53a2d`:

```go
// txn.go:153
func (txn *Txn) Mutate(ctx context.Context, mu *api.Mutation) (*api.Response, error) {
req := &api.Request{
StartTs: txn.context.StartTs,
Mutations: []*api.Mutation{mu},
CommitNow: mu.CommitNow, // <- taken from the caller's mutation
}
return txn.Do(ctx, req)
}

// txn.go:205
if err == nil {
if req.CommitNow { // <- transaction only finishes when set
txn.finished = true
}
err = txn.mergeContext(resp.GetTxn())
return resp, err
}
```

| caller | outcome |
|---|---|
| sets `CommitNow: true` | committed — works |
| omits `CommitNow` | **staged, never committed, no error returned** |

### Reproduction

Against a real Dgraph v21.03.0, using `Client` exactly as an application would (full output below is verbatim, stable over 3 consecutive runs with `drop_all` between):

```go
// A: no CommitNow — what pkg/gofr/migration/dgraph.go:165 does today
respA, errA := c.Mutate(ctx, &api.Mutation{SetJson: []byte(`{"probe":"NO_COMMITNOW"}`)})

// B: with CommitNow — what docs/datasources/dgraph/page.md:117 does
respB, errB := c.Mutate(ctx, &api.Mutation{SetJson: []byte(`{"probe":"WITH_COMMITNOW"}`), CommitNow: true})
```

```
A) Mutate WITHOUT CommitNow -> err=, resp non-nil=true
B) Mutate WITH CommitNow -> err=, resp non-nil=true
PERSISTED: without CommitNow = 0, with CommitNow = 1
```

Both calls report success. Only one wrote anything.

### Scope — deliberately not overstated

The documented example at `docs/datasources/dgraph/page.md:117` **does** set `CommitNow: true`, so a user who copies the docs is unaffected. This is not "all Dgraph writes are lost".

What it is: **a caller who omits `CommitNow` gets a silent no-op**, and nothing in the signature

```go
Mutate(ctx context.Context, mu any) (any, error)
```

or in the method's doc comment (`// Mutate executes a write operation (mutation) in the Dgraph database and returns the result.`) indicates that a field on the argument decides whether the write happens.

That GoFr's own migration code was exactly such a caller is the evidence that this is a real trap rather than a theoretical one — `pkg/gofr/migration/dgraph.go:165` on `development`:

```go
_, err = c.DGraph.Mutate(context.Background(), &api.Mutation{
SetJson: jsonPayload,
})
```

No `CommitNow`. That is one of three reasons Dgraph migrations never persisted; #3186 fixes the migration path by calling `NewTxn()` directly and committing, which leaves the public method as it is.

### Secondary: leaked transaction

On the non-committing path there is also no `Discard`, so an open transaction is left on the server for every such call until Dgraph times it out.

### Suggested fix

Either shape closes it; the second matches what #3186 just did in the migrator:

1. Default `CommitNow` to true in `Client.Mutate` when the caller left it unset — smallest change, matches what the docs already tell people to do, and makes the single-mutation call atomic by default.
2. `NewTxn()` + `Mutate` + explicit `Commit`, with a deferred `Discard`.

Whichever is chosen, the doc comment should say what the commit semantics are, and `docs/datasources/dgraph/page.md` should stop relying on the reader noticing `CommitNow` in a code sample.

If the intent is genuinely to expose an uncommitted transaction, then the method needs to return something the caller can commit — returning `any` and silently dropping the write is the part that cannot stay either way.

### Environment

- gofr `development` @ `dacb55c9d`
- `github.com/dgraph-io/dgo/v210 v210.0.0-20230328113526-b66f8ae53a2d`
- Dgraph v21.03.0

### Related

- #3186 — fixes the migration path around this; does not change `Client.Mutate`

Contributor guide

Open the contributing guide

Research direction

Start with pkg/gofr/datasource/dgraph/dgraph.go:220 and compare its Mutate behavior with pkg/gofr/migration/dgraph.go:165 and the change in #3186. Review docs/datasources/dgraph/page.md:117 and the pinned dgo transaction behavior, then reproduce both CommitNow cases against Dgraph. Done means omitted CommitNow no longer silently loses the mutation or leaks the transaction, with the commit semantics documented.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
api, database
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.