Skip to content

Commit 8d573e4

Browse files
committed
Split Intent metadata off ImportConfigAdapter into IntentAdapter.
Stops JSON/XML from stubbing Orphan/SensitivePaths; Intent Get/GetAll return IntentAdapter while Running stays ImportConfigAdapter.
1 parent 7ced338 commit 8d573e4

22 files changed

Lines changed: 151 additions & 79 deletions

‎mocks/mockcacheclient/client.go‎

Lines changed: 7 additions & 7 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎mocks/mockcacheclient/clientbound.go‎

Lines changed: 7 additions & 7 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎pkg/cache/CONTEXT.md‎

Lines changed: 35 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,9 +20,39 @@ family (own accessors, own storage, excluded from `InstanceIntentGetAll`)
2020
under every backend, because it was never config-server's data to begin with.
2121
_Avoid_: calling it an intent, "the running intent" as a `Client`-level type.
2222

23+
**Orphan**:
24+
Deletion policy for an Intent: drop it from the intended store (stop
25+
managing it) without deleting the corresponding config on the device,
26+
leaving that config unmanaged. Wire `Orphan`, internal `onlyIntended` /
27+
`DeleteOnlyIntended`, and config-server `DeletionPolicy=orphan` are the
28+
same bit. _Avoid_: a second meaning for leaf-variant "orphan delete"
29+
(same policy); "orphan" for unowned tree nodes or "Intent not found".
30+
2331
**ImportConfigAdapter**:
24-
The mechanical, backend-agnostic shape `Client`'s read-side methods return:
25-
"data ready to hand to `Tree.ImportConfig`." Every `Client.*Get`/`*GetAll`
26-
method returns this shape — including `InstanceRunningGet` — regardless of
27-
whether the underlying value is an `Intent` or `Running`, and regardless of
28-
how many concrete representations exist behind it for a given backend.
32+
The mechanical shape ready to hand to `Tree.ImportConfig` (tree walk plus
33+
import-needed metadata: name, priority, deletes, non-revertive). Returned
34+
directly by Running reads (`InstanceRunningGet`). Intent reads return
35+
`IntentAdapter`, which embeds this shape. _Avoid_: putting Orphan or path
36+
markers on this interface; those are Intent-only (see **IntentAdapter**).
37+
38+
**IntentAdapter**:
39+
The `Client` Intent-read shape: an `ImportConfigAdapter` plus Intent-only
40+
metadata (Orphan, path markers / SensitivePaths). Returned by
41+
`InstanceIntentGet` / `InstanceIntentGetAll`. Implemented by proto- and
42+
Document-backed adapters; never by JSON/XML device/running importers.
43+
_Avoid_: `IntentDescriptor` for this type; conflating with
44+
`IntentResponseAdapter` (northbound GetIntent response that also carries a
45+
Tree Entry).
46+
47+
**ConfigSnapshotService**:
48+
The canonical local gRPC seam name for config-server-backed intent reads and
49+
writes (`Get`/`List`/`Modify`/`Delete`) against `TargetSnapshot` data. _Avoid_:
50+
using the older `ConfigReadService` name for current behavior.
51+
52+
**Namespaced name**:
53+
Under `Cache.Type: config-server` only, the `namespace.name` spelling (split on
54+
the first `.`) used for a Target's datastore identity and for an Intent's
55+
owner / Config identity. The local backend has no namespaces and does not use
56+
this spelling. _Avoid_: treating this as a `Client`-level concept; "GVKNSN" as
57+
the everyday term (wire/comment echo of config-server); implying the local
58+
cache understands dotted names.

‎pkg/cache/cache.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,9 +25,9 @@ import (
2525
// it's the one capability every Cache.Type genuinely has data for.
2626
type IntentReader interface {
2727
InstanceIntentsList(ctx context.Context, cacheInstanceName string) ([]string, error)
28-
InstanceIntentGet(ctx context.Context, cacheName string, intentName string) (importer.ImportConfigAdapter, error)
28+
InstanceIntentGet(ctx context.Context, cacheName string, intentName string) (importer.IntentAdapter, error)
2929
InstanceIntentExists(ctx context.Context, cacheName string, intentName string) (bool, error)
30-
InstanceIntentGetAll(ctx context.Context, cacheName string, excludeIntentNames []string, intentChan chan<- importer.ImportConfigAdapter, errChan chan<- error)
30+
InstanceIntentGetAll(ctx context.Context, cacheName string, excludeIntentNames []string, intentChan chan<- importer.IntentAdapter, errChan chan<- error)
3131
}
3232

3333
// IntentWriter persists real Intents. Backends that don't own real-Intent

‎pkg/cache/cacheClientBound.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,9 @@ import (
2424
// BoundIntentReader is the bound-call equivalent of IntentReader.
2525
type BoundIntentReader interface {
2626
IntentsList(ctx context.Context) ([]string, error)
27-
IntentGet(ctx context.Context, intentName string) (importer.ImportConfigAdapter, error)
27+
IntentGet(ctx context.Context, intentName string) (importer.IntentAdapter, error)
2828
IntentExists(ctx context.Context, intentName string) (bool, error)
29-
IntentGetAll(ctx context.Context, excludeIntentNames []string, intentChan chan<- importer.ImportConfigAdapter, errChan chan<- error)
29+
IntentGetAll(ctx context.Context, excludeIntentNames []string, intentChan chan<- importer.IntentAdapter, errChan chan<- error)
3030
}
3131

3232
// BoundIntentWriter is the bound-call equivalent of IntentWriter.
@@ -83,7 +83,7 @@ func (c *CacheClientBoundImpl) InstanceExists(ctx context.Context) bool {
8383
func (c *CacheClientBoundImpl) IntentsList(ctx context.Context) ([]string, error) {
8484
return c.cacheClient.InstanceIntentsList(ctx, c.cacheName)
8585
}
86-
func (c *CacheClientBoundImpl) IntentGet(ctx context.Context, intentName string) (importer.ImportConfigAdapter, error) {
86+
func (c *CacheClientBoundImpl) IntentGet(ctx context.Context, intentName string) (importer.IntentAdapter, error) {
8787
return c.cacheClient.InstanceIntentGet(ctx, c.cacheName, intentName)
8888
}
8989
func (c *CacheClientBoundImpl) IntentModify(ctx context.Context, intent *tree_persist.Intent) error {
@@ -95,7 +95,7 @@ func (c *CacheClientBoundImpl) IntentDelete(ctx context.Context, intentName stri
9595
func (c *CacheClientBoundImpl) IntentExists(ctx context.Context, intentName string) (bool, error) {
9696
return c.cacheClient.InstanceIntentExists(ctx, c.cacheName, intentName)
9797
}
98-
func (c *CacheClientBoundImpl) IntentGetAll(ctx context.Context, excludeIntentNames []string, intentChan chan<- importer.ImportConfigAdapter, errChan chan<- error) {
98+
func (c *CacheClientBoundImpl) IntentGetAll(ctx context.Context, excludeIntentNames []string, intentChan chan<- importer.IntentAdapter, errChan chan<- error) {
9999
c.cacheClient.InstanceIntentGetAll(ctx, c.cacheName, excludeIntentNames, intentChan, errChan)
100100
}
101101
func (c *CacheClientBoundImpl) RunningGet(ctx context.Context) (importer.ImportConfigAdapter, error) {
Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
# Intent vs Running Client return-type split
2+
3+
**Status:** accepted
4+
5+
**Pairs with:** `.scratch/open/03-split-intent-metadata-off-import-adapter.md`
6+
7+
`ImportConfigAdapter` grew `GetOrphan` / `GetSensitivePaths` so cache backends could surface Intent metadata through the same type used for tree walk. JSON/XML forever stubbed those accessors; the import processor never reads them. Every new Intent-only field would force another stub on device/running importers — classic shallow widening.
8+
9+
**Decision:** narrow `ImportConfigAdapter` to tree walk plus import-needed metadata (name, priority, deletes, non-revertive). Introduce `IntentAdapter` (`ImportConfigAdapter` + `GetOrphan` / `GetSensitivePaths`) in `pkg/tree/importer`. Client Intent reads (`InstanceIntentGet` / `InstanceIntentGetAll`, and their bound equivalents) return `IntentAdapter`; Running reads (`InstanceRunningGet`) stay `ImportConfigAdapter`. `documentImporter` and `ProtoTreeImporter` implement `IntentAdapter`; JSON/XML do not.
10+
11+
## Considered options
12+
13+
- **Type-assert Orphan/SensitivePaths at Intent call sites.** Rejected: silent miss when a non-Intent adapter arrives; callers must remember to assert.
14+
- **Separate Client methods for Intent metadata.** Rejected: extra hop for every Intent Get; metadata belongs with the adapter already in hand.
15+
- **Keep a single return type; leave stubs on JSON/XML.** Rejected: fails the deletion test — every new Intent field forces another stub.
16+
- **Name `IntentDescriptor`.** Rejected: clashes with northbound response vocabulary; `IntentAdapter` mirrors `ImportConfigAdapter` and stays distinct from `IntentResponseAdapter`.
17+
- **House `IntentAdapter` under `pkg/cache`.** Rejected: would create a cache←tree reverse dependency or force a wrapper; it lives beside `ImportConfigAdapter` in `pkg/tree/importer`.
18+
19+
## Consequences
20+
21+
Intent-only metadata no longer pollutes device/running importers. `populateSensitivePathIndex` and GetIntent read Orphan/SensitivePaths through `IntentAdapter`. `configsnapshot.NewImportAdapter` returns `IntentAdapter`. Expanding GetIntentResponse / TransactionIntent / config-server wire is out of scope for this split.

‎pkg/cache/local.go‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -50,26 +50,27 @@ type LocalCache struct {
5050
}
5151

5252
// decodeIntent unmarshals the disk-backed store's raw bytes into a
53-
// *tree_persist.Intent and wraps it as an importer.ImportConfigAdapter — the
53+
// *tree_persist.Intent and wraps it as an importer.IntentAdapter — the
5454
// shared decode step every InstanceIntentGet/InstanceIntentGetAll/
55-
// InstanceRunningGet call site needs.
56-
func (l *LocalCache) decodeIntent(b []byte) (importer.ImportConfigAdapter, error) {
55+
// InstanceRunningGet call site needs. Running callers narrow to
56+
// ImportConfigAdapter at the RunningStore boundary.
57+
func (l *LocalCache) decodeIntent(b []byte) (importer.IntentAdapter, error) {
5758
result := &tree_persist.Intent{}
5859
if err := proto.Unmarshal(b, result); err != nil {
5960
return nil, err
6061
}
6162
return treeproto.NewProtoTreeImporter(result), nil
6263
}
6364

64-
func (l *LocalCache) InstanceIntentGet(ctx context.Context, cacheName string, intentName string) (importer.ImportConfigAdapter, error) {
65+
func (l *LocalCache) InstanceIntentGet(ctx context.Context, cacheName string, intentName string) (importer.IntentAdapter, error) {
6566
b, err := l.Cache.InstanceIntentGet(ctx, cacheName, intentName)
6667
if err != nil {
6768
return nil, err
6869
}
6970
return l.decodeIntent(b)
7071
}
7172

72-
func (l *LocalCache) InstanceIntentGetAll(ctx context.Context, cacheName string, excludeIntentNames []string, intentChanOrig chan<- importer.ImportConfigAdapter, errChanOrig chan<- error) {
73+
func (l *LocalCache) InstanceIntentGetAll(ctx context.Context, cacheName string, excludeIntentNames []string, intentChanOrig chan<- importer.IntentAdapter, errChanOrig chan<- error) {
7374
// create new channels
7475
intentChan := make(chan *types.Intent, 5)
7576
errChan := make(chan error, 1)

‎pkg/cache/local_test.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -64,9 +64,9 @@ func TestLocalCache_InstanceIntentGetRoundTrip(t *testing.T) {
6464
// TestLocalCache_RunningRoundTrip verifies InstanceRunningModify/InstanceRunningGet
6565
// are thin passthroughs to the same disk-backed store InstanceIntentGet/Modify use,
6666
// so the local backend's on-disk behavior for "running" is unchanged by the split.
67-
// InstanceRunningGet returns an importer.ImportConfigAdapter — the same mechanical
68-
// "ready for Tree.ImportConfig" shape InstanceIntentGet returns — rather than the
69-
// raw *tree_persist.Intent, so the assertions go through its accessors.
67+
// InstanceRunningGet returns an importer.ImportConfigAdapter (Intent reads return
68+
// IntentAdapter) rather than the raw *tree_persist.Intent, so the assertions go
69+
// through its accessors.
7070
func TestLocalCache_RunningRoundTrip(t *testing.T) {
7171
ctx := context.Background()
7272
lc := newTestLocalCache(t)
@@ -129,7 +129,7 @@ func TestLocalCache_RunningExcludedFromIntentGetAll(t *testing.T) {
129129
t.Fatalf("InstanceIntentModify: %v", err)
130130
}
131131

132-
intentChan := make(chan importer.ImportConfigAdapter)
132+
intentChan := make(chan importer.IntentAdapter)
133133
errChan := make(chan error, 1)
134134
go lc.InstanceIntentGetAll(ctx, cacheName, []string{consts.RunningIntentName}, intentChan, errChan)
135135

‎pkg/datastore/datastore_rpc.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -299,7 +299,7 @@ var _ types.RollbackInterface = &DatastoreRollbackAdapter{}
299299
// sensitive_paths into s. It is called once during Datastore startup, before
300300
// the first northbound read is served.
301301
func populateSensitivePathIndex(ctx context.Context, s *treetypes.SensitivePathIndex, cc cache.BoundIntentReader) error {
302-
return forEachIntent(ctx, cc, []string{consts.RunningIntentName}, func(intent importer.ImportConfigAdapter) error {
302+
return forEachIntent(ctx, cc, []string{consts.RunningIntentName}, func(intent importer.IntentAdapter) error {
303303
if len(intent.GetSensitivePaths()) > 0 {
304304
s.Set(intent.GetName(), intent.GetSensitivePaths())
305305
}

‎pkg/datastore/datastore_rpc_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ func TestPopulateSensitivePathIndex_NarrowIntentReader(t *testing.T) {
3232
reader := mockcacheclient.NewMockBoundIntentReader(ctrl)
3333
reader.EXPECT().
3434
IntentGetAll(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
35-
DoAndReturn(func(_ context.Context, _ []string, intentChan chan<- importer.ImportConfigAdapter, errChan chan<- error) {
35+
DoAndReturn(func(_ context.Context, _ []string, intentChan chan<- importer.IntentAdapter, errChan chan<- error) {
3636
intentChan <- treeproto.NewProtoTreeImporter(withPaths)
3737
intentChan <- treeproto.NewProtoTreeImporter(withoutPaths)
3838
close(intentChan)

0 commit comments

Comments
 (0)