diff --git a/.changeset/27803-composed-datasource-connector.md b/.changeset/27803-composed-datasource-connector.md new file mode 100644 index 00000000000..04ae0a73ee6 --- /dev/null +++ b/.changeset/27803-composed-datasource-connector.md @@ -0,0 +1,18 @@ +--- +'@objectstack/spec': patch +--- + +fix(spec)!: `composeStacks` refuses a datasource whose `external.connector` names no connector instance the composed inputs declare (#27803) + +Clause-②: yes (narrowing) +Metadata-protocol: none + + + +**BREAKING** accept-set narrowing in `composeStacks`, and so in every CLI command that loads a project whose config composes two or more packages (`os build`, `os validate`, `os lint`, `os dev`, `os serve`). + +- **Before.** `defineStack` judged a datasource's `external.connector` against its own stack's `connectors` only, and an input built with `strict: false` skipped that judgement. Composing such an input carried a dangling name, a catalog descriptor (no `provider`) or an instance with no `auth` into the artifact with no verdict. The datasource then had no credential to borrow, found only when it first connected. +- **Now.** The composition's artifact pass judges every input's datasources against the connectors of every input, with the same three findings as the per-stack check, under the `STACK_CROSS_REFERENCE_INVALID` envelope (422). A datasource in one input that borrows an instance another input declares composes. An input that is itself a multi-package artifact is read through its package bodies, where its connectors and datasources live. A connector name declared twice in the flattened artifact is judged against the later entry, which is the one the registry keeps; when the earlier declaration could have lent, the finding names both and asks for the duplicate to be renamed or removed. A connector name that two packages of the artifact declare is kept twice, and the first one registered lends, so when one of the two can lend and the other cannot, the borrow is refused as ambiguous in either order. +- **Unchanged.** A strict `defineStack` refuses a dangling name in its own stack as before, so an artifact built from strict inputs that each resolve their own references is refused only when another input declares the borrowed connector name with an entry that cannot lend. + +**The remedy.** Declare the connector instance the datasource borrows (a `connectors` entry with a `provider` and an `auth` credential) in one of the composed packages, or correct the name. If the upstream needs no credential, drop `external.connector` from the datasource. diff --git a/packages/spec/src/contracts/data-engine.ts b/packages/spec/src/contracts/data-engine.ts index 5798a9c75e5..cca21669b86 100644 --- a/packages/spec/src/contracts/data-engine.ts +++ b/packages/spec/src/contracts/data-engine.ts @@ -496,7 +496,9 @@ export interface IDataEngine { * `schemaMode`, with or without a reference — `credentialsRef` is valid on * a managed datasource too, #8153) and answer FRESH copies: a reader must * not be able to reach through this accessor and mutate the write gate's - * own input. + * own input. Each entry carries the definition's stored `driver` id, and + * omits the key for a definition that never named one (see + * {@link EngineDatasourceDef.driver}). */ listDatasourceDefs?(): EngineDatasourceDef[]; diff --git a/packages/spec/src/data/injected-system-column-provenance.ts b/packages/spec/src/data/injected-system-column-provenance.ts index db49a0f8a02..d2c1b0d1071 100644 --- a/packages/spec/src/data/injected-system-column-provenance.ts +++ b/packages/spec/src/data/injected-system-column-provenance.ts @@ -230,8 +230,9 @@ export function injectedSystemColumnDefs(def: unknown): Record { + const pluginManifest = (id: string) => ({ + id, + namespace: id.split('.').pop(), + version: '1.0.0', + type: 'plugin', + name: id.split('.').pop(), + }); + const owner = (connector: Record = instance()) => + defineStack({ manifest: pluginManifest('com.acme.sap'), connectors: [connector] } as never); + const looseBorrower = (connector = 'sap_s4') => + defineStack({ manifest: pluginManifest('com.acme.orders'), datasources: [sapOrders(connector)] } as never, { + strict: false, + }); + const composeRefusal = (stacks: unknown[], options?: Record): Envelope | undefined => { + try { + composeStacks(stacks as never, options as never); + return undefined; + } catch (err) { + return err as Envelope; + } + }; + const expectComposedFinding = (refused: Envelope | undefined, fragment: string): void => { + expectOneFinding(refused, fragment); + expect(refused?.issues).toHaveLength(1); + expect(refused?.message).toContain('composeStacks artifact cross-reference validation failed (1 issue):'); + }; + + it.each([ + ['owner first, `last`', false, 'last'], + ['borrower first, `last`', true, 'last'], + ['owner first, `preserve`', false, 'preserve'], + ['borrower first, `preserve`', true, 'preserve'], + ])('CONTROL: a datasource borrowing an instance another input declares composes — %s', (_order, reversed, manifestOpt) => { + const stacks = reversed ? [looseBorrower(), owner()] : [owner(), looseBorrower()]; + expect(composeRefusal(stacks, { manifest: manifestOpt })).toBeUndefined(); + }); + + it.each(['last', 'preserve'])('refuses a dangling name a `strict: false` input carries (`%s`)', (manifestOpt) => { + const refused = composeRefusal([owner(instance({ name: 'sap_ariba' })), looseBorrower()], { manifest: manifestOpt }); + expectComposedFinding(refused, 'which is not defined in connectors'); + }); + + it('refuses a descriptor, and an instance with nothing to lend, that another input declares', () => { + const descriptor = { name: 'sap_s4', label: 'SAP S/4HANA', type: 'api' }; + expectComposedFinding(composeRefusal([owner(descriptor), looseBorrower()]), 'which declares no provider'); + expectComposedFinding( + composeRefusal([owner(instance({ auth: { type: 'none' } })), looseBorrower()]), + 'which declares no auth credential', + ); + }); + + it('judges a name two inputs declare in the flattened artifact against the later entry, the one the registry keeps', () => { + const strictPair = defineStack({ + manifest: pluginManifest('com.acme.orders'), + datasources: [sapOrders()], + connectors: [instance()], + } as never); + const descriptor = { name: 'sap_s4', label: 'SAP S/4HANA', type: 'api' }; + const refused = composeRefusal([strictPair, owner(descriptor)]); + expectComposedFinding(refused, 'which declares no provider in the entry the composed artifact keeps'); + expect(String(refused?.issues?.[0])).toContain( + "Connector 'sap_s4' is declared more than once: 'com.acme.orders' (stack #0) declares an instance " + + "that can lend, and 'com.acme.sap' (stack #1) declares the later entry, which the runtime keeps. " + + 'Rename or remove the later entry', + ); + expect(String(refused?.issues?.[0])).not.toContain('Give it the provider'); + expect(composeRefusal([owner(descriptor), strictPair])).toBeUndefined(); + }); + + describe('a name two packages of a `preserve` artifact declare: the registry keeps both, the first registered lends', () => { + const strictPair = () => + defineStack({ + manifest: pluginManifest('com.acme.orders'), + datasources: [sapOrders()], + connectors: [instance()], + } as never); + const descriptor = { name: 'sap_s4', label: 'SAP S/4HANA', type: 'api' }; + + it.each([ + ['instance first', false], + ['descriptor first', true], + ])('refuses the borrow as ambiguous when one can lend and the other cannot — %s', (_order, reversed) => { + const stacks = reversed ? [owner(descriptor), strictPair()] : [strictPair(), owner(descriptor)]; + const refused = composeRefusal(stacks, { manifest: 'preserve' }); + expectComposedFinding(refused, 'which more than one package of the composed artifact declares, and they disagree'); + const finding = String(refused?.issues?.[0]); + expect(finding).toContain("'com.acme.orders' (stack #"); + expect(finding).toContain("declares an instance that can lend, and 'com.acme.sap' (stack #"); + expect(finding).toContain('lends from whichever registers first'); + expect(finding).not.toContain('which the runtime keeps'); + }); + + it('CONTROL: two packages that each declare an instance that can lend compose, in either order', () => { + const other = owner(instance({ auth: { type: 'bearer', credentialRef: 'sap_token_2' } })); + expect(composeRefusal([strictPair(), other], { manifest: 'preserve' })).toBeUndefined(); + expect(composeRefusal([other, strictPair()], { manifest: 'preserve' })).toBeUndefined(); + }); + }); + + it('keeps the per-stack prescription when no declaration of the name could lend', () => { + const descriptor = { name: 'sap_s4', label: 'SAP S/4HANA', type: 'api' }; + const refused = composeRefusal([owner(descriptor), owner(instance({ auth: { type: 'none' } })), looseBorrower()]); + expectComposedFinding(refused, 'which declares no auth credential — there is nothing to borrow. Declare the'); + }); + + describe('a nested multi-package input, whose collections live only in its package bodies', () => { + const other = () => defineStack({ manifest: pluginManifest('com.acme.other') } as never); + const nested = (...stacks: unknown[]) => composeStacks([...stacks, other()] as never, { manifest: 'preserve' }); + + it('CONTROL: a borrower resolves against an instance a package body of the nested input carries', () => { + const inner = nested(owner()); + expect((inner as Record).connectors).toBeUndefined(); + expect(composeRefusal([inner, looseBorrower()], { manifest: 'preserve' })).toBeUndefined(); + expect(composeRefusal([looseBorrower(), inner])).toBeUndefined(); + }); + + it("judges a datasource a nested input's package body carries", () => { + // Hand-assembled: a composition would refuse the dangling name itself. + const inner = defineStack( + { + manifest: pluginManifest('com.acme.bundle'), + packages: [{ manifest: { ...pluginManifest('com.acme.orders'), datasources: [sapOrders('sap_missing')] } }], + } as never, + { strict: false }, + ); + const refused = composeRefusal([owner(), inner], { manifest: 'preserve' }); + expect({ code: refused?.code, status: refused?.status }).toEqual({ code: 'STACK_CROSS_REFERENCE_INVALID', status: 422 }); + expect(refused?.issues).toEqual([ + expect.stringMatching( + /^Datasource 'sap_orders' borrows the credential of connector 'sap_missing' \(external\.connector\), which is not defined in connectors\./, + ), + ]); + }); + + it("treats a package body and another input's top level as two slots, and names the body", () => { + const strictPair = defineStack({ + manifest: pluginManifest('com.acme.orders'), + datasources: [sapOrders()], + connectors: [instance()], + } as never); + const refused = composeRefusal([nested(strictPair), owner({ name: 'sap_s4', label: 'SAP', type: 'api' })]); + expectComposedFinding(refused, 'which more than one package of the composed artifact declares, and they disagree'); + expect(String(refused?.issues?.[0])).toContain( + "package 'com.acme.orders' of 'com.acme.other' (stack #0) declares an instance that can lend", + ); + }); + + it('reads an entry the nested input carries in both halves once', () => { + // A nested artifact whose bodies did not reproduce its flattened half + // keeps both, holding the same entry objects in each. + const borrower = sapOrders('sap_missing'); + const both = defineStack( + { + manifest: pluginManifest('com.acme.bundle'), + datasources: [borrower], + packages: [{ manifest: { ...pluginManifest('com.acme.orders'), datasources: [borrower] } }], + } as never, + { strict: false }, + ); + const refused = composeRefusal([owner(), both]); + expect(refused?.issues).toHaveLength(1); + expect(refused?.message).toContain('composeStacks artifact cross-reference validation failed (1 issue):'); + }); + }); + + it('CONTROL: the per-stack strict refusal is unchanged — a strict input never carries a dangling name in', () => { + const refused = refusalOf({ manifest: pluginManifest('com.acme.orders'), datasources: [sapOrders()] }); + expectOneFinding(refused, 'which is not defined in connectors'); + expect(refused?.message).not.toContain('composeStacks'); + }); +}); diff --git a/packages/spec/src/stack.zod.ts b/packages/spec/src/stack.zod.ts index 7beb89b85e6..67d08be95ab 100644 --- a/packages/spec/src/stack.zod.ts +++ b/packages/spec/src/stack.zod.ts @@ -4397,9 +4397,17 @@ function collectExternalBindingDatasourceErrors(config: ObjectStackDefinition): * no connection record to share. A disabled instance (`enabled: false`) is * accepted: the switch turns the vendor off for its actions and its records * alike, which is the one-grant behaviour D7 asks for, not a dangling name. - * Runs before the object-count early return: it needs no object. - */ -function collectDatasourceConnectorErrors(config: ObjectStackDefinition): string[] { + * Runs before the object-count early return: it needs no object. A + * composition runs it once more over every input's datasources and connectors + * ({@link collectComposedDatasourceConnectorErrors}), passing `shadowedLender`: + * for a name declared more than once in one registration slot whose kept + * entry cannot lend, it returns the fix to prescribe instead (the duplicate), + * or `undefined` when no earlier declaration could have lent either. + */ +function collectDatasourceConnectorErrors( + config: ObjectStackDefinition, + shadowedLender?: (name: string) => string | undefined, +): string[] { const errors: string[] = []; const datasources = Array.isArray(config.datasources) ? config.datasources : []; const borrowing = datasources.filter( @@ -4413,7 +4421,7 @@ function collectDatasourceConnectorErrors(config: ObjectStackDefinition): string for (const ds of borrowing) { const name = String((ds.external as Record).connector); const entry = connectors.get(name); - const subject = `Datasource '${String(ds.name)}' borrows the credential of connector '${name}' (external.connector)`; + const subject = connectorBorrowSubject(ds.name, name); if (entry === undefined) { errors.push( `${subject}, which is not defined in connectors. Declare the connector instance it borrows ` + @@ -4422,27 +4430,57 @@ function collectDatasourceConnectorErrors(config: ObjectStackDefinition): string ); continue; } - if (typeof entry.provider !== 'string' || entry.provider.length === 0) { + // The one door that sees a name declared twice in one registration slot: + // the later entry is the one the runtime keeps, so an earlier instance + // that could lend is shadowed, and the fix is the duplicate, not the + // entry's provider or auth. + const shadow = connectorCanLend(entry) ? undefined : shadowedLender?.(name); + if (!connectorHasProvider(entry)) { errors.push( - `${subject}, which declares no provider — a connector entry with no provider is a catalog ` + - `descriptor, never materialized, and holds no credential to lend (ADR-0097). Give it the ` + - `provider that serves this vendor and an auth credential, or name a connector instance ` + - `that has one.`, + shadow !== undefined + ? `${subject}, which declares no provider in the entry the composed artifact keeps — a catalog ` + + `descriptor that holds no credential to lend (ADR-0097). ${shadow}` + : `${subject}, which declares no provider — a connector entry with no provider is a catalog ` + + `descriptor, never materialized, and holds no credential to lend (ADR-0097). Give it the ` + + `provider that serves this vendor and an auth credential, or name a connector instance ` + + `that has one.`, ); continue; } - const auth = entry.auth; - if (!isRecord(auth) || auth.type === 'none') { + if (!connectorHasCredential(entry)) { errors.push( - `${subject}, which declares no auth credential — there is nothing to borrow. Declare the ` + - `instance's auth (e.g. auth: { type: 'bearer', credentialRef }), or, if the upstream ` + - `needs no credential, remove external.connector from the datasource.`, + shadow !== undefined + ? `${subject}, which declares no auth credential in the entry the composed artifact keeps — ` + + `there is nothing to borrow. ${shadow}` + : `${subject}, which declares no auth credential — there is nothing to borrow. Declare the ` + + `instance's auth (e.g. auth: { type: 'bearer', credentialRef }), or, if the upstream ` + + `needs no credential, remove external.connector from the datasource.`, ); } } return errors; } +/** The subject every `external.connector` finding opens with. */ +function connectorBorrowSubject(datasource: unknown, connector: string): string { + return `Datasource '${String(datasource)}' borrows the credential of connector '${connector}' (external.connector)`; +} + +/** A connector entry with a `provider`: an instance, not a catalog descriptor (ADR-0097). */ +function connectorHasProvider(entry: Record): boolean { + return typeof entry.provider === 'string' && entry.provider.length > 0; +} + +/** A connector entry whose `auth` holds a credential to lend: present, and not `type: 'none'`. */ +function connectorHasCredential(entry: Record): boolean { + return isRecord(entry.auth) && entry.auth.type !== 'none'; +} + +/** A connector entry a datasource's `external.connector` can borrow from. */ +function connectorCanLend(entry: Record): boolean { + return connectorHasProvider(entry) && connectorHasCredential(entry); +} + /** * [ADR-0015 §4.2 · #25211] {@link collectExternalBindingDatasourceErrors} over * a COMPOSITION: an `external` object of one input bound to a datasource @@ -4470,6 +4508,165 @@ function collectComposedExternalBindingDatasourceErrors( return collectExternalBindingDatasourceErrors({ objects: composedObjects, datasources } as ObjectStackDefinition); } +/** One declaration a composition input provides, and the registration slot it boots into. */ +interface ProvidedCollectionEntry { + entry: unknown; + /** Where it was declared, for a finding: the input, or a package body of it. */ + declaredIn: string; + /** + * The registry slot the entry registers in: the registry keys a + * package-owned item by package id and name, so two entries under one name + * in one slot are one item (the later wins), and in two slots they are two. + */ + slot: string; +} + +/** + * Every entry of collection `key` that one composition input provides: the + * input's own top level, plus each `packages[]` body when the input is itself + * a multi-package artifact, whose collections live ONLY in those bodies + * (#14512) — the {@link providedFlowNames} reading, for the same reason: the + * top level alone would refuse an entry the input carries. + * + * Each entry object is listed once. A nested artifact whose bodies did not + * reproduce its flattened half keeps both halves, holding the same entry + * objects in each, so the bodies are read first and the top level adds only + * what no body carries. The slot follows how the artifact boots: a package + * body registers under its own id; a top-level entry registers under the + * input's own package when `preserve` folds each input into a package body, + * and under the composed artifact's one package otherwise. + */ +function providedCollectionEntries( + stack: ObjectStackDefinition, + index: number, + key: 'connectors' | 'datasources', + preserve: boolean, +): ProvidedCollectionEntry[] { + const entries: ProvidedCollectionEntry[] = []; + const seen = new Set(); + const input = stackLabel(stack, index); + const collect = (list: unknown, declaredIn: string, slot: string): void => { + if (!Array.isArray(list)) return; + for (const entry of list) { + if (seen.has(entry)) continue; + seen.add(entry); + entries.push({ entry, declaredIn, slot }); + } + }; + const source = stack as Record; + if (Array.isArray(source.packages)) { + for (const pkg of source.packages) { + if (!isRecord(pkg) || !isRecord(pkg.manifest)) continue; + const id = pkg.manifest.id ?? pkg.manifest.name; + if (typeof id === 'string') collect(pkg.manifest[key], `package '${id}' of ${input}`, `package:${id}`); + else collect(pkg.manifest[key], input, `input:${index}`); + } + } + const ownId = stack.manifest?.id ?? stack.manifest?.name; + collect(source[key], input, preserve && typeof ownId === 'string' ? `package:${ownId}` : 'artifact'); + return entries; +} + +/** + * [ADR-0015 amendment D7] {@link collectDatasourceConnectorErrors} over a + * COMPOSITION: a datasource's `external.connector` against the `connectors` of + * every input. + * + * `defineStack` judges the reference against its own stack only, and an input + * built with `strict: false` never reaches that pass, so a dangling name it + * carries reached the composed artifact with no verdict: the datasource has no + * credential to borrow, and the author learns of the typo when it first + * connects instead of when the artifact is built. Judged here over the + * `datasources` and the `connectors` of every input, top level and package + * bodies alike ({@link providedCollectionEntries}), by the same collector, so + * the three findings and their fixes read exactly as the per-stack ones do. + * + * What this can newly refuse: a dangling name, a descriptor or an instance + * with nothing to lend, carried by an input built with `strict: false`. A + * cross-input pair resolves (a datasource in one input borrowing an instance + * another declares, or one of a nested multi-package input's package bodies + * declares), because the composed artifact registers both. + * + * A connector name declared more than once is judged by how the artifact + * boots it. Within one registration slot the later declaration is the one the + * registry keeps ({@link SAME_STACK_ENTRY_IDENTITY}'s `connectors` row), so + * the kept entry is judged, and when it cannot lend but an earlier one in the + * slot could, the finding names both and prescribes the duplicate's fix. In + * DIFFERENT slots (two packages of the artifact) the registry keeps one entry + * per package, and the lender answers whichever registers first, so when the + * kept entries disagree on whether they can lend the borrow is refused as + * ambiguous in either declaration order: the build cannot know which one the + * datasource gets, and one name meaning one grant is what D7 asks for. When + * they agree, the verdict is the same whichever one lends. + */ +function collectComposedDatasourceConnectorErrors( + stacks: readonly ObjectStackDefinition[], + preserve: boolean, +): string[] { + const datasources: unknown[] = []; + const declarations = new Map>(); + for (const [i, stack] of stacks.entries()) { + for (const { entry } of providedCollectionEntries(stack, i, 'datasources', preserve)) datasources.push(entry); + for (const provided of providedCollectionEntries(stack, i, 'connectors', preserve)) { + const { entry, slot } = provided; + if (!isRecord(entry) || typeof entry.name !== 'string') continue; + const bySlot = declarations.get(entry.name) ?? new Map(); + bySlot.set(slot, [...(bySlot.get(slot) ?? []), provided]); + declarations.set(entry.name, bySlot); + } + } + + const lends = (provided: ProvidedCollectionEntry): boolean => connectorCanLend(provided.entry as Record); + const connectors: unknown[] = []; + const shadows = new Map(); + const ambiguities = new Map(); + for (const [name, bySlot] of declarations) { + const kept = [...bySlot.values()].map((inSlot) => inSlot[inSlot.length - 1]); + const lending = kept.filter(lends); + const notLending = kept.filter((provided) => !lends(provided)); + if (lending.length > 0 && notLending.length > 0) { + const sites = (list: ProvidedCollectionEntry[]): string => list.map((p) => p.declaredIn).join(', '); + ambiguities.set( + name, + `which more than one package of the composed artifact declares, and they disagree: ` + + `${sites(lending)} declares an instance that can lend, and ${sites(notLending)} declares an ` + + `entry that cannot. The runtime keeps one entry per package and lends from whichever ` + + `registers first, so the datasource could boot with no credential. Rename or remove the ` + + `entry that cannot lend, so that one connector name means one instance.`, + ); + continue; + } + const representative = kept[kept.length - 1]; + connectors.push(representative.entry); + if (lends(representative)) continue; + const inSlot = bySlot.get(representative.slot) ?? []; + const lender = inSlot.slice(0, -1).reverse().find(lends); + if (lender === undefined) continue; + shadows.set( + name, + `Connector '${name}' is declared more than once: ${lender.declaredIn} declares an instance that ` + + `can lend, and ${representative.declaredIn} declares the later entry, which the runtime keeps. ` + + `Rename or remove the later entry, or give one of the two its own name and point ` + + `external.connector at the instance.`, + ); + } + + const errors: string[] = []; + const judged: unknown[] = []; + for (const ds of datasources) { + const connector = isRecord(ds) && isRecord(ds.external) ? ds.external.connector : undefined; + const ambiguity = typeof connector === 'string' ? ambiguities.get(connector) : undefined; + if (ambiguity === undefined) judged.push(ds); + else errors.push(`${connectorBorrowSubject((ds as Record).name, connector as string)}, ${ambiguity}`); + } + errors.push( + ...collectDatasourceConnectorErrors({ datasources: judged, connectors } as ObjectStackDefinition, (name) => + shadows.get(name), + ), + ); + return errors; +} + /** * Perform strict cross-reference validation on a parsed stack definition. * Returns an array of error messages (empty if valid). @@ -7489,6 +7686,9 @@ function composeBuiltStacks( // [ADR-0015 §4.2 · #25211] …and an `external` object of one input bound // to a datasource another input declares managed: see // {@link collectComposedExternalBindingDatasourceErrors}. + // [ADR-0015 amendment D7] …and a datasource's `external.connector` + // against the connectors of every input: see + // {@link collectComposedDatasourceConnectorErrors}. // [#25234] …and one membership key declared on two objects of the // composed artifact: see {@link collectComposedMembershipKeyErrors}. // [#25514] …and every flow target an input resolved only through @@ -7501,6 +7701,7 @@ function composeBuiltStacks( ...collectComposedDefaultRecordPageErrors(stacks, opts.manifest === 'preserve'), ...collectComposedMembershipKeyErrors(stacks, objects as readonly unknown[] | undefined), ...collectComposedExternalBindingDatasourceErrors(stacks, objects), + ...collectComposedDatasourceConnectorErrors(stacks, opts.manifest === 'preserve'), ...collectArtifactFlowClaimErrors(stacks), ]; if (artifactCrossRefErrors.length > 0) {