diff --git a/parent_chain_test.go b/parent_chain_test.go new file mode 100644 index 0000000..5fe8948 --- /dev/null +++ b/parent_chain_test.go @@ -0,0 +1,262 @@ +package dalgo2ghingitdb + +// parent_chain_test.go is the regression suite for the per-parent ("per-space") +// scoping fix. Before the fix the adapter mapped a dal.Key to an in-repo path +// using only the leaf collection + record id, dropping the parent chain, so two +// keys that share a leaf (e.g. contacts/c1) but live under different parents +// (spaces/family vs spaces/work) collided on the same file and clobbered each +// other. After the fix each nested record is scoped under its parent record's +// directory (spaces//contacts/...), so they no longer collide. + +import ( + "context" + "encoding/base64" + "encoding/json" + "net/http" + "net/http/httptest" + "path" + "strings" + "testing" + + "github.com/dal-go/dalgo/dal" + + "github.com/ingitdb/ingitdb-go/ingitdb" +) + +// newStatefulContentsServer returns an httptest server that emulates the GitHub +// contents API with real read-after-write semantics: a PUT stores the decoded +// content keyed by its repo path, and a subsequent GET of that path serves it +// back (404 until first written). Unlike newGitHubContentsServer it does not +// gate GET on a pre-seeded fixture, so it can round-trip freshly written paths — +// which is exactly what the per-parent scoping regression needs. +func newStatefulContentsServer(t *testing.T) *httptest.Server { + t.Helper() + contents := make(map[string][]byte) + const pathPrefix = "/api/v3/repos/ingitdb/ingitdb-cli/contents/" + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if !strings.HasPrefix(r.URL.Path, pathPrefix) { + http.NotFound(w, r) + return + } + repoPath := strings.TrimPrefix(r.URL.Path, pathPrefix) + w.Header().Set("Content-Type", "application/json") + switch r.Method { + case http.MethodPut: + var body map[string]any + if err := json.NewDecoder(r.Body).Decode(&body); err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + decoded, err := base64.StdEncoding.DecodeString(body["content"].(string)) + if err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + contents[repoPath] = decoded + _ = json.NewEncoder(w).Encode(map[string]any{ + "content": map[string]any{"name": path.Base(repoPath), "path": repoPath, "sha": "sha-" + repoPath}, + }) + case http.MethodGet: + content, ok := contents[repoPath] + if !ok { + http.NotFound(w, r) + return + } + _ = json.NewEncoder(w).Encode(map[string]any{ + "type": "file", "encoding": "base64", + "content": base64.StdEncoding.EncodeToString(content), + "sha": "sha-" + repoPath, "name": path.Base(repoPath), "path": repoPath, + }) + default: + http.Error(w, "unexpected method "+r.Method, http.StatusInternalServerError) + } + })) +} + +// spacesWithContactsSubDef returns a definition with a top-level "spaces" +// collection and a "contacts" subcollection nested under it. Both are +// single-record YAML collections with a {key}.yaml record file. +func spacesWithContactsSubDef() *ingitdb.Definition { + contacts := &ingitdb.CollectionDef{ + ID: "contacts", + DirPath: "spaces/contacts", // schema-declared; scoping overrides this per parent record + RecordFile: &ingitdb.RecordFileDef{ + Name: "{key}.yaml", + Format: "yaml", + RecordType: ingitdb.SingleRecord, + }, + Columns: map[string]*ingitdb.ColumnDef{ + "name": {Type: ingitdb.ColumnTypeString}, + }, + } + spaces := &ingitdb.CollectionDef{ + ID: "spaces", + DirPath: "spaces", + RecordFile: &ingitdb.RecordFileDef{ + Name: "{key}.yaml", + Format: "yaml", + RecordType: ingitdb.SingleRecord, + }, + Columns: map[string]*ingitdb.ColumnDef{ + "name": {Type: ingitdb.ColumnTypeString}, + }, + SubCollections: map[string]*ingitdb.CollectionDef{ + "contacts": contacts, + }, + } + return &ingitdb.Definition{ + Collections: map[string]*ingitdb.CollectionDef{ + "spaces": spaces, + }, + } +} + +// TestResolveScopedCollection_DistinctPathsForSameLeaf proves the core fix at +// the path-mapping layer: two keys that share a leaf collection + record id but +// differ in their parent chain resolve to DISTINCT in-repo paths. It also +// asserts that a top-level (non-nested) key still resolves to the flat, +// schema-declared path so existing behaviour is unchanged. +func TestResolveScopedCollection_DistinctPathsForSameLeaf(t *testing.T) { + t.Parallel() + def := spacesWithContactsSubDef() + + familyKey := dal.NewKeyWithParentAndID(dal.NewKeyWithID("spaces", "family"), "contacts", "c1") + workKey := dal.NewKeyWithParentAndID(dal.NewKeyWithID("spaces", "work"), "contacts", "c1") + + familyDef, err := resolveScopedCollection(def, familyKey.Collection(), familyKey.Parent()) + if err != nil { + t.Fatalf("resolveScopedCollection(family): %v", err) + } + workDef, err := resolveScopedCollection(def, workKey.Collection(), workKey.Parent()) + if err != nil { + t.Fatalf("resolveScopedCollection(work): %v", err) + } + + familyPath := resolveRecordPath(familyDef, "c1") + workPath := resolveRecordPath(workDef, "c1") + + wantFamily := "spaces/family/contacts/$records/c1.yaml" + wantWork := "spaces/work/contacts/$records/c1.yaml" + if familyPath != wantFamily { + t.Errorf("family path = %q, want %q", familyPath, wantFamily) + } + if workPath != wantWork { + t.Errorf("work path = %q, want %q", workPath, wantWork) + } + if familyPath == workPath { + t.Fatalf("regression: same-leaf keys under different parents collide on %q", familyPath) + } + + // Top-level key is untouched: flat schema-declared DirPath. + topKey := dal.NewKeyWithID("spaces", "family") + topDef, err := resolveScopedCollection(def, topKey.Collection(), topKey.Parent()) + if err != nil { + t.Fatalf("resolveScopedCollection(top-level): %v", err) + } + if got, want := resolveRecordPath(topDef, "family"), "spaces/$records/family.yaml"; got != want { + t.Errorf("top-level path = %q, want %q", got, want) + } +} + +// TestResolveScopedCollection_DeepNesting exercises the intermediate-ancestor +// loop: a grandchild collection (spaces/{s}/projects/{p}/tasks/{t}) must +// interleave every parent-record id and subcollection name into the path. +func TestResolveScopedCollection_DeepNesting(t *testing.T) { + t.Parallel() + tasks := &ingitdb.CollectionDef{ + ID: "tasks", DirPath: "ignored", + RecordFile: &ingitdb.RecordFileDef{Name: "{key}.yaml", Format: "yaml", RecordType: ingitdb.SingleRecord}, + } + projects := &ingitdb.CollectionDef{ + ID: "projects", DirPath: "ignored", + RecordFile: &ingitdb.RecordFileDef{Name: "{key}.yaml", Format: "yaml", RecordType: ingitdb.SingleRecord}, + SubCollections: map[string]*ingitdb.CollectionDef{"tasks": tasks}, + } + spaces := &ingitdb.CollectionDef{ + ID: "spaces", DirPath: "spaces", + RecordFile: &ingitdb.RecordFileDef{Name: "{key}.yaml", Format: "yaml", RecordType: ingitdb.SingleRecord}, + SubCollections: map[string]*ingitdb.CollectionDef{"projects": projects}, + } + def := &ingitdb.Definition{Collections: map[string]*ingitdb.CollectionDef{"spaces": spaces}} + + // key: spaces/s1/projects/p1/tasks/t1 + parent := dal.NewKeyWithParentAndID(dal.NewKeyWithParentAndID(dal.NewKeyWithID("spaces", "s1"), "projects", "p1"), "tasks", "t1").Parent() + colDef, err := resolveScopedCollection(def, "tasks", parent) + if err != nil { + t.Fatalf("resolveScopedCollection: %v", err) + } + if got, want := resolveRecordPath(colDef, "t1"), "spaces/s1/projects/p1/tasks/$records/t1.yaml"; got != want { + t.Errorf("deep nested path = %q, want %q", got, want) + } +} + +// TestResolveScopedCollection_Errors covers the error branches: nil definition, +// unknown root collection, and an unknown subcollection under a known parent. +func TestResolveScopedCollection_Errors(t *testing.T) { + t.Parallel() + def := spacesWithContactsSubDef() + + if _, err := resolveScopedCollection(nil, "contacts", dal.NewKeyWithID("spaces", "x")); err == nil { + t.Error("nil definition: expected error, got nil") + } + + // Unknown root collection with a parent. + if _, err := resolveScopedCollection(def, "contacts", dal.NewKeyWithID("unknown", "x")); err == nil || + !strings.Contains(err.Error(), "not found in definition") { + t.Errorf("unknown root: got %v, want 'not found in definition'", err) + } + + // Unknown subcollection under a known parent record. + if _, err := resolveScopedCollection(def, "widgets", dal.NewKeyWithID("spaces", "family")); err == nil || + !strings.Contains(err.Error(), "not found in definition") { + t.Errorf("unknown subcollection: got %v, want 'not found in definition'", err) + } +} + +// TestGitHubDB_NestedKeys_ScopedByParentRecord is the end-to-end round-trip +// regression: write a contact "c1" under spaces/family and a different contact +// "c1" under spaces/work through the real Set path, then read both back. Before +// the fix the second write clobbered the first (both mapped to +// contacts/$records/c1.yaml) so both reads would return "Bob"; after the fix +// each is physically scoped under its parent space and round-trips independently. +func TestGitHubDB_NestedKeys_ScopedByParentRecord(t *testing.T) { + t.Parallel() + // Stateful fake: PUTs are stored and served back on GET, so a write→read + // round-trip works and a collision would be observable as a clobbered value. + server := newStatefulContentsServer(t) + defer server.Close() + + cfg := Config{Owner: "ingitdb", Repo: "ingitdb-cli", Ref: "main", APIBaseURL: server.URL + "/"} + db, err := NewGitHubDBWithDef(cfg, spacesWithContactsSubDef()) + if err != nil { + t.Fatalf("NewGitHubDBWithDef: %v", err) + } + + familyContact := dal.NewKeyWithParentAndID(dal.NewKeyWithID("spaces", "family"), "contacts", "c1") + workContact := dal.NewKeyWithParentAndID(dal.NewKeyWithID("spaces", "work"), "contacts", "c1") + + ctx := context.Background() + if err := db.RunReadwriteTransaction(ctx, func(ctx context.Context, tx dal.ReadwriteTransaction) error { + if setErr := tx.Set(ctx, dal.NewRecordWithData(familyContact, map[string]any{"name": "Alice"})); setErr != nil { + return setErr + } + return tx.Set(ctx, dal.NewRecordWithData(workContact, map[string]any{"name": "Bob"})) + }); err != nil { + t.Fatalf("write nested contacts: %v", err) + } + + famRec := dal.NewRecordWithData(familyContact, map[string]any{}) + workRec := dal.NewRecordWithData(workContact, map[string]any{}) + if getErr := db.Get(ctx, famRec); getErr != nil { + t.Fatalf("get family contact: %v", getErr) + } + if getErr := db.Get(ctx, workRec); getErr != nil { + t.Fatalf("get work contact: %v", getErr) + } + if got := famRec.Data().(map[string]any)["name"]; got != "Alice" { + t.Errorf("family contact name: got %v, want Alice (records collided across parents?)", got) + } + if got := workRec.Data().(map[string]any)["name"]; got != "Bob" { + t.Errorf("work contact name: got %v, want Bob", got) + } +} diff --git a/scoped_collection.go b/scoped_collection.go new file mode 100644 index 0000000..5da831e --- /dev/null +++ b/scoped_collection.go @@ -0,0 +1,97 @@ +package dalgo2ghingitdb + +import ( + "fmt" + "path" + + "github.com/dal-go/dalgo/dal" + "github.com/ingitdb/ingitdb-go/ingitdb" +) + +// resolveScopedCollection resolves the collection definition for a (possibly +// nested) collection identified by its leaf name and the key of its parent +// record, returning a copy of that definition whose DirPath is scoped to the +// concrete parent-record path in the repo. +// +// On-disk / in-repo layout (Option A — mirrors Firestore's document/subcollection +// nesting so a repo stays human-browsable): a subcollection's data lives *under* +// its parent record's directory, interleaving parent-record ids with +// subcollection names: +// +// spaces/family/contacts/c1.yaml (key: spaces/family/contacts/c1) +// spaces/work/contacts/c1.yaml (key: spaces/work/contacts/c1) +// +// This is what keeps records physically scoped by their parent chain: two keys +// that share a leaf collection + record id but differ in their parent chain no +// longer collide (previously both mapped to the same flat contacts path and +// clobbered each other). +// +// A top-level collection (parent == nil) resolves exactly as before: a flat +// lookup in def.Collections with the schema-declared DirPath untouched. Only +// nested keys take the scoping path, so existing (non-nested) behaviour is +// unchanged. +// +// The returned CollectionDef is a shallow copy of the schema definition with +// only DirPath overridden; RecordFile, Columns, format, etc. still come from the +// subcollection's own schema so encoding/decoding is unaffected. Paths use +// forward slashes (path.Join) because inGitDB repo paths are always POSIX. +func resolveScopedCollection(def *ingitdb.Definition, collection string, parent *dal.Key) (*ingitdb.CollectionDef, error) { + if def == nil { + return nil, fmt.Errorf("definition is required") + } + + // Top-level collection: preserve the original flat behaviour verbatim, + // including the exact "collection %q not found in definition" message that + // callers (and tests) rely on. + if parent == nil { + colDef, ok := def.Collections[collection] + if !ok { + return nil, fmt.Errorf("collection %q not found in definition", collection) + } + return colDef, nil + } + + // Build the ancestor record chain from root down to the immediate parent. + // Each step is a (collection, recordID) pair describing one ancestor record. + type step struct { + col string + id string + } + var ancestors []step + for k := parent; k != nil; k = k.Parent() { + ancestors = append([]step{{col: k.Collection(), id: fmt.Sprintf("%v", k.ID)}}, ancestors...) + } + + // Descend from the root collection, interleaving parent-record ids and + // subcollection names into the in-repo path. + rootCol := ancestors[0].col + cur, ok := def.Collections[rootCol] + if !ok { + return nil, fmt.Errorf("collection %q not found in definition", rootCol) + } + dir := cur.DirPath + // Walk the intermediate ancestors (root's children, grandchildren, ...). + for i := 1; i < len(ancestors); i++ { + parentID := ancestors[i-1].id + subColID := ancestors[i].col + sub, ok := cur.SubCollections[subColID] + if !ok { + return nil, fmt.Errorf("subcollection %q under %q not found in definition", subColID, cur.ID) + } + dir = path.Join(dir, parentID, subColID) + cur = sub + } + + // Finally descend into the target collection as a subcollection of the + // deepest ancestor record. + parentID := ancestors[len(ancestors)-1].id + target, ok := cur.SubCollections[collection] + if !ok { + return nil, fmt.Errorf("subcollection %q under %q not found in definition", collection, cur.ID) + } + dir = path.Join(dir, parentID, collection) + + scoped := *target + scoped.DirPath = dir + return &scoped, nil +} diff --git a/tx_readonly.go b/tx_readonly.go index 1f7986b..6b182c1 100644 --- a/tx_readonly.go +++ b/tx_readonly.go @@ -24,13 +24,10 @@ func (r readonlyTx) Get(ctx context.Context, record dal.Record) error { if r.db.def == nil { return fmt.Errorf("definition is required") } - key := record.Key() - collectionID := key.Collection() - colDef, ok := r.db.def.Collections[collectionID] - if !ok { - return fmt.Errorf("collection %q not found in definition", collectionID) + colDef, recordKey, err := r.resolveCollection(record.Key()) + if err != nil { + return err } - recordKey := fmt.Sprintf("%v", key.ID) recordPath := resolveRecordPath(colDef, recordKey) switch colDef.RecordFile.RecordType { case ingitdb.SingleRecord: @@ -122,13 +119,18 @@ func (r readonlyTx) resolveCollection(key *dal.Key) (*ingitdb.CollectionDef, str if r.db.def == nil { return nil, "", fmt.Errorf("definition is required") } - collectionID := key.Collection() - colDef, ok := r.db.def.Collections[collectionID] - if !ok { - return nil, "", fmt.Errorf("collection %q not found in definition", collectionID) + if key == nil { + return nil, "", fmt.Errorf("key is nil") + } + // resolveScopedCollection walks the key's parent chain so nested records are + // physically scoped under their parent record (spaces/family/contacts/...). + // For a top-level key (no parent) it is a plain flat lookup, unchanged. + colDef, err := resolveScopedCollection(r.db.def, key.Collection(), key.Parent()) + if err != nil { + return nil, "", err } if colDef.RecordFile == nil { - return nil, "", fmt.Errorf("collection %q has no record file", collectionID) + return nil, "", fmt.Errorf("collection %q has no record file", key.Collection()) } recordKey := fmt.Sprintf("%v", key.ID) return colDef, recordKey, nil