From f1ad565b321b0d45baed5c3304b4a9990c4b9c41 Mon Sep 17 00:00:00 2001 From: Kevin Kim Date: Mon, 16 Dec 2024 11:16:27 -0500 Subject: [PATCH 01/11] fix: Update owlbot.py to exculde sync repo (#1549) Part of fixing owl bot post processing to make it an optional check --- .github/sync-repo-settings.yaml | 1 - owlbot.py | 3 ++- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/sync-repo-settings.yaml b/.github/sync-repo-settings.yaml index b46e4c4d6..36ec74f96 100644 --- a/.github/sync-repo-settings.yaml +++ b/.github/sync-repo-settings.yaml @@ -13,7 +13,6 @@ branchProtectionRules: - test (18) - cla/google - windows - - OwlBot Post Processor permissionRules: - team: yoshi-admins permission: admin diff --git a/owlbot.py b/owlbot.py index 2513a4a3d..2b75bdf00 100644 --- a/owlbot.py +++ b/owlbot.py @@ -60,7 +60,8 @@ source_location='build/src' ) s.copy(templates,excludes=[ - '.github/auto-approve.yml' + '.github/auto-approve.yml', + '.github/sync-repo-settings.yaml' ]) node.postprocess_gapic_library_hermetic() From d009a8f9bb86c2efa192e98e565cd7b305700ff9 Mon Sep 17 00:00:00 2001 From: Kevin Kim Date: Mon, 16 Dec 2024 15:08:21 -0500 Subject: [PATCH 02/11] fix: Paused scan test is now fixed (#1539) Remove paused scan from failing test log. PR in test proxy now fixes this: https://togithub.com/googleapis/cloud-bigtable-clients-test/pull/211. Also adds some minor syntax updates to the test proxy server. --- testproxy/index.js | 3 +-- testproxy/known_failures.txt | 1 - 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/testproxy/index.js b/testproxy/index.js index 1caccc305..9d0eb17fb 100644 --- a/testproxy/index.js +++ b/testproxy/index.js @@ -49,7 +49,6 @@ function startServer(service) { grpc.ServerCredentials.createInsecure(), () => { console.log(`grpc server started on port: ${port}`); - server.start(); } ); } @@ -58,7 +57,7 @@ async function main() { const descriptor = await loadDescriptor(); const {service} = descriptor.google.bigtable.testproxy.CloudBigtableV2TestProxy; - await startServer(service); + startServer(service); } main(); diff --git a/testproxy/known_failures.txt b/testproxy/known_failures.txt index dc9d27d24..049920009 100644 --- a/testproxy/known_failures.txt +++ b/testproxy/known_failures.txt @@ -11,7 +11,6 @@ TestReadRow_Retry_WithRoutingCookie\| TestReadRow_Retry_WithRetryInfo\| TestReadRows_ReverseScans_FeatureFlag_Enabled\| TestReadRows_NoRetry_OutOfOrderError_Reverse\| -TestReadRows_Retry_PausedScan\| TestReadRows_Retry_LastScannedRow_Reverse\| TestReadRows_Retry_WithRoutingCookie\| TestReadRows_Retry_WithRoutingCookie_MultipleErrorResponses\| From f6176c110f832dbacc14ac60fa1870e69aa139ce Mon Sep 17 00:00:00 2001 From: Kevin Kim Date: Mon, 16 Dec 2024 15:14:22 -0500 Subject: [PATCH 03/11] Fix: TestMutateRow_Generic_Headers (#1540) Mutate rows is grabbing the `appProfileId` from the client. https://togithub.com/googleapis/nodejs-bigtable/blob/eb2cd9fb5134e454709b724d99b4da2db9ccadc0/src/tabular-api-surface.ts#L765 We should modify the test proxy test here to do the same. --- testproxy/known_failures.txt | 1 - testproxy/services/mutate-row.js | 5 +++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/testproxy/known_failures.txt b/testproxy/known_failures.txt index 049920009..50a2d43f7 100644 --- a/testproxy/known_failures.txt +++ b/testproxy/known_failures.txt @@ -1,4 +1,3 @@ -TestMutateRow_Generic_Headers\| TestMutateRow_Generic_DeadlineExceeded\| TestMutateRows_Generic_CloseClient\| TestMutateRows_Retry_WithRoutingCookie\| diff --git a/testproxy/services/mutate-row.js b/testproxy/services/mutate-row.js index 4148f753b..88c344e88 100644 --- a/testproxy/services/mutate-row.js +++ b/testproxy/services/mutate-row.js @@ -23,10 +23,11 @@ const mutateRow = ({clientMap}) => normalizeCallback(async rawRequest => { const {request} = rawRequest; const {request: mutateRequest} = request; - const {appProfileId, mutations, tableName, rowKey} = mutateRequest; - + const {mutations, tableName, rowKey} = mutateRequest; const {clientId} = request; + const appProfileId = clientMap.get(clientId).appProfileId; const client = clientMap.get(clientId)[v2]; + await client.mutateRow({ appProfileId, mutations, From 6ef76713dc8823cfb2131a60f3a09174d42b655c Mon Sep 17 00:00:00 2001 From: Kevin Kim Date: Tue, 17 Dec 2024 14:12:26 -0500 Subject: [PATCH 04/11] fix: Sample rowkey generic header conformance test (#1550) Grab app profiled id from client to fix conformance test --- testproxy/known_failures.txt | 1 - testproxy/services/sample-row-keys.js | 5 +++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/testproxy/known_failures.txt b/testproxy/known_failures.txt index 50a2d43f7..b83f23003 100644 --- a/testproxy/known_failures.txt +++ b/testproxy/known_failures.txt @@ -18,6 +18,5 @@ TestReadRows_Retry_WithRetryInfo_MultipleErrorResponse\| TestCheckAndMutateRow_NoRetry_TransientError\| TestCheckAndMutateRow_Generic_DeadlineExceeded\| TestCheckAndMutateRow_Generic_Headers\| -TestSampleRowKeys_Generic_Headers\| TestSampleRowKeys_Generic_DeadlineExceeded\| TestSampleRowKeys_Retry_WithRoutingCookie diff --git a/testproxy/services/sample-row-keys.js b/testproxy/services/sample-row-keys.js index 0f466ae02..f8a0b680e 100644 --- a/testproxy/services/sample-row-keys.js +++ b/testproxy/services/sample-row-keys.js @@ -23,9 +23,10 @@ const sampleRowKeys = ({clientMap}) => normalizeCallback(async rawRequest => { const {request} = rawRequest; const {request: sampleRowKeysRequest} = request; - const {appProfileId, tableName} = sampleRowKeysRequest; - + const {tableName} = sampleRowKeysRequest; const {clientId} = request; + + const appProfileId = clientMap.get(clientId).appProfileId; const client = clientMap.get(clientId)[v2]; const samples = await new Promise((res, rej) => { const response = []; From 7f1099afbd5af1639b843285a9ce6358f067e50e Mon Sep 17 00:00:00 2001 From: Kevin Kim Date: Tue, 17 Dec 2024 15:40:48 -0500 Subject: [PATCH 05/11] fix: Check and mutate generic header conformance test (#1551) Grab app profiled id from client --- testproxy/known_failures.txt | 1 - testproxy/services/check-and-mutate-row.js | 4 +++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/testproxy/known_failures.txt b/testproxy/known_failures.txt index b83f23003..95b03e50d 100644 --- a/testproxy/known_failures.txt +++ b/testproxy/known_failures.txt @@ -17,6 +17,5 @@ TestReadRows_Retry_WithRetryInfo\| TestReadRows_Retry_WithRetryInfo_MultipleErrorResponse\| TestCheckAndMutateRow_NoRetry_TransientError\| TestCheckAndMutateRow_Generic_DeadlineExceeded\| -TestCheckAndMutateRow_Generic_Headers\| TestSampleRowKeys_Generic_DeadlineExceeded\| TestSampleRowKeys_Retry_WithRoutingCookie diff --git a/testproxy/services/check-and-mutate-row.js b/testproxy/services/check-and-mutate-row.js index d846d34b4..0f9474249 100644 --- a/testproxy/services/check-and-mutate-row.js +++ b/testproxy/services/check-and-mutate-row.js @@ -23,11 +23,13 @@ const checkAndMutateRow = ({clientMap}) => normalizeCallback(async rawRequest => { const {request} = rawRequest; const {request: checkAndMutateRequest} = request; - const {appProfileId, falseMutations, rowKey, tableName, trueMutations} = + const {falseMutations, rowKey, tableName, trueMutations} = checkAndMutateRequest; const {clientId} = request; const client = clientMap.get(clientId)[v2]; + const appProfileId = clientMap.get(clientId).appProfileId; + const [result] = await client.checkAndMutateRow({ appProfileId, falseMutations, From 86f99f59ac530f2ead89ce817f5e20b47d28728d Mon Sep 17 00:00:00 2001 From: danieljbruce Date: Wed, 18 Dec 2024 17:07:39 -0500 Subject: [PATCH 06/11] chore(testproxy): Change the checkAndMutateRow test proxy service so that it uses the handwritten layer (#1541) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Build the request in a separate function * Apply the right mocks to the tests * Setup the frame for the new timeout test * Make sure to return the stream * Add assert checks - call count is 11 * Revert "Add assert checks - call count is 11" This reverts commit 24e3e334a8d884ffb6778dcc870d6e3e0a1d02cf. * Revert "Make sure to return the stream" This reverts commit 768242a3ee175d630ebf07f25350a2ae1db3f547. * Revert "Setup the frame for the new timeout test" This reverts commit f854546f9a814f7d988172afcd23aa363a6bf3a2. * Try to generate inverses * More code trying to invert the mutations * Delete files that are not needed anymore * Create a method for mutate.parse inverses * Create a method for createFlatMutationsListWithFn * Omit AppProfileId from call * Add a TODO for the other mutation types * Rename the file * Should invert mutations properly * Create some copy/pasted functions and a blank file * Remove rowKey from the inverse function * Remove unused row key * Rename mutations to entries * Create tests for the flatten mutations code * Add comment * Remove unused code * Set up the test proxy to use the handwritten layer * Fix the require errors * Remove a known failure that has been addressed * Add a test to ensure Gapic request is preserved * Remove some unused imports * Remove only everywhere * revert changes in the source file * Remove the mocks from the test file * This file does not need to live here anymore. * Add headers * Clarify the TODO * Simplify the variable name * Generate documentation for handwrittenLayerMutations * Address td-ignore statements * Use the callback provided to the Gapic layer * Remove onlys * Don’t modify mutation in the tests * Add a known failure * Add another known failure * Added more TestCheckAndMutateRow failures * Indent this code to indicate it is separate * Move inside the block * Refactor out the clientId string * Function renames * Prefer inline variable name * Better variable names * Eliminate use of any * Eliminate intermediate variable * Eliminate a line of code. Make variable inline. * Inline another variable * Shorten another test * Remove unused import * Shorten imports * Revert "Shorten imports" This reverts commit 4bb1bbe25e5ba4da23a57fde0cfc10370498a8f9. * Revert "Remove unused import" This reverts commit 554ba8d39845e4f97cdabe7c7811d712c91eeff5. * Revert "Revert "Remove unused import"" This reverts commit b14f66fabebde6937a419d5f6b18d4e72b8e6bb3. * Use service appProfileId by default * Eliminate large comment block * Address the TODOs - remove them * linter * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * Remove test * Add js suffix * Compile the test proxy folder * Add all testproxy typescript files to compiler --------- Co-authored-by: Kevin Kim Co-authored-by: Owl Bot --- test/mutation.ts | 4 +- test/test-proxy/checkAndMutateRowService.ts | 159 ++++++++++++++++++ test/test-proxy/mutationParseInverse.ts | 46 +++++ test/test-proxy/readModifyWriteRowService.ts | 24 ++- testproxy/known_failures.txt | 6 +- testproxy/services/check-and-mutate-row.js | 93 +++++++--- .../utils/request/createFlatMutationsList.ts | 73 ++++++++ .../services/utils/request/mutateInverse.ts | 97 +++++++++++ tsconfig.json | 4 +- 9 files changed, 479 insertions(+), 27 deletions(-) create mode 100644 test/test-proxy/checkAndMutateRowService.ts create mode 100644 test/test-proxy/mutationParseInverse.ts create mode 100644 testproxy/services/utils/request/createFlatMutationsList.ts create mode 100644 testproxy/services/utils/request/mutateInverse.ts diff --git a/test/mutation.ts b/test/mutation.ts index 7566a7496..a45c136b5 100644 --- a/test/mutation.ts +++ b/test/mutation.ts @@ -494,10 +494,10 @@ describe('Bigtable/Mutation', () => { data: [], }; const mutation = new Mutation(data); - Mutation.encodeSetCell = _data => { + sandbox.stub(Mutation, 'encodeSetCell').callsFake(_data => { assert.strictEqual(_data, data.data); return fakeEncoded; - }; + }); const mutationProto = mutation.toProto(); assert.strictEqual(mutationProto.mutations, fakeEncoded); assert.strictEqual(mutationProto.rowKey, data.key); diff --git a/test/test-proxy/checkAndMutateRowService.ts b/test/test-proxy/checkAndMutateRowService.ts new file mode 100644 index 000000000..9946c6612 --- /dev/null +++ b/test/test-proxy/checkAndMutateRowService.ts @@ -0,0 +1,159 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import * as assert from 'assert'; + +import {describe} from 'mocha'; +import {protos} from '../../src'; +import {BigtableClient} from '../../src/v2'; +import type {Callback, CallOptions, ServiceError} from 'google-gax'; +const checkAndMutateRowService = require('../../../testproxy/services/check-and-mutate-row.js'); +const createClient = require('../../../testproxy/services/create-client.js'); + +describe('TestProxy/CheckAndMutateRow', () => { + const testCases: protos.google.bigtable.v2.ICheckAndMutateRowRequest[] = [ + { + tableName: 'projects/projectId/instances/instance/tables/test-table', + appProfileId: 'test-app-profile', + rowKey: Buffer.from('test-row-key'), + predicateFilter: null, + trueMutations: [ + { + setCell: { + familyName: 'cf1', + timestampMicros: 1000007, + columnQualifier: Buffer.from('cq1'), + value: Buffer.from('value1'), + }, + }, + { + setCell: { + familyName: 'cf2', + timestampMicros: 1000007, + columnQualifier: Buffer.from('cq2'), + value: Buffer.from('value2'), + }, + }, + ], + falseMutations: [ + { + setCell: { + familyName: 'cf1', + timestampMicros: 1000007, + columnQualifier: Buffer.from('cq1'), + value: Buffer.from('value1'), + }, + }, + { + setCell: { + familyName: 'cf2', + timestampMicros: 1000007, + columnQualifier: Buffer.from('cq2'), + value: Buffer.from('value2'), + }, + }, + ], + }, + ]; + describe('Ensure the proper request is passed to the Gapic Layer', () => { + const clientId = 'TestCheckAndMutateRow_NoRetry_TransientError'; + testCases.forEach((checkAndMutateRowRequest, index) => { + it(`Run test ${index}`, done => { + (async () => { + const clientMap = new Map(); + const createClientFunction = createClient({clientMap}); + await new Promise((resolve, reject) => { + createClientFunction( + { + request: { + clientId, + dataTarget: 'localhost:1234', + projectId: 'projectId', + instanceId: 'instance', + appProfileId: '', + }, + }, + (error: ServiceError, response: {}) => { + if (error) { + reject(error); + } + resolve(response); + } + ); + }); + { + // Mock out the Gapic layer so we can see requests coming into it + const bigtable = clientMap.get(clientId); + const bigtableClient = new BigtableClient( + bigtable.options.BigtableClient + ); + bigtable.api['BigtableClient'] = bigtableClient; + bigtableClient.checkAndMutateRow = ( + request?: protos.google.bigtable.v2.ICheckAndMutateRowRequest, + optionsOrCallback?: + | CallOptions + | Callback< + protos.google.bigtable.v2.ICheckAndMutateRowResponse, + | protos.google.bigtable.v2.ICheckAndMutateRowRequest + | null + | undefined, + {} | null | undefined + >, + callback?: Callback< + protos.google.bigtable.v2.ICheckAndMutateRowResponse, + | protos.google.bigtable.v2.ICheckAndMutateRowRequest + | null + | undefined, + {} | null | undefined + > + ) => { + try { + // If the Gapic request is correct then the test passes. + assert.deepStrictEqual(request, checkAndMutateRowRequest); + } catch (e) { + // If the Gapic request is incorrect then the test fails with an error. + done(e); + } + if (callback) { + callback(null, {}); + } + return new Promise(resolve => { + const response: protos.google.bigtable.v2.ICheckAndMutateRowResponse = + {}; + resolve([response, {}, undefined]); + }); + }; + } + await new Promise((resolve, reject) => { + checkAndMutateRowService({clientMap})( + { + request: { + clientId, + request: checkAndMutateRowRequest, + }, + }, + (error: ServiceError, response: {}) => { + if (error) { + reject(error); + } + resolve(response); + } + ); + }); + done(); + })(); + }); + }); + }); +}); diff --git a/test/test-proxy/mutationParseInverse.ts b/test/test-proxy/mutationParseInverse.ts new file mode 100644 index 000000000..fd9dcbfa6 --- /dev/null +++ b/test/test-proxy/mutationParseInverse.ts @@ -0,0 +1,46 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import * as assert from 'assert'; +import {Mutation} from '../../src/mutation'; +import {mutationParseInverse} from '../../testproxy/services/utils/request/mutateInverse'; + +describe('Check mutation parse and mutationParseInverse are inverses', () => { + it('should invert mutations properly', () => { + const gapicLayerRequest = { + mutations: [ + { + setCell: { + familyName: 'cf1', + timestampMicros: 1000007, + columnQualifier: Buffer.from('cq1'), + value: Buffer.from('value1'), + }, + }, + { + setCell: { + familyName: 'cf2', + timestampMicros: 1000007, + columnQualifier: Buffer.from('cq2'), + value: Buffer.from('value2'), + }, + }, + ], + }; + assert.deepStrictEqual( + Mutation.parse(mutationParseInverse(gapicLayerRequest)), + gapicLayerRequest + ); + }); +}); diff --git a/test/test-proxy/readModifyWriteRowService.ts b/test/test-proxy/readModifyWriteRowService.ts index 474d25eab..de1abfda9 100644 --- a/test/test-proxy/readModifyWriteRowService.ts +++ b/test/test-proxy/readModifyWriteRowService.ts @@ -17,6 +17,7 @@ import * as assert from 'assert'; import {describe} from 'mocha'; import {protos} from '../../src'; import {BigtableClient} from '../../src/v2'; +import type {Callback, CallOptions} from 'google-gax'; const readModifyWriteRowService = require('../../../testproxy/services/read-modify-write-row.js'); const createClient = require('../../../testproxy/services/create-client.js'); @@ -74,16 +75,34 @@ describe('TestProxy/ReadModifyWriteRow', () => { ); bigtable.api['BigtableClient'] = bigtableClient; bigtableClient.readModifyWriteRow = ( - request?: protos.google.bigtable.v2.IReadModifyWriteRowRequest + request?: protos.google.bigtable.v2.IReadModifyWriteRowRequest, + optionsOrCallback?: + | CallOptions + | Callback< + protos.google.bigtable.v2.IReadModifyWriteRowResponse, + | protos.google.bigtable.v2.IReadModifyWriteRowRequest + | null + | undefined, + {} | null | undefined + >, + callback?: Callback< + protos.google.bigtable.v2.IReadModifyWriteRowResponse, + | protos.google.bigtable.v2.IReadModifyWriteRowRequest + | null + | undefined, + {} | null | undefined + > ) => { try { // If the Gapic request is correct then the test passes. assert.deepStrictEqual(request, readModifyWriteRowRequest); - done(); } catch (e) { // If the Gapic request is incorrect then the test fails with an error. done(e); } + if (callback) { + callback(null, {}); + } return new Promise(resolve => { const response: protos.google.bigtable.v2.IReadModifyWriteRowResponse = {}; @@ -109,6 +128,7 @@ describe('TestProxy/ReadModifyWriteRow', () => { } ); }); + done(); })(); }); }); diff --git a/testproxy/known_failures.txt b/testproxy/known_failures.txt index 95b03e50d..a370863d2 100644 --- a/testproxy/known_failures.txt +++ b/testproxy/known_failures.txt @@ -15,7 +15,11 @@ TestReadRows_Retry_WithRoutingCookie\| TestReadRows_Retry_WithRoutingCookie_MultipleErrorResponses\| TestReadRows_Retry_WithRetryInfo\| TestReadRows_Retry_WithRetryInfo_MultipleErrorResponse\| -TestCheckAndMutateRow_NoRetry_TransientError\| TestCheckAndMutateRow_Generic_DeadlineExceeded\| +TestCheckAndMutateRow_Generic_Headers\| +TestCheckAndMutateRow_NoRetry_TrueMutations\| +TestCheckAndMutateRow_NoRetry_FalseMutations\| +TestCheckAndMutateRow_Generic_CloseClient\| +TestCheckAndMutateRow_Generic_MultiStreams\| TestSampleRowKeys_Generic_DeadlineExceeded\| TestSampleRowKeys_Retry_WithRoutingCookie diff --git a/testproxy/services/check-and-mutate-row.js b/testproxy/services/check-and-mutate-row.js index 0f9474249..efbcf6d4f 100644 --- a/testproxy/services/check-and-mutate-row.js +++ b/testproxy/services/check-and-mutate-row.js @@ -16,32 +16,83 @@ const grpc = require('@grpc/grpc-js'); const normalizeCallback = require('./utils/normalize-callback.js'); +const getTableInfo = require('./utils/get-table-info'); +const { + createFlatMutationsListWithFnInverse, +} = require('../../build/testproxy/services/utils/request/createFlatMutationsList.js'); +const { + mutationParseInverse, +} = require('../../build/testproxy/services/utils/request/mutateInverse.js'); -const v2 = Symbol.for('v2'); +/** + * Transforms mutations from the gRPC layer format to the handwritten layer format. + * This function takes an array of gRPC layer mutations (google.bigtable.v2.IMutation[]) and converts + * them back to the format used by the handwritten layer. It essentially reverses the transformation + * performed by Mutation.parse. It's used internally by the test proxy for checkAndMutateRow. + * + * @param {google.bigtable.v2.IMutation[]} gapicLayerMutations An array of mutations in the gRPC layer format. + * @returns {FilterConfigOption[]} An array of mutations in the handwritten layer format. + */ +function handwrittenLayerMutations(gapicLayerMutations) { + return createFlatMutationsListWithFnInverse( + [ + { + mutations: gapicLayerMutations, + }, + ], + mutationParseInverse, + 1 + ); +} + +/** + * Converts a byte array (or string) back to a string. This is the inverse of + * Mutation.convertToBytes. + * + * @param {Bytes} bytes The byte array or string to convert. + * @returns {string} The converted string. + */ +function convertFromBytes(bytes) { + if (bytes instanceof Buffer) { + return bytes.toString(); + } else if (typeof bytes === 'string') { + return bytes; + } else { + throw new Error('Invalid input type. Must be Buffer or string.'); + } +} const checkAndMutateRow = ({clientMap}) => normalizeCallback(async rawRequest => { const {request} = rawRequest; - const {request: checkAndMutateRequest} = request; - const {falseMutations, rowKey, tableName, trueMutations} = - checkAndMutateRequest; - - const {clientId} = request; - const client = clientMap.get(clientId)[v2]; - const appProfileId = clientMap.get(clientId).appProfileId; - - const [result] = await client.checkAndMutateRow({ - appProfileId, - falseMutations, - rowKey, - tableName, - trueMutations, - }); - - return { - status: {code: grpc.status.OK, details: []}, - result, - }; + const {clientId, request: checkAndMutateRowRequest} = request; + const {appProfileId, falseMutations, rowKey, tableName, trueMutations} = + checkAndMutateRowRequest; + const onMatch = handwrittenLayerMutations(trueMutations); + const onNoMatch = handwrittenLayerMutations(falseMutations); + const id = convertFromBytes(rowKey); + const bigtable = clientMap.get(clientId); + bigtable.appProfileId = + appProfileId === '' ? clientMap.get(clientId).appProfileId : appProfileId; + const table = getTableInfo(bigtable, tableName); + const row = table.row(id); + const filter = []; + const filterConfig = {onMatch, onNoMatch}; + try { + const result = await row.filter(filter, filterConfig); + return { + status: {code: grpc.status.OK, details: []}, + row: result, + }; + } catch (e) { + return { + status: { + code: e.code, + details: [], + message: e.message, + }, + }; + } }); module.exports = checkAndMutateRow; diff --git a/testproxy/services/utils/request/createFlatMutationsList.ts b/testproxy/services/utils/request/createFlatMutationsList.ts new file mode 100644 index 000000000..7a074f18d --- /dev/null +++ b/testproxy/services/utils/request/createFlatMutationsList.ts @@ -0,0 +1,73 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import {FilterConfigOption} from '../../../../src/row'; +import {Mutation} from '../../../../src/mutation'; +import arrify = require('arrify'); + +/** + * Creates a flattened list of mutations by applying a transformation function to each entry in an array of FilterConfigOptions. + * + * This function takes an array of `FilterConfigOption` objects and a transformation function `f`. It applies `f` to each + * `FilterConfigOption` (after casting it to a `Mutation` object) and extracts the `mutations` property from the result of `f`. + * The `mutations` property is expected to be an array of type `T`, which extends `google.bigtable.v2.IMutation`. + * Finally, it flattens the resulting array of `T[]` into a single, concatenated array of `T`. + * + * @template T The type of mutation objects within the `mutations` array returned by `f`. Must extend `google.bigtable.v2.IMutation`. + * @param {FilterConfigOption[]} entries An array of `FilterConfigOption` objects, each representing a set of mutations. + * @param {function(Mutation): {mutations: T[]}} f The transformation function to apply to each `FilterConfigOption`. + * This function takes a `Mutation` object as input and must return an object with a `mutations` property that is an array of `T` objects. + * @returns {T[]} A flattened array of mutations of type `T`, created by concatenating the `mutations` arrays returned by applying `f` to each entry. + */ +export function createFlatMutationsListWithFn( + entries: FilterConfigOption[], + f: (entry: Mutation) => {mutations: T[]} +) { + const e2 = arrify(entries).map(entry => f(entry as Mutation).mutations!); + return e2.reduce((a, b) => a.concat(b), []); +} + +/** + * Partially inverts createFlatMutationsListWithFn, reconstructing the original + * FilterConfigOption[] by assuming 'f' converts Mutations to their protobuf + * representation. Note: This does *not* invert the transformation performed by 'f' itself. + * + * @param {T[]} entries The flattened mutations list. + * @param {Function} fInverse The inverse of the function 'f' used in createFlatMutationsListWithFn. MUST BE PROVIDED and MUST correctly invert 'f'. + * @param {number} numEntries The original number of entries. This is REQUIRED as the flattening operation loses this information. + * @returns {FilterConfigOption[]} The reconstructed FilterConfigOption array. + */ +export function createFlatMutationsListWithFnInverse( + entries: T[], + fInverse: (entry: T) => Mutation, + numEntries: number +): FilterConfigOption[] { + const invertedEntries: FilterConfigOption[] = []; + const mutationsPerEntry = entries.length / numEntries; + + for (let i = 0; i < numEntries; i++) { + const start = i * mutationsPerEntry; + const end = start + mutationsPerEntry; + const entryMutations = entries.slice(start, end) as unknown as T[]; // Type cast to align with fInverse input + + const invertedEntry: FilterConfigOption = {}; + for (const mutation of entryMutations) { + Object.assign(invertedEntry, fInverse(mutation)); // Apply inverse function to each mutation + } + + invertedEntries.push(invertedEntry); + } + + return invertedEntries; +} diff --git a/testproxy/services/utils/request/mutateInverse.ts b/testproxy/services/utils/request/mutateInverse.ts new file mode 100644 index 000000000..899b8bf48 --- /dev/null +++ b/testproxy/services/utils/request/mutateInverse.ts @@ -0,0 +1,97 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import {Data, Mutation, Bytes} from '../../../../src/mutation'; +import * as protos from '../../../../protos/protos'; + +/** + * Inverts the Mutation.parse function. Reconstructs a Mutation object from its + * protobuf representation. + * + * @param {protos.google.bigtable.v2.IMutateRowRequest} req The protobuf representation of the mutation. + * @returns {Mutation} The reconstructed Mutation object. + */ +export function mutationParseInverse(req: { + mutations: protos.google.bigtable.v2.IMutation[]; +}): Mutation { + let method: string | undefined; + let data: Data; + + if (req.mutations && req.mutations.length > 0) { + if (req.mutations[0].setCell) { + method = Mutation.methods.INSERT; + const localData = {} as any; + + req.mutations.forEach(m => { + const cell = m.setCell; + if (cell) { + const family = cell.familyName!; + const qualifier = Mutation.convertFromBytes( + cell.columnQualifier! as Bytes + ); + + // Now TypeScript knows that 'data' is an object, and 'family' is a string key + if (!localData[family]) { + localData[family] = {}; + } + localData[family][qualifier as string] = { + value: Mutation.convertFromBytes(cell?.value as Bytes), + timestamp: cell?.timestampMicros, + }; + } + }); + data = localData; + } else if ( + req.mutations.some( + m => m.deleteFromColumn || m.deleteFromFamily || m.deleteFromRow + ) + ) { + method = Mutation.methods.DELETE; + const localData: Data[] = []; + + req.mutations.forEach(m => { + if (m.deleteFromColumn) { + const col = m.deleteFromColumn; + + const column = { + family: col?.familyName, + qualifier: Mutation.convertFromBytes(col?.columnQualifier as Bytes), + }; + + const qualifier = `${column.family}:${column.qualifier}`; + + if ( + col?.timeRange?.startTimestampMicros && + col?.timeRange.endTimestampMicros + ) { + const time = { + start: col?.timeRange?.startTimestampMicros, + end: col?.timeRange.endTimestampMicros, + }; + localData.push({column: qualifier, time}); + } else { + localData.push(qualifier); + } + } else if (m.deleteFromFamily) { + localData.push(m.deleteFromFamily?.familyName); + } else if (m.deleteFromRow) { + localData.push({}); // Represent deleteFromRow as an empty object + } + }); + data = localData; + } + } + + return {method: method!, data} as Mutation; // method cannot be undefined here, assert non-null +} diff --git a/tsconfig.json b/tsconfig.json index c78f1c884..15d0354e8 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -14,6 +14,8 @@ "src/**/*.ts", "test/*.ts", "test/**/*.ts", - "system-test/*.ts" + "system-test/*.ts", + "testproxy/*.ts", + "testproxy/**/*.ts" ] } From 1ae4365a96e3dbc521e2df9682540125ab039c11 Mon Sep 17 00:00:00 2001 From: danieljbruce Date: Thu, 19 Dec 2024 14:09:26 -0500 Subject: [PATCH 07/11] chore(testproxy): Address the TestMutateRows_Generic_DeadlineExceeded conformance test (#1554) * Changes to arrive at passing test * Add a comment to this unexpected change * Make fewer test proxy changes * Change back to service error to eliminate type chg * Only have partial failures errors on timeout * Check time only in one place * Address the test - remove from list * Correct error - applied to mutate rows and not rr * error for each pending index * Eliminate the timeout exceeded check * Append mutations to the original error * Pass all entries back to test runner * Eliminate the special case in the test proxy * Added unit test * Remove consol log --- src/tabular-api-surface.ts | 44 ++++++++++++++++++++--- test/table.ts | 50 ++++++++++++++++++++++++++ testproxy/known_failures.txt | 1 - testproxy/services/bulk-mutate-rows.js | 29 ++++++++------- 4 files changed, 106 insertions(+), 18 deletions(-) diff --git a/src/tabular-api-surface.ts b/src/tabular-api-surface.ts index c3e259904..809aec495 100644 --- a/src/tabular-api-surface.ts +++ b/src/tabular-api-surface.ts @@ -692,6 +692,19 @@ Please use the format 'prezzy' or '${instance.name}/tables/prezzy'.`); [] ); + /* + The following line of code sets the timeout if it was provided while + creating the client. This will be used to determine if the client should + retry on errors. Eventually, this will be handled downstream in google-gax. + */ + const timeout = + options?.gaxOptions?.timeout || + (this?.bigtable?.options?.BigtableClient?.clientConfig?.interfaces && + this?.bigtable?.options?.BigtableClient?.clientConfig?.interfaces[ + 'google.bigtable.v2.Bigtable' + ]?.methods['MutateRows']?.timeout_millis); + const callTimeMillis = new Date().getTime(); + let numRequestsMade = 0; const maxRetries = is.number(this.maxRetries) ? this.maxRetries! : 3; @@ -703,7 +716,14 @@ Please use the format 'prezzy' or '${instance.name}/tables/prezzy'.`); ); const mutationErrorsByEntryIndex = new Map(); - const isRetryable = (err: ServiceError | null) => { + const isRetryable = ( + err: ServiceError | null, + timeoutExceeded: boolean + ) => { + if (timeoutExceeded) { + // If the timeout has been exceeded then do not retry. + return false; + } // Don't retry if there are no more entries or retry attempts if (pendingEntryIndices.size === 0 || numRequestsMade >= maxRetries + 1) { return false; @@ -721,7 +741,10 @@ Please use the format 'prezzy' or '${instance.name}/tables/prezzy'.`); return; } - if (isRetryable(err)) { + const timeoutExceeded = !!( + timeout && timeout < new Date().getTime() - callTimeMillis + ); + if (isRetryable(err, timeoutExceeded)) { const backOffSettings = options.gaxOptions?.retry?.backoffSettings || DEFAULT_BACKOFF_SETTINGS; @@ -736,12 +759,25 @@ Please use the format 'prezzy' or '${instance.name}/tables/prezzy'.`); err = null; } + const mutationErrors = Array.from(mutationErrorsByEntryIndex.values()); if (mutationErrorsByEntryIndex.size !== 0) { - const mutationErrors = Array.from(mutationErrorsByEntryIndex.values()); callback(new PartialFailureError(mutationErrors, err)); return; } - + if (err) { + /* If there's an RPC level failure and the mutation entries don't have + a status code, the RPC level failure error code will be used as the + entry failure code. + */ + (err as ServiceError & {errors?: ServiceError[]}).errors = + mutationErrors.concat( + [...pendingEntryIndices] + .filter(index => !mutationErrorsByEntryIndex.has(index)) + .map(() => err) + ); + callback(err); + return; + } callback(err); }; diff --git a/test/table.ts b/test/table.ts index 5d1a8d015..ed3f92879 100644 --- a/test/table.ts +++ b/test/table.ts @@ -2752,6 +2752,56 @@ describe('Bigtable/Table', () => { }; }); + it('should send back errors for each pending entry', done => { + const mutateEntries = [ + { + key: 'a', + method: 'insert', + data: {}, + }, + { + key: 'b', + method: 'insert', + data: {}, + }, + ]; + const unretriableError = new Error('not retryable') as ServiceError; + unretriableError.code = 3; // INVALID_ARGUMENT + emitters = [ + ((stream: Writable) => { + stream.emit('data', { + entries: [ + { + index: 0, + status: { + details: [], + code: 0, + message: 'Received data', + }, + }, + ], + }); + stream.emit('error', unretriableError); + }) as {} as EventEmitter, + ]; + table.maxRetries = 3; + table.mutate( + mutateEntries, + (err: ServiceError & {errors?: ServiceError[]}) => { + try { + assert.strictEqual(err.code, 3); + assert.strictEqual(err.message, 'not retryable'); + assert(err.errors); + assert.strictEqual(err.errors.length, 1); + assert.strictEqual(err.errors[0].code, 3); + assert.strictEqual(err.errors[0].message, 'not retryable'); + done(); + } catch (e) { + done(e); + } + } + ); + }); it('should not retry unretriable errors', done => { const unretriableError = new Error('not retryable') as ServiceError; unretriableError.code = 3; // INVALID_ARGUMENT diff --git a/testproxy/known_failures.txt b/testproxy/known_failures.txt index a370863d2..1ab91160e 100644 --- a/testproxy/known_failures.txt +++ b/testproxy/known_failures.txt @@ -1,7 +1,6 @@ TestMutateRow_Generic_DeadlineExceeded\| TestMutateRows_Generic_CloseClient\| TestMutateRows_Retry_WithRoutingCookie\| -TestMutateRows_Generic_DeadlineExceeded\| TestReadModifyWriteRow_NoRetry_MultiValues\| TestReadModifyWriteRow_Generic_CloseClient\| TestReadModifyWriteRow_Generic_MultiStreams\| diff --git a/testproxy/services/bulk-mutate-rows.js b/testproxy/services/bulk-mutate-rows.js index 5247afa15..56e760f03 100644 --- a/testproxy/services/bulk-mutate-rows.js +++ b/testproxy/services/bulk-mutate-rows.js @@ -37,20 +37,23 @@ const bulkMutateRows = ({clientMap}) => entries: [], }; } catch (error) { - if (error.name === 'PartialFailureError') { - return { - status: error, - entries: Array.from(error.errors.entries()).map(([index, entry]) => ({ + const entries = error.errors + ? Array.from(error.errors.entries()).map(([index, entry]) => ({ index: index + 1, - status: entry, - })), - }; - } else { - return { - status: error, - entries: [], - }; - } + status: { + code: entry.code, + message: entry.message, + }, + })) + : []; + return { + status: { + code: error.code, + details: [], + message: error.message, + }, + entries, + }; } }); From bcef5bc259690c04c6625eda45d6cb76fbe5f2ae Mon Sep 17 00:00:00 2001 From: danieljbruce Date: Thu, 19 Dec 2024 15:10:26 -0500 Subject: [PATCH 08/11] chore(testproxy): Remove TestCheckAndMutateRow_Generic_Headers from known failures (#1557) TestCheckAndMutateRow_Generic_Headers has actually already been addressed. --- testproxy/known_failures.txt | 1 - 1 file changed, 1 deletion(-) diff --git a/testproxy/known_failures.txt b/testproxy/known_failures.txt index 1ab91160e..a7f1c72ae 100644 --- a/testproxy/known_failures.txt +++ b/testproxy/known_failures.txt @@ -15,7 +15,6 @@ TestReadRows_Retry_WithRoutingCookie_MultipleErrorResponses\| TestReadRows_Retry_WithRetryInfo\| TestReadRows_Retry_WithRetryInfo_MultipleErrorResponse\| TestCheckAndMutateRow_Generic_DeadlineExceeded\| -TestCheckAndMutateRow_Generic_Headers\| TestCheckAndMutateRow_NoRetry_TrueMutations\| TestCheckAndMutateRow_NoRetry_FalseMutations\| TestCheckAndMutateRow_Generic_CloseClient\| From 49c64c58cba9e48538965f9a88889a3bd562a71c Mon Sep 17 00:00:00 2001 From: danieljbruce Date: Fri, 20 Dec 2024 10:04:31 -0500 Subject: [PATCH 09/11] chore(testproxy): Address the TestReadModifyWriteRow_Generic_CloseClient conformance test (#1555) **Summary** The `TestReadModifyWriteRow_Generic_CloseClient` conformance test is failing because results are not being passed along by the test proxy and the closed client error doesn't have an error code. These two problems are addressed in this PR so that the test passes. Changes are discussed inline. --- testproxy/known_failures.txt | 5 +++-- testproxy/services/close-client.js | 3 --- testproxy/services/read-modify-write-row.js | 4 ++-- 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/testproxy/known_failures.txt b/testproxy/known_failures.txt index a7f1c72ae..425ea9cd0 100644 --- a/testproxy/known_failures.txt +++ b/testproxy/known_failures.txt @@ -1,8 +1,8 @@ TestMutateRow_Generic_DeadlineExceeded\| +TestMutateRow_Generic_CloseClient\| TestMutateRows_Generic_CloseClient\| TestMutateRows_Retry_WithRoutingCookie\| TestReadModifyWriteRow_NoRetry_MultiValues\| -TestReadModifyWriteRow_Generic_CloseClient\| TestReadModifyWriteRow_Generic_MultiStreams\| TestReadRow_Generic_DeadlineExceeded\| TestReadRow_Retry_WithRoutingCookie\| @@ -20,4 +20,5 @@ TestCheckAndMutateRow_NoRetry_FalseMutations\| TestCheckAndMutateRow_Generic_CloseClient\| TestCheckAndMutateRow_Generic_MultiStreams\| TestSampleRowKeys_Generic_DeadlineExceeded\| -TestSampleRowKeys_Retry_WithRoutingCookie +TestSampleRowKeys_Retry_WithRoutingCookie\| +TestSampleRowKeys_Generic_CloseClient diff --git a/testproxy/services/close-client.js b/testproxy/services/close-client.js index 54eebf52a..2db286c3f 100644 --- a/testproxy/services/close-client.js +++ b/testproxy/services/close-client.js @@ -15,8 +15,6 @@ const normalizeCallback = require('./utils/normalize-callback.js'); -const v2 = Symbol.for('v2'); - const closeClient = ({clientMap}) => normalizeCallback(async rawRequest => { const request = rawRequest.request; @@ -24,7 +22,6 @@ const closeClient = ({clientMap}) => const bigtable = clientMap.get(clientId); if (bigtable) { - await bigtable[v2].close(); await bigtable.close(); return {}; } diff --git a/testproxy/services/read-modify-write-row.js b/testproxy/services/read-modify-write-row.js index 0bb5e0956..5117a4fcd 100644 --- a/testproxy/services/read-modify-write-row.js +++ b/testproxy/services/read-modify-write-row.js @@ -34,7 +34,7 @@ const readModifyWriteRow = ({clientMap}) => const table = getTableInfo(bigtable, tableName); const row = table.row(handWrittenRequest.id); try { - const result = await row.createRules(handWrittenRequest.rules); + const [result] = await row.createRules(handWrittenRequest.rules); return { status: {code: grpc.status.OK, details: []}, row: result.row, @@ -42,7 +42,7 @@ const readModifyWriteRow = ({clientMap}) => } catch (e) { return { status: { - code: e.code, + code: e.code ? e.code : grpc.status.UNKNOWN, details: [], message: e.message, }, From 82ab10581822ef659438d3b8741bbce87769b73d Mon Sep 17 00:00:00 2001 From: danieljbruce Date: Fri, 20 Dec 2024 11:23:51 -0500 Subject: [PATCH 10/11] chore(testproxy): Address the CheckAndMutateRow_Generic_CloseClient conformance test (#1556) * Attach code to closed client * Test proxy must send back the result * Address the known failure * Add rmrw code back in * eliminate unnecessary changes * Pass the code along in the test proxy, not client --- testproxy/known_failures.txt | 1 - testproxy/services/check-and-mutate-row.js | 6 +++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/testproxy/known_failures.txt b/testproxy/known_failures.txt index 425ea9cd0..229bbe011 100644 --- a/testproxy/known_failures.txt +++ b/testproxy/known_failures.txt @@ -17,7 +17,6 @@ TestReadRows_Retry_WithRetryInfo_MultipleErrorResponse\| TestCheckAndMutateRow_Generic_DeadlineExceeded\| TestCheckAndMutateRow_NoRetry_TrueMutations\| TestCheckAndMutateRow_NoRetry_FalseMutations\| -TestCheckAndMutateRow_Generic_CloseClient\| TestCheckAndMutateRow_Generic_MultiStreams\| TestSampleRowKeys_Generic_DeadlineExceeded\| TestSampleRowKeys_Retry_WithRoutingCookie\| diff --git a/testproxy/services/check-and-mutate-row.js b/testproxy/services/check-and-mutate-row.js index efbcf6d4f..3a7a38419 100644 --- a/testproxy/services/check-and-mutate-row.js +++ b/testproxy/services/check-and-mutate-row.js @@ -79,15 +79,15 @@ const checkAndMutateRow = ({clientMap}) => const filter = []; const filterConfig = {onMatch, onNoMatch}; try { - const result = await row.filter(filter, filterConfig); + const [, result] = await row.filter(filter, filterConfig); return { status: {code: grpc.status.OK, details: []}, - row: result, + result, }; } catch (e) { return { status: { - code: e.code, + code: e.code ? e.code : grpc.status.UNKNOWN, details: [], message: e.message, }, From 8f9c35509f73a8b0d0ca8d24589d4a591d078e25 Mon Sep 17 00:00:00 2001 From: danieljbruce Date: Fri, 20 Dec 2024 12:53:59 -0500 Subject: [PATCH 11/11] chore(testproxy): Remove multiple known failures that have already been addressed (#1560) * Reduce list * Remove another test that has been addressed * We got another passing test for free --- testproxy/known_failures.txt | 4 ---- 1 file changed, 4 deletions(-) diff --git a/testproxy/known_failures.txt b/testproxy/known_failures.txt index 229bbe011..71d655333 100644 --- a/testproxy/known_failures.txt +++ b/testproxy/known_failures.txt @@ -2,8 +2,6 @@ TestMutateRow_Generic_DeadlineExceeded\| TestMutateRow_Generic_CloseClient\| TestMutateRows_Generic_CloseClient\| TestMutateRows_Retry_WithRoutingCookie\| -TestReadModifyWriteRow_NoRetry_MultiValues\| -TestReadModifyWriteRow_Generic_MultiStreams\| TestReadRow_Generic_DeadlineExceeded\| TestReadRow_Retry_WithRoutingCookie\| TestReadRow_Retry_WithRetryInfo\| @@ -14,10 +12,8 @@ TestReadRows_Retry_WithRoutingCookie\| TestReadRows_Retry_WithRoutingCookie_MultipleErrorResponses\| TestReadRows_Retry_WithRetryInfo\| TestReadRows_Retry_WithRetryInfo_MultipleErrorResponse\| -TestCheckAndMutateRow_Generic_DeadlineExceeded\| TestCheckAndMutateRow_NoRetry_TrueMutations\| TestCheckAndMutateRow_NoRetry_FalseMutations\| -TestCheckAndMutateRow_Generic_MultiStreams\| TestSampleRowKeys_Generic_DeadlineExceeded\| TestSampleRowKeys_Retry_WithRoutingCookie\| TestSampleRowKeys_Generic_CloseClient