From 1af8275f1fa197b6b6c07e6c64252cdb0c1961f0 Mon Sep 17 00:00:00 2001 From: Miguel Rodriguez Date: Fri, 23 Oct 2026 23:31:26 +0000 Subject: [PATCH 1/3] feat(pull): warn about the aspects pull leaves out Pull fetched each entry with only the aspect types catalog.yaml lists and its entry type requires, so it could not tell an author what it left out. A catalog.yaml written before init listed any aspects pulls a knowledge base document with an empty body and says nothing. Pull now fetches each entry with every aspect (the FULL view), keeps the aspects of the types it knows, and warns once for each aspect type it left out, naming the number of entries and how to pull it. What it stores is unchanged. Each scope names the aspects the service writes beyond the required ones (CatalogSource.systemAspects), which pull leaves out without a warning. For a BigQuery dataset these are bigquery-policy and usage, the aspects BigQuery writes on a table besides the ones its type requires. lookupEntry takes 'FULL' to return every aspect. Bug: b/576454554 --- toolbox/mdcode/src/libts/gcp/dataplex.ts | 8 +++- toolbox/mdcode/src/libts/source.ts | 3 ++ .../mdcode/src/libts/sources/bq-dataset.ts | 5 ++ .../mdcode/src/libts/sources/entrygroup.ts | 1 + toolbox/mdcode/src/libts/sources/kb.ts | 1 + .../src/libts/sources/semantic-model.ts | 1 + toolbox/mdcode/src/libts/sync.ts | 31 ++++++++++-- .../mdcode/tests/libts/gcp/dataplex.test.ts | 18 +++++++ toolbox/mdcode/tests/libts/mocks.ts | 9 ++-- toolbox/mdcode/tests/libts/scenarios.ts | 13 ++++- .../mdcode/tests/scenarios/pull_basic.yaml | 4 +- toolbox/mdcode/tests/scenarios/pull_bq.yaml | 14 +++++- .../mdcode/tests/scenarios/pull_left_out.yaml | 48 +++++++++++++++++++ 13 files changed, 141 insertions(+), 15 deletions(-) create mode 100644 toolbox/mdcode/tests/scenarios/pull_left_out.yaml diff --git a/toolbox/mdcode/src/libts/gcp/dataplex.ts b/toolbox/mdcode/src/libts/gcp/dataplex.ts index 15d411b2f..ee43fb5f4 100644 --- a/toolbox/mdcode/src/libts/gcp/dataplex.ts +++ b/toolbox/mdcode/src/libts/gcp/dataplex.ts @@ -148,11 +148,15 @@ export class CatalogClient extends api.ApiClient { return res; } + // `aspects` names the aspect types to return, or is 'FULL' for every aspect + // on the entry. With neither, no aspect is returned. async lookupEntry(project: string, location: string, name: string, - aspects?: string[]): Promise> { + aspects?: string[] | 'FULL'): Promise> { const container = `${catalogContainer(project, location)}:lookupEntry`; const params: Record = { entry: name, view: 'BASIC' }; - if (aspects && aspects.length) { + if (aspects === 'FULL') { + params.view = 'FULL'; + } else if (aspects && aspects.length) { params.view = 'CUSTOM'; params.aspectTypes = aspects; } diff --git a/toolbox/mdcode/src/libts/source.ts b/toolbox/mdcode/src/libts/source.ts index 2a3ee4eea..fd1747332 100644 --- a/toolbox/mdcode/src/libts/source.ts +++ b/toolbox/mdcode/src/libts/source.ts @@ -30,6 +30,9 @@ export interface CatalogSource { // them along with those each entry's type requires. System-generated aspects // are left out, since push sends every local aspect back. readonly defaultAspects: readonly string[]; + // The aspect types the service writes on this scope's entries beyond those + // their entry types require. Pull leaves them out without a warning. + readonly systemAspects: readonly string[]; entries(ctx: gcp.ApiContext): AsyncGenerator; localName(entry: gcp.Entry): string; diff --git a/toolbox/mdcode/src/libts/sources/bq-dataset.ts b/toolbox/mdcode/src/libts/sources/bq-dataset.ts index f3ba836c8..1344ab9b5 100644 --- a/toolbox/mdcode/src/libts/sources/bq-dataset.ts +++ b/toolbox/mdcode/src/libts/sources/bq-dataset.ts @@ -14,6 +14,11 @@ export class BigQueryDatasetSource implements CatalogSource { readonly ingestedEntries = true; readonly layout = Layouts.STANDARD; readonly defaultAspects = [md.OVERVIEW_ASPECT]; + // BigQuery writes these on a table besides the ones its type requires. + readonly systemAspects = [ + 'dataplex-types.global.bigquery-policy', + 'dataplex-types.global.usage', + ]; private readonly _datasets: Map; diff --git a/toolbox/mdcode/src/libts/sources/entrygroup.ts b/toolbox/mdcode/src/libts/sources/entrygroup.ts index 066ae4d83..981a10f7f 100644 --- a/toolbox/mdcode/src/libts/sources/entrygroup.ts +++ b/toolbox/mdcode/src/libts/sources/entrygroup.ts @@ -14,6 +14,7 @@ export class EntryGroupSource implements CatalogSource { readonly ingestedEntries: boolean; readonly layout = Layouts.STANDARD; readonly defaultAspects = [md.OVERVIEW_ASPECT]; + readonly systemAspects = []; private readonly _name: string[]; private readonly _entryGroup: dataplex.EntryGroup; diff --git a/toolbox/mdcode/src/libts/sources/kb.ts b/toolbox/mdcode/src/libts/sources/kb.ts index 9f409cd25..d504c649d 100644 --- a/toolbox/mdcode/src/libts/sources/kb.ts +++ b/toolbox/mdcode/src/libts/sources/kb.ts @@ -14,6 +14,7 @@ export class KnowledgeBaseSource implements CatalogSource { readonly ingestedEntries = false; readonly layout = Layouts.DOCUMENTS; readonly defaultAspects = [md.OVERVIEW_ASPECT]; + readonly systemAspects = []; private readonly _name: string[]; private readonly _entryGroup: dataplex.EntryGroup; diff --git a/toolbox/mdcode/src/libts/sources/semantic-model.ts b/toolbox/mdcode/src/libts/sources/semantic-model.ts index 77dadf204..eb43c4bf0 100644 --- a/toolbox/mdcode/src/libts/sources/semantic-model.ts +++ b/toolbox/mdcode/src/libts/sources/semantic-model.ts @@ -20,6 +20,7 @@ export class SemanticModelSource implements CatalogSource { readonly layout = Layouts.SEMANTIC_MODEL; // A semantic model is pulled by its own path, not by snapshot aspects. readonly defaultAspects = []; + readonly systemAspects = []; readonly project: string; readonly location: string; diff --git a/toolbox/mdcode/src/libts/sync.ts b/toolbox/mdcode/src/libts/sync.ts index 7b7bbccc2..e7a184720 100644 --- a/toolbox/mdcode/src/libts/sync.ts +++ b/toolbox/mdcode/src/libts/sync.ts @@ -54,14 +54,19 @@ export class CatalogSync { entries.push(entry); } - const aspectTypes = [...this._snapshot.aspectTypes.keys()]; + // Each entry is fetched with every aspect, so pull can name the aspect + // types it leaves out: those catalog.yaml does not list and the entry's + // type does not require, apart from the scope's system aspects. + const aspectTypes = this._snapshot.aspectTypes; + const system = new Set(this._snapshot.manifest.source.systemAspects); + const leftOut = new Map(); for (const entry of entries) { // TODO: Handle local modification conflicts. // TODO: Handle config changes or service deletions that require removing local entries. const nameParts = entry.name.split('/'); const res = await this._catalog.lookupEntry(nameParts[1], nameParts[3], entry.name, - aspectTypes); + 'FULL'); // The server will respond with 403 permission denied for both resource not exist or // insufficient permission. We cannot tell if a resource not exist or user does not // have the access. Thus using 200 for an ensured result. @@ -69,7 +74,27 @@ export class CatalogSync { continue; } - await this._snapshot._storeEntry(res.result); + const kept: Record = {}; + const missing = new Set(); + for (const [key, aspect] of Object.entries(res.result.aspects ?? {})) { + // lookupEntry sets every returned aspect's type. + const type = aspect.aspectType!; + if (aspectTypes.has(type)) { + kept[key] = aspect; + } else { + missing.add(gcp._nameToTypeRef(type)); + } + } + for (const type of missing) { + if (!system.has(type)) leftOut.set(type, (leftOut.get(type) ?? 0) + 1); + } + await this._snapshot._storeEntry({ ...res.result, aspects: kept }); + } + for (const [type, count] of leftOut) { + console.warn( + `Warning: pull left out aspect type '${type}' on ${count} ` + + `${count === 1 ? 'entry' : 'entries'}; list it under ` + + `snapshot.aspects in catalog.yaml to pull it.`); } return { success: true }; } diff --git a/toolbox/mdcode/tests/libts/gcp/dataplex.test.ts b/toolbox/mdcode/tests/libts/gcp/dataplex.test.ts index bcd296186..772d54ea1 100644 --- a/toolbox/mdcode/tests/libts/gcp/dataplex.test.ts +++ b/toolbox/mdcode/tests/libts/gcp/dataplex.test.ts @@ -249,6 +249,24 @@ describe('_fixEntry removed aspects', () => { // Dataplex keeps an aspect that aspectKeys names and the body leaves out // unless deleteMissingAspects is set, so a caller that means to remove one has // to send it. +describe('CatalogClient.lookupEntry', () => { + test('picks the view from the aspects asked for', async () => { + const client = new CatalogClient(CTX); + const get = spyOn(client as any, '_get') + .mockImplementation(async () => ({status: 404})); + + await client.lookupEntry('proj', 'us', ENTRY); + await client.lookupEntry('proj', 'us', ENTRY, [GUIDELINES_TYPE]); + await client.lookupEntry('proj', 'us', ENTRY, 'FULL'); + + expect(get.mock.calls.map(c => c[1])).toEqual([ + {entry: ENTRY, view: 'BASIC'}, + {entry: ENTRY, view: 'CUSTOM', aspectTypes: [GUIDELINES_TYPE]}, + {entry: ENTRY, view: 'FULL'}, + ]); + }); +}); + describe('CatalogClient.updateEntry', () => { test('sends deleteMissingAspects only when asked to', async () => { const client = new CatalogClient(CTX); diff --git a/toolbox/mdcode/tests/libts/mocks.ts b/toolbox/mdcode/tests/libts/mocks.ts index ad1ce0a30..6a025e475 100644 --- a/toolbox/mdcode/tests/libts/mocks.ts +++ b/toolbox/mdcode/tests/libts/mocks.ts @@ -23,7 +23,7 @@ export class CatalogClientMock extends gcp.CatalogClient { public mockEntryTypes: Map = new Map(); public mockAspectTypes: Map = new Map(); // The aspect types the last lookupEntry call asked for. - public lookupAspectTypes?: string[]; + public lookupAspectTypes?: string[] | 'FULL'; constructor() { super(TEST_API_CONTEXT); @@ -86,15 +86,16 @@ export class CatalogClientMock extends gcp.CatalogClient { return { status: 404, message: 'Not found' }; } - async lookupEntry(project: string, location: string, name: string, aspects?: string[]): Promise> { + async lookupEntry(project: string, location: string, name: string, aspects?: string[] | 'FULL'): Promise> { this.lookupAspectTypes = aspects; const entry = this.mockEntries.find(e => e.name == name); if (!entry) { return { status: 404, message: 'Not found' }; } - // As Dataplex does: only the aspects of the requested types, none when no type is requested. + // As Dataplex does: every aspect for 'FULL', else only the aspects of the + // requested types, none when no type is requested. const kept = Object.entries(entry.aspects ?? {}) - .filter(([, a]) => aspects?.includes(a.aspectType ?? '')); + .filter(([, a]) => aspects === 'FULL' || aspects?.includes(a.aspectType ?? '')); return { status: 200, result: { ...entry, aspects: Object.fromEntries(kept) } }; } diff --git a/toolbox/mdcode/tests/libts/scenarios.ts b/toolbox/mdcode/tests/libts/scenarios.ts index 0be2dd2cc..b0d599c87 100644 --- a/toolbox/mdcode/tests/libts/scenarios.ts +++ b/toolbox/mdcode/tests/libts/scenarios.ts @@ -92,7 +92,10 @@ function runScenario(scenario: any) { const snapshot = await kcmac.CatalogSnapshot.fromPath('/', TEST_API_CONTEXT); const sync = new kcmac.CatalogSync(catalog, snapshot); - // Execute - Snapshot actions + // Execute - Snapshot actions, recording the warnings they print. + const warnings: string[] = []; + const warn = spyOn(console, 'warn').mockImplementation( + (...args: any[]) => { warnings.push(args.join(' ')); }); const actions = scenario.actions ?? []; for (const actionStep of actions) { const { action, ...params } = actionStep; @@ -119,6 +122,12 @@ function runScenario(scenario: any) { throw new Error(`Unknown action: ${action}`); } } + warn.mockRestore(); + + // Assert expectations - Warnings + if (scenario.assert?.warnings) { + expect(warnings).toEqual(scenario.assert.warnings); + } // Assert expectations - Filesystem if (scenario.assert?.fileSystem) { @@ -219,7 +228,7 @@ function main() { ); spyOn(gcp.CatalogClient.prototype, 'lookupEntry').mockImplementation( - async function(project: string, location: string, entry: string, aspects?: string[]) { + async function(project: string, location: string, entry: string, aspects?: string[] | 'FULL') { if (currentCatalogMock) { return await currentCatalogMock.lookupEntry(project, location, entry, aspects); } diff --git a/toolbox/mdcode/tests/scenarios/pull_basic.yaml b/toolbox/mdcode/tests/scenarios/pull_basic.yaml index 610b93e57..7cdd59ba6 100644 --- a/toolbox/mdcode/tests/scenarios/pull_basic.yaml +++ b/toolbox/mdcode/tests/scenarios/pull_basic.yaml @@ -49,6 +49,4 @@ assert: - contains: "note: on e3" catalog/g1_entry.yaml: null catalog: - lookupAspectTypes: - - projects/test/locations/global/aspectTypes/a1 - - test.global.a1 + lookupAspectTypes: FULL diff --git a/toolbox/mdcode/tests/scenarios/pull_bq.yaml b/toolbox/mdcode/tests/scenarios/pull_bq.yaml index 5b8830933..9f9110a89 100644 --- a/toolbox/mdcode/tests/scenarios/pull_bq.yaml +++ b/toolbox/mdcode/tests/scenarios/pull_bq.yaml @@ -1,5 +1,5 @@ name: pull_bq -description: BigQuery dataset +description: BigQuery dataset, whose system-written aspects are left out without a warning setup: bigQuery: datasets: @@ -23,6 +23,16 @@ setup: entryType: projects/dataplex-types/locations/global/entryTypes/bigquery-dataset - name: projects/p/locations/us/entryGroups/@bigquery/entries/bigquery.googleapis.com/projects/p/datasets/d/tables/t1 entryType: projects/dataplex-types/locations/global/entryTypes/bigquery-table + # Aspects BigQuery writes besides the required ones: left out, with + # no warning. + aspects: + dataplex-types.global.usage: + aspectType: projects/dataplex-types/locations/global/aspectTypes/usage + data: + queryCount: 7 + dataplex-types.global.bigquery-policy: + aspectType: projects/dataplex-types/locations/global/aspectTypes/bigquery-policy + data: {} fileSystem: catalog.yaml: | @@ -39,3 +49,5 @@ assert: catalog/p.d/t1.yaml: - contains: "name: p.d/t1" - contains: "type: dataplex-types.global.bigquery-table" + - notContains: "queryCount" + warnings: [] diff --git a/toolbox/mdcode/tests/scenarios/pull_left_out.yaml b/toolbox/mdcode/tests/scenarios/pull_left_out.yaml new file mode 100644 index 000000000..92814eeff --- /dev/null +++ b/toolbox/mdcode/tests/scenarios/pull_left_out.yaml @@ -0,0 +1,48 @@ +name: pull_left_out +description: An entry group pulled with a catalog.yaml that lists no aspects leaves out the aspects it does not list and warns once for each aspect type +setup: + catalog: + entryGroups: + - name: projects/test/locations/us/entryGroups/g1 + entryTypes: + - name: projects/test/locations/global/entryTypes/et1 + aspectTypes: + - name: projects/test/locations/global/aspectTypes/extra + entries: + - name: projects/test/locations/us/entryGroups/g1/entries/e1 + entryType: projects/test/locations/global/entryTypes/et1 + aspects: + test.global.extra: + aspectType: projects/test/locations/global/aspectTypes/extra + data: + note: on e1 + dataplex-types.global.overview: + aspectType: projects/dataplex-types/locations/global/aspectTypes/overview + data: + content: Text in Knowledge Catalog. + - name: projects/test/locations/us/entryGroups/g1/entries/e2 + entryType: projects/test/locations/global/entryTypes/et1 + aspects: + test.global.extra: + aspectType: projects/test/locations/global/aspectTypes/extra + data: + note: on e2 + + fileSystem: + catalog.yaml: | + scope: entryGroup.test.us.g1 + +actions: + - action: pull + +assert: + fileSystem: + catalog/e1.yaml: + - contains: "name: e1" + - notContains: "note: on e1" + - notContains: "Text in Knowledge Catalog." + catalog/e2.yaml: + - notContains: "note: on e2" + warnings: + - "Warning: pull left out aspect type 'test.global.extra' on 2 entries; list it under snapshot.aspects in catalog.yaml to pull it." + - "Warning: pull left out aspect type 'dataplex-types.global.overview' on 1 entry; list it under snapshot.aspects in catalog.yaml to pull it." From 454df410a48c42fe5e7419cd13e9eb65a1213878 Mon Sep 17 00:00:00 2001 From: Miguel Rodriguez Date: Mon, 26 Oct 2026 17:59:27 +0000 Subject: [PATCH 2/3] fix(pull): read the aspects to warn about without losing listed ones A FULL read gives data only to the aspects an entry's type requires, and _fixEntry drops the rest, so pulling through it stored no listed aspect beyond the required ones and never warned. Pull now reads each entry with the aspect types it knows, as before, and stores that result. A new CatalogClient.lookupEntryAspectTypes reads the FULL view's keys alone and names each aspect type on the entry once, with project numbers resolved. Pull warns about a type the first read did not return, it does not know and the scope does not list as a system aspect. lookupEntry is back to its earlier form. The BigQuery system aspects also name data-profile and data-quality-scorecard, which Dataplex data scans write. Bug: b/576454554 --- toolbox/mdcode/src/libts/gcp/dataplex.ts | 46 ++++++++++++++----- .../mdcode/src/libts/sources/bq-dataset.ts | 5 +- toolbox/mdcode/src/libts/sync.ts | 32 ++++++------- .../mdcode/tests/libts/gcp/dataplex.test.ts | 46 +++++++++++-------- toolbox/mdcode/tests/libts/mocks.ts | 20 ++++++-- toolbox/mdcode/tests/libts/scenarios.ts | 11 ++++- .../mdcode/tests/scenarios/pull_basic.yaml | 4 +- 7 files changed, 109 insertions(+), 55 deletions(-) diff --git a/toolbox/mdcode/src/libts/gcp/dataplex.ts b/toolbox/mdcode/src/libts/gcp/dataplex.ts index ee43fb5f4..45ac698ab 100644 --- a/toolbox/mdcode/src/libts/gcp/dataplex.ts +++ b/toolbox/mdcode/src/libts/gcp/dataplex.ts @@ -148,15 +148,11 @@ export class CatalogClient extends api.ApiClient { return res; } - // `aspects` names the aspect types to return, or is 'FULL' for every aspect - // on the entry. With neither, no aspect is returned. async lookupEntry(project: string, location: string, name: string, - aspects?: string[] | 'FULL'): Promise> { + aspects?: string[]): Promise> { const container = `${catalogContainer(project, location)}:lookupEntry`; const params: Record = { entry: name, view: 'BASIC' }; - if (aspects === 'FULL') { - params.view = 'FULL'; - } else if (aspects && aspects.length) { + if (aspects && aspects.length) { params.view = 'CUSTOM'; params.aspectTypes = aspects; } @@ -169,6 +165,25 @@ export class CatalogClient extends api.ApiClient { return res; } + // The type of every aspect on an entry, as project.location.type, each + // once. A FULL read lists every aspect but gives data only to those the + // entry's type requires, so this reads the keys alone, which lookupEntry + // would drop. + async lookupEntryAspectTypes(project: string, location: string, + name: string): Promise> { + const container = `${catalogContainer(project, location)}:lookupEntry`; + const res = await this._get(container, { entry: name, view: 'FULL' }); + if (res.status != 200 || !res.result) { + return { status: res.status, message: res.message }; + } + const types = new Set(); + for (const key of Object.keys(res.result.aspects ?? {})) { + const { aspectType } = await _aspectTypeOfKey(key, this.context); + types.add(_nameToTypeRef(aspectType)); + } + return { status: 200, result: [...types] }; + } + async modifyEntry(project: string, location: string, entry: Entry, updateMask?: string[], aspectKeys?: string[]): Promise> { @@ -449,12 +464,7 @@ export async function _fixEntry(entry: Entry, ctx: context.ApiContext, const fixedAspects: Record = {}; for (const [aspectKey, aspectValue] of Object.entries(entry.aspects)) { if (!aspectValue || !Object.keys(aspectValue).length) continue; - const atIdx = aspectKey.indexOf('@'); - const typeRef = atIdx === -1 ? aspectKey : aspectKey.slice(0, atIdx); - const keyPath = atIdx === -1 ? undefined : aspectKey.slice(atIdx + 1); - - const aspectType = - await crm.fixProject(_typeRefToName(typeRef, 'aspect'), ctx); + const { aspectType, keyPath } = await _aspectTypeOfKey(aspectKey, ctx); const path = aspectValue.path || keyPath; const baseKey = _nameToTypeRef(aspectType); @@ -470,6 +480,18 @@ export async function _fixEntry(entry: Entry, ctx: context.ApiContext, } } +// The aspect type an aspect key names, as a resource name with a project ID, +// and the key's `@` part, if any. +async function _aspectTypeOfKey(key: string, ctx: context.ApiContext): + Promise<{ aspectType: string; keyPath?: string }> { + const atIdx = key.indexOf('@'); + const typeRef = atIdx === -1 ? key : key.slice(0, atIdx); + return { + aspectType: await crm.fixProject(_typeRefToName(typeRef, 'aspect'), ctx), + keyPath: atIdx === -1 ? undefined : key.slice(atIdx + 1), + }; +} + // Constructs canonical names for catalog container resources, identified by project, location and // optionally, depending on use-case, the entry group. export function catalogContainer(project: string, location: string, entryGroup: string=''): string { diff --git a/toolbox/mdcode/src/libts/sources/bq-dataset.ts b/toolbox/mdcode/src/libts/sources/bq-dataset.ts index 1344ab9b5..69644c51f 100644 --- a/toolbox/mdcode/src/libts/sources/bq-dataset.ts +++ b/toolbox/mdcode/src/libts/sources/bq-dataset.ts @@ -14,9 +14,12 @@ export class BigQueryDatasetSource implements CatalogSource { readonly ingestedEntries = true; readonly layout = Layouts.STANDARD; readonly defaultAspects = [md.OVERVIEW_ASPECT]; - // BigQuery writes these on a table besides the ones its type requires. + // BigQuery and Dataplex data scans write these on a table besides the ones + // its type requires. readonly systemAspects = [ 'dataplex-types.global.bigquery-policy', + 'dataplex-types.global.data-profile', + 'dataplex-types.global.data-quality-scorecard', 'dataplex-types.global.usage', ]; diff --git a/toolbox/mdcode/src/libts/sync.ts b/toolbox/mdcode/src/libts/sync.ts index e7a184720..f49ca5a8b 100644 --- a/toolbox/mdcode/src/libts/sync.ts +++ b/toolbox/mdcode/src/libts/sync.ts @@ -54,10 +54,11 @@ export class CatalogSync { entries.push(entry); } - // Each entry is fetched with every aspect, so pull can name the aspect - // types it leaves out: those catalog.yaml does not list and the entry's - // type does not require, apart from the scope's system aspects. + // Pull fetches the aspect types it knows: those catalog.yaml lists and + // each entry's type requires. It warns about each other aspect type an + // entry carries, apart from the scope's system aspects. const aspectTypes = this._snapshot.aspectTypes; + const requested = [...aspectTypes.keys()]; const system = new Set(this._snapshot.manifest.source.systemAspects); const leftOut = new Map(); for (const entry of entries) { @@ -66,7 +67,7 @@ export class CatalogSync { const nameParts = entry.name.split('/'); const res = await this._catalog.lookupEntry(nameParts[1], nameParts[3], entry.name, - 'FULL'); + requested); // The server will respond with 403 permission denied for both resource not exist or // insufficient permission. We cannot tell if a resource not exist or user does not // have the access. Thus using 200 for an ensured result. @@ -74,21 +75,18 @@ export class CatalogSync { continue; } - const kept: Record = {}; - const missing = new Set(); - for (const [key, aspect] of Object.entries(res.result.aspects ?? {})) { - // lookupEntry sets every returned aspect's type. - const type = aspect.aspectType!; - if (aspectTypes.has(type)) { - kept[key] = aspect; - } else { - missing.add(gcp._nameToTypeRef(type)); + // lookupEntry sets every returned aspect's type. + const stored = new Set(Object.values(res.result.aspects ?? {}) + .map(a => gcp._nameToTypeRef(a.aspectType!))); + await this._snapshot._storeEntry(res.result); + + const onEntry = await this._catalog.lookupEntryAspectTypes( + nameParts[1], nameParts[3], entry.name); + for (const type of onEntry.result ?? []) { + if (!stored.has(type) && !aspectTypes.has(type) && !system.has(type)) { + leftOut.set(type, (leftOut.get(type) ?? 0) + 1); } } - for (const type of missing) { - if (!system.has(type)) leftOut.set(type, (leftOut.get(type) ?? 0) + 1); - } - await this._snapshot._storeEntry({ ...res.result, aspects: kept }); } for (const [type, count] of leftOut) { console.warn( diff --git a/toolbox/mdcode/tests/libts/gcp/dataplex.test.ts b/toolbox/mdcode/tests/libts/gcp/dataplex.test.ts index 772d54ea1..3dd6e412d 100644 --- a/toolbox/mdcode/tests/libts/gcp/dataplex.test.ts +++ b/toolbox/mdcode/tests/libts/gcp/dataplex.test.ts @@ -246,27 +246,37 @@ describe('_fixEntry removed aspects', () => { }); +describe('CatalogClient.lookupEntryAspectTypes', () => { + test('reads the FULL view and names each aspect type once, with a project ID', + async () => { + const client = new CatalogClient(CTX); + const get = spyOn(client as any, '_get').mockImplementation(async () => ({ + status: 200, + result: { + name: ENTRY, + aspects: { + // A FULL read gives no data to an aspect the type does not require. + 'dataplex-types.global.overview': {}, + '655216118709.global.guidelines@Schema.order_id': {}, + 'dataplex-types.global.guidelines@Schema.customer_id': {}, + }, + }, + })); + + const res = await client.lookupEntryAspectTypes('proj', 'us', ENTRY); + + expect(get.mock.calls[0].slice(0, 2)).toEqual( + ['projects/proj/locations/us:lookupEntry', {entry: ENTRY, view: 'FULL'}]); + expect(res).toEqual({ + status: 200, + result: ['dataplex-types.global.overview', 'dataplex-types.global.guidelines'], + }); + }); +}); + // Dataplex keeps an aspect that aspectKeys names and the body leaves out // unless deleteMissingAspects is set, so a caller that means to remove one has // to send it. -describe('CatalogClient.lookupEntry', () => { - test('picks the view from the aspects asked for', async () => { - const client = new CatalogClient(CTX); - const get = spyOn(client as any, '_get') - .mockImplementation(async () => ({status: 404})); - - await client.lookupEntry('proj', 'us', ENTRY); - await client.lookupEntry('proj', 'us', ENTRY, [GUIDELINES_TYPE]); - await client.lookupEntry('proj', 'us', ENTRY, 'FULL'); - - expect(get.mock.calls.map(c => c[1])).toEqual([ - {entry: ENTRY, view: 'BASIC'}, - {entry: ENTRY, view: 'CUSTOM', aspectTypes: [GUIDELINES_TYPE]}, - {entry: ENTRY, view: 'FULL'}, - ]); - }); -}); - describe('CatalogClient.updateEntry', () => { test('sends deleteMissingAspects only when asked to', async () => { const client = new CatalogClient(CTX); diff --git a/toolbox/mdcode/tests/libts/mocks.ts b/toolbox/mdcode/tests/libts/mocks.ts index 6a025e475..856fdb48a 100644 --- a/toolbox/mdcode/tests/libts/mocks.ts +++ b/toolbox/mdcode/tests/libts/mocks.ts @@ -23,7 +23,7 @@ export class CatalogClientMock extends gcp.CatalogClient { public mockEntryTypes: Map = new Map(); public mockAspectTypes: Map = new Map(); // The aspect types the last lookupEntry call asked for. - public lookupAspectTypes?: string[] | 'FULL'; + public lookupAspectTypes?: string[]; constructor() { super(TEST_API_CONTEXT); @@ -86,19 +86,29 @@ export class CatalogClientMock extends gcp.CatalogClient { return { status: 404, message: 'Not found' }; } - async lookupEntry(project: string, location: string, name: string, aspects?: string[] | 'FULL'): Promise> { + async lookupEntry(project: string, location: string, name: string, aspects?: string[]): Promise> { this.lookupAspectTypes = aspects; const entry = this.mockEntries.find(e => e.name == name); if (!entry) { return { status: 404, message: 'Not found' }; } - // As Dataplex does: every aspect for 'FULL', else only the aspects of the - // requested types, none when no type is requested. + // As Dataplex does: only the aspects of the requested types, none when no type is requested. const kept = Object.entries(entry.aspects ?? {}) - .filter(([, a]) => aspects === 'FULL' || aspects?.includes(a.aspectType ?? '')); + .filter(([, a]) => aspects?.includes(a.aspectType ?? '')); return { status: 200, result: { ...entry, aspects: Object.fromEntries(kept) } }; } + async lookupEntryAspectTypes(project: string, location: string, name: string): Promise> { + const entry = this.mockEntries.find(e => e.name == name); + if (!entry) { + return { status: 404, message: 'Not found' }; + } + // As Dataplex's FULL view does: it lists every aspect key on the entry, + // with data or not, so every aspect type appears, each once. + const types = Object.keys(entry.aspects ?? {}).map(k => k.split('@')[0]); + return { status: 200, result: [...new Set(types)] }; + } + async modifyEntry(project: string, location: string, entry: gcp.Entry, updateMask?: string[], aspectKeys?: string[]): Promise> { return this.applyUpdate(entry, updateMask, aspectKeys); } diff --git a/toolbox/mdcode/tests/libts/scenarios.ts b/toolbox/mdcode/tests/libts/scenarios.ts index b0d599c87..913e076ae 100644 --- a/toolbox/mdcode/tests/libts/scenarios.ts +++ b/toolbox/mdcode/tests/libts/scenarios.ts @@ -228,7 +228,7 @@ function main() { ); spyOn(gcp.CatalogClient.prototype, 'lookupEntry').mockImplementation( - async function(project: string, location: string, entry: string, aspects?: string[] | 'FULL') { + async function(project: string, location: string, entry: string, aspects?: string[]) { if (currentCatalogMock) { return await currentCatalogMock.lookupEntry(project, location, entry, aspects); } @@ -236,6 +236,15 @@ function main() { } ); + spyOn(gcp.CatalogClient.prototype, 'lookupEntryAspectTypes').mockImplementation( + async function(project: string, location: string, entry: string) { + if (currentCatalogMock) { + return await currentCatalogMock.lookupEntryAspectTypes(project, location, entry); + } + return { status: 404, message: 'Not found' }; + } + ); + spyOn(gcp.CatalogClient.prototype, 'modifyEntry').mockImplementation( async function(project: string, location: string, entry: gcp.Entry, updateMask?: string[], aspectKeys?: string[]) { if (currentCatalogMock) { diff --git a/toolbox/mdcode/tests/scenarios/pull_basic.yaml b/toolbox/mdcode/tests/scenarios/pull_basic.yaml index 7cdd59ba6..610b93e57 100644 --- a/toolbox/mdcode/tests/scenarios/pull_basic.yaml +++ b/toolbox/mdcode/tests/scenarios/pull_basic.yaml @@ -49,4 +49,6 @@ assert: - contains: "note: on e3" catalog/g1_entry.yaml: null catalog: - lookupAspectTypes: FULL + lookupAspectTypes: + - projects/test/locations/global/aspectTypes/a1 + - test.global.a1 From 55d6a0aa063470147fd484412a03919d58defd5f Mon Sep 17 00:00:00 2001 From: Miguel Rodriguez Date: Mon, 26 Oct 2026 20:15:31 +0000 Subject: [PATCH 3/3] perf(pull): resolve each aspect type once when listing an entry's aspects lookupEntryAspectTypes resolved the type of every aspect key on its own, and crm caches only the projects it can read. An entry with several field-path aspects of one type in an unreadable project made one failing lookup per key. It now resolves each distinct type once. Bug: b/576454554 --- toolbox/mdcode/src/libts/gcp/dataplex.ts | 8 +++++-- .../mdcode/tests/libts/gcp/dataplex.test.ts | 24 +++++++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/toolbox/mdcode/src/libts/gcp/dataplex.ts b/toolbox/mdcode/src/libts/gcp/dataplex.ts index 45ac698ab..344799a36 100644 --- a/toolbox/mdcode/src/libts/gcp/dataplex.ts +++ b/toolbox/mdcode/src/libts/gcp/dataplex.ts @@ -176,9 +176,13 @@ export class CatalogClient extends api.ApiClient { if (res.status != 200 || !res.result) { return { status: res.status, message: res.message }; } + // Each type is resolved once: crm caches only the projects it can read, + // so several field-path keys of one unreadable type would each look it up. + const typeRefs = new Set(Object.keys(res.result.aspects ?? {}) + .map(key => key.split('@')[0])); const types = new Set(); - for (const key of Object.keys(res.result.aspects ?? {})) { - const { aspectType } = await _aspectTypeOfKey(key, this.context); + for (const typeRef of typeRefs) { + const { aspectType } = await _aspectTypeOfKey(typeRef, this.context); types.add(_nameToTypeRef(aspectType)); } return { status: 200, result: [...types] }; diff --git a/toolbox/mdcode/tests/libts/gcp/dataplex.test.ts b/toolbox/mdcode/tests/libts/gcp/dataplex.test.ts index 3dd6e412d..ec21b5db1 100644 --- a/toolbox/mdcode/tests/libts/gcp/dataplex.test.ts +++ b/toolbox/mdcode/tests/libts/gcp/dataplex.test.ts @@ -272,6 +272,30 @@ describe('CatalogClient.lookupEntryAspectTypes', () => { result: ['dataplex-types.global.overview', 'dataplex-types.global.guidelines'], }); }); + + test('looks up an unreadable project once per aspect type', async () => { + const client = new CatalogClient(CTX); + spyOn(client as any, '_get').mockImplementation(async () => ({ + status: 200, + result: { + name: ENTRY, + aspects: { + '111.global.guidelines@Schema.order_id': {}, + '111.global.guidelines@Schema.customer_id': {}, + }, + }, + })); + // Resource Manager cannot read the project, and crm caches only the + // projects it can read. + const lookup = spyOn(ResourceManagerClient.prototype, 'getProject') + .mockResolvedValue({status: 403} as any); + + const res = await client.lookupEntryAspectTypes('proj', 'us', ENTRY); + + expect(res.result).toEqual(['111.global.guidelines']); + expect(lookup).toHaveBeenCalledTimes(1); + lookup.mockRestore(); + }); }); // Dataplex keeps an aspect that aspectKeys names and the body leaves out