diff --git a/toolbox/mdcode/src/libts/gcp/dataplex.ts b/toolbox/mdcode/src/libts/gcp/dataplex.ts index 15d411b2f..344799a36 100644 --- a/toolbox/mdcode/src/libts/gcp/dataplex.ts +++ b/toolbox/mdcode/src/libts/gcp/dataplex.ts @@ -165,6 +165,29 @@ 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 }; + } + // 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 typeRef of typeRefs) { + const { aspectType } = await _aspectTypeOfKey(typeRef, this.context); + types.add(_nameToTypeRef(aspectType)); + } + return { status: 200, result: [...types] }; + } + async modifyEntry(project: string, location: string, entry: Entry, updateMask?: string[], aspectKeys?: string[]): Promise> { @@ -445,12 +468,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); @@ -466,6 +484,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/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..69644c51f 100644 --- a/toolbox/mdcode/src/libts/sources/bq-dataset.ts +++ b/toolbox/mdcode/src/libts/sources/bq-dataset.ts @@ -14,6 +14,14 @@ export class BigQueryDatasetSource implements CatalogSource { readonly ingestedEntries = true; readonly layout = Layouts.STANDARD; readonly defaultAspects = [md.OVERVIEW_ASPECT]; + // 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', + ]; 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..f49ca5a8b 100644 --- a/toolbox/mdcode/src/libts/sync.ts +++ b/toolbox/mdcode/src/libts/sync.ts @@ -54,14 +54,20 @@ export class CatalogSync { entries.push(entry); } - const aspectTypes = [...this._snapshot.aspectTypes.keys()]; + // 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) { // 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); + 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. @@ -69,7 +75,24 @@ export class CatalogSync { continue; } + // 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, 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..ec21b5db1 100644 --- a/toolbox/mdcode/tests/libts/gcp/dataplex.test.ts +++ b/toolbox/mdcode/tests/libts/gcp/dataplex.test.ts @@ -246,6 +246,58 @@ 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'], + }); + }); + + 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 // unless deleteMissingAspects is set, so a caller that means to remove one has // to send it. diff --git a/toolbox/mdcode/tests/libts/mocks.ts b/toolbox/mdcode/tests/libts/mocks.ts index ad1ce0a30..856fdb48a 100644 --- a/toolbox/mdcode/tests/libts/mocks.ts +++ b/toolbox/mdcode/tests/libts/mocks.ts @@ -98,6 +98,17 @@ export class CatalogClientMock extends gcp.CatalogClient { 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 0be2dd2cc..913e076ae 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) { @@ -227,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_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."