Dgraph: Client.Mutate never commits, so a mutation without CommitNow is silently dropped
- 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
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