From a57c3cfa7d4e753d3a9b238eaf767e2621a26677 Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Thu, 11 Jul 2024 14:58:36 -0600 Subject: [PATCH 1/7] fix: Disable pretty-print for JSON reponses when using the REST fallback --- gax/src/fallbackRest.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/gax/src/fallbackRest.ts b/gax/src/fallbackRest.ts index 587504a07..96a4df82e 100644 --- a/gax/src/fallbackRest.ts +++ b/gax/src/fallbackRest.ts @@ -61,6 +61,11 @@ export function encodeRequest( '$alt=json%3Benum-encoding=int'; } + // Disable pretty-print JSON responses + transcoded.queryString = + (transcoded.queryString ? `${transcoded.queryString}&` : '') + + '$prettyPrint=0'; + // Converts httpMethod to method that permitted in standard Fetch API spec // https://fetch.spec.whatwg.org/#methods const method = transcoded.httpMethod.toUpperCase() as FetchParametersMethod; From dc4b9e08d1b391805db09aa5742172036d23d2a3 Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Thu, 11 Jul 2024 16:15:22 -0600 Subject: [PATCH 2/7] Investigating CI issue with failing numeric enum tests. --- gax/src/fallbackRest.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/gax/src/fallbackRest.ts b/gax/src/fallbackRest.ts index 96a4df82e..1d4b74691 100644 --- a/gax/src/fallbackRest.ts +++ b/gax/src/fallbackRest.ts @@ -54,6 +54,11 @@ export function encodeRequest( ); } + // Disable pretty-print JSON responses + transcoded.queryString = + (transcoded.queryString ? `${transcoded.queryString}&` : '') + + '$prettyPrint=0'; + // If numeric enums feature is requested, add extra parameter to the query string if (numericEnums) { transcoded.queryString = @@ -61,11 +66,6 @@ export function encodeRequest( '$alt=json%3Benum-encoding=int'; } - // Disable pretty-print JSON responses - transcoded.queryString = - (transcoded.queryString ? `${transcoded.queryString}&` : '') + - '$prettyPrint=0'; - // Converts httpMethod to method that permitted in standard Fetch API spec // https://fetch.spec.whatwg.org/#methods const method = transcoded.httpMethod.toUpperCase() as FetchParametersMethod; From 1fe50ae996726e01b7f30a71d468e72ecbaa1bdb Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Thu, 11 Jul 2024 16:36:27 -0600 Subject: [PATCH 3/7] Fix failing CI tests. --- gax/src/fallbackRest.ts | 10 +++++----- gax/test/unit/regapic.ts | 24 ++++++++++++------------ 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/gax/src/fallbackRest.ts b/gax/src/fallbackRest.ts index 1d4b74691..96a4df82e 100644 --- a/gax/src/fallbackRest.ts +++ b/gax/src/fallbackRest.ts @@ -54,11 +54,6 @@ export function encodeRequest( ); } - // Disable pretty-print JSON responses - transcoded.queryString = - (transcoded.queryString ? `${transcoded.queryString}&` : '') + - '$prettyPrint=0'; - // If numeric enums feature is requested, add extra parameter to the query string if (numericEnums) { transcoded.queryString = @@ -66,6 +61,11 @@ export function encodeRequest( '$alt=json%3Benum-encoding=int'; } + // Disable pretty-print JSON responses + transcoded.queryString = + (transcoded.queryString ? `${transcoded.queryString}&` : '') + + '$prettyPrint=0'; + // Converts httpMethod to method that permitted in standard Fetch API spec // https://fetch.spec.whatwg.org/#methods const method = transcoded.httpMethod.toUpperCase() as FetchParametersMethod; diff --git a/gax/test/unit/regapic.ts b/gax/test/unit/regapic.ts index 9ebf08019..4c78aea51 100644 --- a/gax/test/unit/regapic.ts +++ b/gax/test/unit/regapic.ts @@ -296,9 +296,9 @@ describe('REGAPIC', () => { .createStub(libraryService, stubOptions) .then(libStub => { libStub.getShelf(requestObject, {}, {}, (err?: {}, result?: {}) => { - assert.strictEqual( - spy.getCall(0).returnValue?.queryString, - '$alt=json%3Benum-encoding=int' + assert.match( + spy.getCall(0).returnValue?.queryString ?? '', + /\$alt=json%3Benum-encoding=int(&.*)?$/ ); assert.strictEqual(err, null); assert.strictEqual( @@ -336,9 +336,9 @@ describe('REGAPIC', () => { .createStub(libraryService, stubOptions) .then(libStub => { libStub.createShelf(requestObject, {}, {}, (err?: {}) => { - assert.strictEqual( - spy.getCall(0).returnValue?.queryString, - '$alt=json%3Benum-encoding=int' + assert.match( + spy.getCall(0).returnValue?.queryString ?? '', + /\$alt=json%3Benum-encoding=int(&.*)?$/ ); assert.strictEqual(err, null); done(); @@ -371,9 +371,9 @@ describe('REGAPIC', () => { .createStub(libraryService, stubOptions) .then(libStub => { libStub.createShelf(requestObject, {}, {}, (err?: {}) => { - assert.strictEqual( - spy.getCall(0).returnValue?.queryString, - 'queryStringParameter=must-be-preserved&$alt=json%3Benum-encoding=int' + assert.match( + spy.getCall(0).returnValue?.queryString ?? '', + /^queryStringParameter=must-be-preserved(&.*)?&\$alt=json%3Benum-encoding=int(&.*)?$/ ); assert.strictEqual(err, null); done(); @@ -403,9 +403,9 @@ describe('REGAPIC', () => { .createStub(libraryService, stubOptions) .then(libStub => { libStub.createShelf(requestObject, {}, {}, (err?: {}) => { - assert.strictEqual( - spy.getCall(0).returnValue?.queryString, - '$alt=json%3Benum-encoding=int' + assert.match( + spy.getCall(0).returnValue?.queryString ?? '', + /\$alt=json%3Benum-encoding=int(&.*)?$/ ); assert.strictEqual(err, null); done(); From 96d2acdb4bceb6118e60d51c9959f0b6b0d135eb Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Thu, 11 Jul 2024 16:46:57 -0600 Subject: [PATCH 4/7] Additional test fixes. --- gax/test/unit/regapic.ts | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/gax/test/unit/regapic.ts b/gax/test/unit/regapic.ts index 4c78aea51..c637f3b43 100644 --- a/gax/test/unit/regapic.ts +++ b/gax/test/unit/regapic.ts @@ -202,7 +202,10 @@ describe('REGAPIC', () => { gaxGrpc.createStub(libraryService, stubOptions).then(libStub => { libStub.getShelf(requestObject, {}, {}, (err?: {}, result?: {}) => { - assert.strictEqual(spy.getCall(0).returnValue?.queryString, ''); + assert.doesNotMatch( + spy.getCall(0).returnValue?.queryString ?? '', + /\$alt/ + ); assert.strictEqual(err, null); assert.strictEqual( 'shelf-name', @@ -237,7 +240,10 @@ describe('REGAPIC', () => { ); gaxGrpc.createStub(libraryService, stubOptions).then(libStub => { libStub.createShelf(requestObject, {}, {}, (err?: {}) => { - assert.strictEqual(spy.getCall(0).returnValue?.queryString, ''); + assert.doesNotMatch( + spy.getCall(0).returnValue?.queryString ?? '', + /\$alt/ + ); assert.strictEqual(err, null); done(); }); @@ -264,7 +270,10 @@ describe('REGAPIC', () => { ); gaxGrpc.createStub(libraryService, stubOptions).then(libStub => { libStub.createShelf(requestObject, {}, {}, (err?: {}) => { - assert.strictEqual(spy.getCall(0).returnValue?.queryString, ''); + assert.doesNotMatch( + spy.getCall(0).returnValue?.queryString ?? '', + /\$alt/ + ); assert.strictEqual(err, null); done(); }); From 47417e4d97a43b4cbb2c4ac16e574201b65edc43 Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Fri, 12 Jul 2024 10:45:43 -0600 Subject: [PATCH 5/7] Add opt-in setting minifyJson and tests. --- gax/src/fallback.ts | 5 +- gax/src/fallbackRest.ts | 13 +++-- gax/src/fallbackServiceStub.ts | 9 ++-- gax/src/grpc.ts | 1 + gax/test/unit/regapic.ts | 87 ++++++++++++++++++++++++++++++++++ 5 files changed, 106 insertions(+), 9 deletions(-) diff --git a/gax/src/fallback.ts b/gax/src/fallback.ts index e493bbb47..9e779f027 100644 --- a/gax/src/fallback.ts +++ b/gax/src/fallback.ts @@ -100,6 +100,7 @@ export class GrpcClient { private static protoCache = new Map(); httpRules?: Array; numericEnums: boolean; + minifyJson: boolean; /** * In rare cases users might need to deallocate all memory consumed by loaded protos. @@ -144,6 +145,7 @@ export class GrpcClient { this.grpcVersion = require('../../package.json').version; this.httpRules = (options as GrpcClientOptions).httpRules; this.numericEnums = (options as GrpcClientOptions).numericEnums ?? false; + this.minifyJson = (options as GrpcClientOptions).minifyJson ?? false; } /** @@ -343,7 +345,8 @@ export class GrpcClient { this.authClient, encoder, decoder, - this.numericEnums + this.numericEnums, + this.minifyJson ); return serviceStub; diff --git a/gax/src/fallbackRest.ts b/gax/src/fallbackRest.ts index 96a4df82e..20c257742 100644 --- a/gax/src/fallbackRest.ts +++ b/gax/src/fallbackRest.ts @@ -28,7 +28,8 @@ export function encodeRequest( servicePath: string, servicePort: number, request: {}, - numericEnums: boolean + numericEnums: boolean, + minifyJson: boolean ): FetchParameters { const headers: {[key: string]: string} = { 'Content-Type': 'application/json', @@ -61,10 +62,12 @@ export function encodeRequest( '$alt=json%3Benum-encoding=int'; } - // Disable pretty-print JSON responses - transcoded.queryString = - (transcoded.queryString ? `${transcoded.queryString}&` : '') + - '$prettyPrint=0'; + // If minifyJson feature is requsted, disable pretty-print JSON responses + if (minifyJson) { + transcoded.queryString = + (transcoded.queryString ? `${transcoded.queryString}&` : '') + + '$prettyPrint=0'; + } // Converts httpMethod to method that permitted in standard Fetch API spec // https://fetch.spec.whatwg.org/#methods diff --git a/gax/src/fallbackServiceStub.ts b/gax/src/fallbackServiceStub.ts index bb7a1b661..21206d9e7 100644 --- a/gax/src/fallbackServiceStub.ts +++ b/gax/src/fallbackServiceStub.ts @@ -61,14 +61,16 @@ export function generateServiceStub( servicePath: string, servicePort: number, request: {}, - numericEnums: boolean + numericEnums: boolean, + minifyJson: boolean ) => FetchParameters, responseDecoder: ( rpc: protobuf.Method, ok: boolean, response: Buffer | ArrayBuffer ) => {}, - numericEnums: boolean + numericEnums: boolean, + minifyJson: boolean ) { const fetch = hasWindowFetch() ? window.fetch @@ -100,7 +102,8 @@ export function generateServiceStub( servicePath, servicePort, request, - numericEnums + numericEnums, + minifyJson ); } catch (err) { // we could not encode parameters; pass error to the callback diff --git a/gax/src/grpc.ts b/gax/src/grpc.ts index f3db7b735..ff8d06f67 100644 --- a/gax/src/grpc.ts +++ b/gax/src/grpc.ts @@ -47,6 +47,7 @@ const COMMON_PROTO_FILES: string[] = commonProtoFiles.map(file => export interface GrpcClientOptions extends GoogleAuthOptions { auth?: GoogleAuth; grpc?: GrpcModule; + minifyJson?: boolean; protoJson?: protobuf.Root; httpRules?: Array; numericEnums?: boolean; diff --git a/gax/test/unit/regapic.ts b/gax/test/unit/regapic.ts index c637f3b43..3194173e2 100644 --- a/gax/test/unit/regapic.ts +++ b/gax/test/unit/regapic.ts @@ -50,6 +50,7 @@ const opts = { describe('REGAPIC', () => { let gaxGrpc: GrpcClient, gaxGrpcNumericEnums: GrpcClient, + gaxGrpcMinifyJson: GrpcClient, protos: protobuf.NamespaceBase, libProtos: protobuf.NamespaceBase, echoService: protobuf.Service, @@ -67,6 +68,10 @@ describe('REGAPIC', () => { ...opts, numericEnums: true, }); + gaxGrpcMinifyJson = new GrpcClient({ + ...opts, + minifyJson: true, + }); protos = gaxGrpc.loadProto(echoProtoJson); echoService = protos.lookupService('Echo'); const TEST_JSON = path.resolve( @@ -578,4 +583,86 @@ describe('REGAPIC', () => { }, /* catch: */ done); }); }); + + describe('should support json minification', () => { + it('should send prettyPrint=0 when json minification is requested', done => { + const requestObject = {name: 'shelves/shelf-name'}; + const responseObject = { + name: 'shelf-name', + theme: 'shelf-theme', + type: 100, // unknown enum value + }; + const spy = sinon.spy(transcoding, 'transcode'); + // incomplete types for nodeFetch, so... + // eslint-disable-next-line @typescript-eslint/no-explicit-any + sinon.stub(nodeFetch, 'Promise' as any).returns( + Promise.resolve({ + ok: true, + arrayBuffer: () => { + return Promise.resolve(Buffer.from(JSON.stringify(responseObject))); + }, + }) + ); + + gaxGrpcMinifyJson + .createStub(libraryService, stubOptions) + .then(libStub => { + libStub.getShelf(requestObject, {}, {}, (err?: {}, result?: {}) => { + assert.match( + spy.getCall(0).returnValue?.queryString ?? '', + /\$prettyPrint=0(&.*)?$/ + ); + assert.strictEqual(err, null); + assert.strictEqual( + 'shelf-name', + (result as {name: {}; theme: {}; type: {}}).name + ); + assert.strictEqual( + 100, + (result as {name: {}; theme: {}; type: {}}).type + ); + done(); + }); + }, /* catch: */ done); + }); + + it('should not send prettyPrint setting when json minification is not requested', done => { + const requestObject = {name: 'shelves/shelf-name'}; + const responseObject = { + name: 'shelf-name', + theme: 'shelf-theme', + type: 100, // unknown enum value + }; + const spy = sinon.spy(transcoding, 'transcode'); + // incomplete types for nodeFetch, so... + // eslint-disable-next-line @typescript-eslint/no-explicit-any + sinon.stub(nodeFetch, 'Promise' as any).returns( + Promise.resolve({ + ok: true, + arrayBuffer: () => { + return Promise.resolve(Buffer.from(JSON.stringify(responseObject))); + }, + }) + ); + + gaxGrpc.createStub(libraryService, stubOptions).then(libStub => { + libStub.getShelf(requestObject, {}, {}, (err?: {}, result?: {}) => { + assert.doesNotMatch( + spy.getCall(0).returnValue?.queryString ?? '', + /prettyPrint/ + ); + assert.strictEqual(err, null); + assert.strictEqual( + 'shelf-name', + (result as {name: {}; theme: {}; type: {}}).name + ); + assert.strictEqual( + 100, + (result as {name: {}; theme: {}; type: {}}).type + ); + done(); + }); + }, /* catch: */ done); + }); + }); }); From ee55874f06f43b09ad53f955503e26df6dfc16df Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Thu, 18 Jul 2024 08:34:04 -0400 Subject: [PATCH 6/7] Tweaks for code review comments. --- gax/src/fallbackRest.ts | 2 +- gax/test/unit/regapic.ts | 10 ++-------- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/gax/src/fallbackRest.ts b/gax/src/fallbackRest.ts index 20c257742..cb68a26bd 100644 --- a/gax/src/fallbackRest.ts +++ b/gax/src/fallbackRest.ts @@ -62,7 +62,7 @@ export function encodeRequest( '$alt=json%3Benum-encoding=int'; } - // If minifyJson feature is requsted, disable pretty-print JSON responses + // If minifyJson feature is requested, disable pretty-print JSON responses if (minifyJson) { transcoded.queryString = (transcoded.queryString ? `${transcoded.queryString}&` : '') + diff --git a/gax/test/unit/regapic.ts b/gax/test/unit/regapic.ts index 3194173e2..4b02e89c2 100644 --- a/gax/test/unit/regapic.ts +++ b/gax/test/unit/regapic.ts @@ -207,10 +207,7 @@ describe('REGAPIC', () => { gaxGrpc.createStub(libraryService, stubOptions).then(libStub => { libStub.getShelf(requestObject, {}, {}, (err?: {}, result?: {}) => { - assert.doesNotMatch( - spy.getCall(0).returnValue?.queryString ?? '', - /\$alt/ - ); + assert.strictEqual(spy.getCall(0).returnValue?.queryString, ''); assert.strictEqual(err, null); assert.strictEqual( 'shelf-name', @@ -245,10 +242,7 @@ describe('REGAPIC', () => { ); gaxGrpc.createStub(libraryService, stubOptions).then(libStub => { libStub.createShelf(requestObject, {}, {}, (err?: {}) => { - assert.doesNotMatch( - spy.getCall(0).returnValue?.queryString ?? '', - /\$alt/ - ); + assert.strictEqual(spy.getCall(0).returnValue?.queryString, ''); assert.strictEqual(err, null); done(); }); From 9a2a1a0bf4efe4d3eb80765592ce775c0734b903 Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Thu, 18 Jul 2024 08:55:43 -0400 Subject: [PATCH 7/7] More cleanup. --- gax/test/unit/regapic.ts | 41 ++++++++++++++++++++++++---------------- 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/gax/test/unit/regapic.ts b/gax/test/unit/regapic.ts index 4b02e89c2..6c85e75dd 100644 --- a/gax/test/unit/regapic.ts +++ b/gax/test/unit/regapic.ts @@ -269,10 +269,7 @@ describe('REGAPIC', () => { ); gaxGrpc.createStub(libraryService, stubOptions).then(libStub => { libStub.createShelf(requestObject, {}, {}, (err?: {}) => { - assert.doesNotMatch( - spy.getCall(0).returnValue?.queryString ?? '', - /\$alt/ - ); + assert.strictEqual(spy.getCall(0).returnValue?.queryString, ''); assert.strictEqual(err, null); done(); }); @@ -304,9 +301,9 @@ describe('REGAPIC', () => { .createStub(libraryService, stubOptions) .then(libStub => { libStub.getShelf(requestObject, {}, {}, (err?: {}, result?: {}) => { - assert.match( - spy.getCall(0).returnValue?.queryString ?? '', - /\$alt=json%3Benum-encoding=int(&.*)?$/ + assert.strictEqual( + spy.getCall(0).returnValue?.queryString, + '$alt=json%3Benum-encoding=int' ); assert.strictEqual(err, null); assert.strictEqual( @@ -344,8 +341,12 @@ describe('REGAPIC', () => { .createStub(libraryService, stubOptions) .then(libStub => { libStub.createShelf(requestObject, {}, {}, (err?: {}) => { + assert.strictEqual( + 'string', + typeof spy.getCall(0).returnValue?.queryString + ); assert.match( - spy.getCall(0).returnValue?.queryString ?? '', + spy.getCall(0).returnValue?.queryString, /\$alt=json%3Benum-encoding=int(&.*)?$/ ); assert.strictEqual(err, null); @@ -379,9 +380,9 @@ describe('REGAPIC', () => { .createStub(libraryService, stubOptions) .then(libStub => { libStub.createShelf(requestObject, {}, {}, (err?: {}) => { - assert.match( - spy.getCall(0).returnValue?.queryString ?? '', - /^queryStringParameter=must-be-preserved(&.*)?&\$alt=json%3Benum-encoding=int(&.*)?$/ + assert.strictEqual( + spy.getCall(0).returnValue?.queryString, + 'queryStringParameter=must-be-preserved&$alt=json%3Benum-encoding=int' ); assert.strictEqual(err, null); done(); @@ -411,9 +412,9 @@ describe('REGAPIC', () => { .createStub(libraryService, stubOptions) .then(libStub => { libStub.createShelf(requestObject, {}, {}, (err?: {}) => { - assert.match( - spy.getCall(0).returnValue?.queryString ?? '', - /\$alt=json%3Benum-encoding=int(&.*)?$/ + assert.strictEqual( + spy.getCall(0).returnValue?.queryString, + '$alt=json%3Benum-encoding=int' ); assert.strictEqual(err, null); done(); @@ -602,8 +603,12 @@ describe('REGAPIC', () => { .createStub(libraryService, stubOptions) .then(libStub => { libStub.getShelf(requestObject, {}, {}, (err?: {}, result?: {}) => { + assert.strictEqual( + 'string', + typeof spy.getCall(0).returnValue?.queryString + ); assert.match( - spy.getCall(0).returnValue?.queryString ?? '', + spy.getCall(0).returnValue?.queryString, /\$prettyPrint=0(&.*)?$/ ); assert.strictEqual(err, null); @@ -641,8 +646,12 @@ describe('REGAPIC', () => { gaxGrpc.createStub(libraryService, stubOptions).then(libStub => { libStub.getShelf(requestObject, {}, {}, (err?: {}, result?: {}) => { + assert.strictEqual( + 'string', + typeof spy.getCall(0).returnValue?.queryString + ); assert.doesNotMatch( - spy.getCall(0).returnValue?.queryString ?? '', + spy.getCall(0).returnValue?.queryString, /prettyPrint/ ); assert.strictEqual(err, null);