Add LoadOrStore and LoadAndDelete (atomic insert-if-absent and read-and-consume)#26
Open
mlwelles wants to merge 40 commits into
Open
Add LoadOrStore and LoadAndDelete (atomic insert-if-absent and read-and-consume)#26mlwelles wants to merge 40 commits into
mlwelles wants to merge 40 commits into
Conversation
Add a generic typed layer over modusgraph.Client: typed.Client[T] with CRUD and iterators; a fluent Query[T] builder (filters, ordering, paging, edge traversal, IterNodes); MultiQuery for N homogeneous blocks in one round-trip; functional options; a filter DSL (typed/filter); and ordered result merging (typed/search). A small no-op-by-default Tracer seam (typed.SetTracer) lets a host plug in tracing without the typed package depending on any telemetry library. Self-contained: builds and tests against the current client with no other changes.
Add RetryPolicy / DefaultRetryPolicy and a runner that re-executes a function on aborted Dgraph transactions with exponential backoff (retry.go), exposed on the client via a WithRetry method.
Add the Schema interface (SchemaTypeName), the UnwrapSchema reflection helper, and the DgraphMapper interface (record.go). The client unwraps schema-defining values at the mutation and query boundary so generated wrapper types route to their backing schema struct. Plain structs do not implement Schema and are unaffected — UnwrapSchema is identity for them.
The unit-test job runs `go test -short`, which skips every test that needs a live Dgraph. Standing up a dgraph/standalone container (and setting MODUSGRAPH_TEST_ADDR) therefore adds setup the job never uses. Remove both; the integration and load suites keep their own dedicated jobs.
Add common local artifacts to .gitignore: editor config (.idea/, .vscode/), the built ./query binary, load_test benchmark JSON, and git worktrees.
Adds WithGRPCDialOption(opt grpc.DialOption), a general escape hatch for gRPC dial settings the dedicated options do not cover — TLS transport credentials, interceptors, keepalive, and so on — on remote (dgraph://) connections. The existing WithMaxRecvMsgSize is folded into the same dial-option assembly, so the two compose cleanly, and the client dedup key now counts the custom dial options so differently-configured clients are not merged. No change for embedded (file://) URIs.
Adds raw schema-DDL primitives that complement UpdateSchema's object-template inference: - Client.AlterSchema(ctx, schema) applies a raw DQL schema string directly, giving full control over predicate types, indexes, and directives — useful for migrations that declare predicates no Go type models yet. - Engine.dropPredicate deletes a single predicate (and its data) from the embedded engine via posting.DeletePredicate. - embedded_client.go routes an Alter carrying DropAttr to dropPredicate, so the embedded path matches a remote Dgraph cluster's DropAttr behavior. TestDropPredicateEmbedded exercises the full declare/insert/drop cycle against the embedded engine.
Adds SelfValidator, an opt-in seam that lets a type drive its own validation. When a value passed to Insert, Upsert, or Update implements SelfValidator, the client calls ValidateWith instead of handing the value straight to the configured StructValidator. validateStruct now routes each element through a new validateOne helper that detects SelfValidator (on the value or its address) and otherwise falls back to StructCtx exactly as before — behavior is unchanged for ordinary structs. This is the runtime seam generated entities use to validate unexported fields: the generated ValidateWith builds a mirror struct with exported fields the go-playground validator can read by reflection.
Adds SelfValidator, an opt-in seam that lets a type drive its own validation. When a value passed to Insert, Upsert, or Update implements SelfValidator, the client calls ValidateWith instead of handing the value straight to the configured StructValidator. This covers validation that struct tags cannot express on their own: cross-field rules (one field constrained by another), conditional rules, checks on computed or setter-derived values, and broader business rules. ValidateWith receives the configured StructValidator, so an implementation can still run ordinary tag-based checks and layer custom logic on top. validateStruct routes each element through a new validateOne helper that detects SelfValidator (on the value or its address) and otherwise falls back to StructCtx exactly as before — behavior is unchanged for ordinary structs.
Add TestLoadAndDeleteSingleWinner and serialize LoadAndDelete's read-then-delete critical section with a per-client mutex so exactly one in-process caller consumes a node. The embedded engine's commit path does no optimistic-concurrency conflict check, so the shared read-write transaction alone cannot abort losers; the lock guarantees single-winner semantics regardless of backend.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
10 issues found across 31 files
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
Addresses review feedback on the typed query builder: - combineAnd now parenthesizes each accumulated @filter fragment. Without this, a fragment containing OR ANDed with another fragment rendered as "a OR b AND c", which dgraph parses as "a OR (b AND c)" — silently widening results. dgman exposes only a string Filter(), so the builder must compose the expression itself; this makes that composition correct. - WhereEdge's pre-pass no longer discards a caller-set UID/RootFunc root. When a custom root is present, the matched UIDs are intersected via a uid() @filter instead of overwriting the root. - MultiQuery.Add rejects the same *Query[T] under two names; Execute names the underlying query in place, so reuse would corrupt block composition. - NodesAndCount now opens a tracing span, matching the other terminals. Tests: add a precedence regression, a WhereEdge+UID intersection test, and a duplicate-Query guard test; strengthen the filter-sequencing test to assert the exact expression; make the IterNodes laziness check delta-based. Docs: add package doc.go with a before/after narrative, runnable examples for the client/query builder/MultiQuery, and a verified filter example.
…Retries Addresses review feedback on the retry policy: - delay() now adds jitter before the final MaxDelay clamp, so the documented invariant "no single delay exceeds MaxDelay" holds. Previously the cap was applied before jitter, letting the delay reach MaxDelay*(1+Jitter). The exponential is also capped before the shift to avoid overflow at large attempt counts. - WithRetry clamps a negative MaxRetries to zero so fn always runs at least once, as documented, instead of skipping the loop and hitting the unreachable panic. Tests: - TestRetryPolicyDelayJitterNeverExceedsMaxCap asserts the cap invariant across attempts with large jitter. - TestWithRetryNegativeMaxRetries asserts fn runs once with MaxRetries=-1. - TestWithRetryContextCancellation now returns dgo.ErrAborted from fn so it actually enters the backoff sleep and exercises the ctx.Done() path it claims to cover (previously it returned ctx.Err() and bailed immediately). Docs: add runnable examples for WithRetry and a custom RetryPolicy.
…Schema Addresses review feedback on generated-schema-type routing: - UnwrapSchema no longer panics on a typed nil pointer: it returns the value untouched instead of invoking Unwrap on a nil receiver that may dereference. - UnwrapSchema now finds a pointer-receiver Unwrap on a wrapper passed by value, by looking the method up on an addressable copy (a value's method set excludes pointer-receiver methods). Tests: - Add unit tests for the typed-nil-pointer and value-wrapper cases. - Add an integration test (TestClientUnwrapsWrapperThroughRealMutation) that inserts a wrapper through the real client and reads it back, so a mutation method dropping its UnwrapSchema call is caught — the prior tests exercised UnwrapSchema in isolation through a local mock and would not catch that. Docs: add a runnable ExampleSchema showing the wrapper/Unwrap pattern.
Addresses review feedback on WithGRPCDialOption: - client.key() keyed custom gRPC dial options by count only, so two clients to the same URI with one different option each produced the same key and were merged in the dedup cache. The key now includes each option's runtime identity (grpc.DialOption is opaque and not comparable), so differently configured clients are never merged. The cache errs toward keeping clients apart rather than merging connections configured differently. - Dial options apply only to remote (dgraph://) connections, so they now contribute to the key only for remote URIs — consistent with the documented "ignored for file://" behavior. Tests: - TestKeyDistinguishesGRPCDialOptions now also asserts two clients with the same option count but different options get different keys. - TestKeyIgnoresGRPCDialOptionsForEmbedded asserts dial options do not affect the key for embedded (file://) clients. Docs: add a runnable ExampleWithGRPCDialOption (TLS credentials + keepalive).
Document the raw schema-DDL path with a runnable example showing predicate types, indexes, and directives applied directly via AlterSchema.
…d example - TestValidateSelfValidatorInSlice now asserts the configured StructValidator is never called for a SelfValidator slice element, matching the scalar test; a regression invoking both paths would otherwise pass silently. - Add a runnable ExampleSelfValidator showing a cross-field rule that layers on the configured StructValidator.
…oad-or-store # Conflicts: # client.go # self_validator_test.go
…eflection Addresses review feedback specific to LoadOrStore/LoadAndDelete: - LoadAndDelete took a process-wide mutex unconditionally, serializing consumers across all keys and backends. The mutex is only needed for the embedded engine (no commit-time conflict check); a remote Dgraph cluster aborts the loser of a commit conflict and the bounded retry already elects a single winner. The lock is now taken only for embedded clients (c.engine != nil), removing head-of-line blocking for remote consumers. - firstUpsertPredicate dereferenced reflect.Value without guarding a nil pointer or non-struct input, panicking on Type()/NumField(). It now returns "" for an invalid or non-struct value. Tests: - TestLoadOrStore / TestTypedLoadOrStore now assert the passed object is hydrated with the existing record (its UID) on the loaded=true path, so a regression returning loaded=true without populating fields is caught. Docs: add runnable ExampleClient_loadOrStore and ExampleClient_loadAndDelete. This branch also merges the updated feature branches, bringing in the review fixes for the typed query builder, retry policy, schema routing, gRPC dial options, and self-validation.
Wrap a >120-char error string in MultiQuery.Execute and a long test helper signature, and pre-allocate two test result slices. Resolves the Trunk golangci-lint findings; no behavior change.
Drop the int->uint shift conversion (Go allows a signed shift count, and attempt is bounded to [0,63)), clearing gosec G115. Annotate the jitter RNG with nolint:gosec — backoff jitter is not security-sensitive, so math/rand/v2 is appropriate. No behavior change.
The Trunk config pinned trivy@0.59.1, but that release does not exist on github.com/aquasecurity/trivy — Trunk's templated download (.../v0.59.1/trivy_0.59.1_Linux-64bit.tar.gz) returns HTTP 404, failing the Trunk Code Quality check on any PR whose diff trivy scans (e.g. workflow or broad changes) while reporting no actual lint issues. Bump to trivy@0.69.3, a real release with the expected Linux-64bit asset. The plugin (v1.6.7) downloads trivy via a version-templated URL, so no other change is needed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this adds
Two atomic key-keyed operations on
modusgraph.Client(and the typedClient[T]), plus the cumulative feature set this fork has been upstreaming.LoadOrStore— insert-if-absent. Atomically stores a node keyed by anupsert predicate, or reports that one already exists and hydrates the passed
object with it. The building block for "claim a one-time token."
LoadAndDelete— atomic read-and-consume. Reads a node and deletes it,electing a single winner under concurrency: exactly one caller gets
loaded=truewith the record, the rest getloaded=false. For consuming anonce, a pending job, or a single-use code.
Scope of this PR
This is the cumulative branch: it integrates every feature this fork is
upstreaming (typed client and query builder, retry policy, schema routing, gRPC
dial options, schema DDL, self-validation) and adds the consume operations on
top. The individual features have their own focused PRs (#17, #18, #20, #21,
#22, #23, #24, #25); this branch is where they compose, so its review surfaced
issues across all of them.
Review round — changes since first push
This branch now merges the review fixes from each feature branch and adds two
fixes specific to the consume operations. Addresses the automated review
(cubic):
LoadAndDeleteover-serialization (P2): the read-and-consume mutex wastaken unconditionally, serializing consumers across all keys and backends. It
is needed only for the embedded engine (no commit-time conflict check); a
remote cluster aborts the commit loser and the bounded retry already elects a
single winner. The lock is now taken only for embedded clients.
firstUpsertPredicate(P2): reflection dereferenced a nilpointer / non-struct without guarding, panicking on
Type(). It now returns""for invalid input.LoadOrStorehydration untested (P2): the test did not assert the passedobject is populated on the
loaded=truepath. It now asserts the existingnode's UID is hydrated.
keyed on each option's identity (carried in from feat: WithGRPCDialOption for custom gRPC dial settings #23).
MaxRetries/ jitter cap / cancellation test (P2): retry fixescarried in from feat: aborted-transaction retry policy and client integration #18.
UnwrapSchemanil pointer (P2): carried in from feat: recognize generated schema types (SchemaTypeName + UnwrapSchema) #20.NodesAndCounttracing (P2): carried in from feat(typed): generic, type-safe client and query builder #17.and feat: SelfValidator for custom and cross-field validation #25.
Tests
Full root and typed suites pass (
go test -short). New and strengthened testscover the consume hydration path, the single-winner race, and every carried-in
fix.
Documentation
Runnable
ExampleClient_loadOrStoreandExampleClient_loadAndDelete, plus theexamples and package docs from the merged feature branches.