feat: Add minifyJson param to request minified JSON responses when using the REST fallback #1632
Conversation
product-auto-label
bot
added
size: s
product-auto-label
bot
added
size: m
MarkDuckworth
changed the title
gax/src/fallbackRest.ts
Outdated
| @@ -61,6 +62,13 @@ export function encodeRequest( | |||
| '$alt=json%3Benum-encoding=int'; | |||
| } | |||
|
|
|||
| // If minifyJson feature is requsted, disable pretty-print JSON responses | |||
There was a problem hiding this comment.
| // If minifyJson feature is requsted, disable pretty-print JSON responses | |
| // If minifyJson feature is requested, disable pretty-print JSON responses |
gax/test/unit/regapic.ts
Outdated
| @@ -202,7 +207,10 @@ describe('REGAPIC', () => { | |||
|
|
|||
| gaxGrpc.createStub(libraryService, stubOptions).then(libStub => { | |||
| libStub.getShelf(requestObject, {}, {}, (err?: {}, result?: {}) => { | |||
| assert.strictEqual(spy.getCall(0).returnValue?.queryString, ''); | |||
| assert.doesNotMatch( | |||
There was a problem hiding this comment.
Not sure what this is testing (what is $alt)? If possible, could we search for what we are looking for? This will also allow readability in the code (it will help future maintainers understand what the behavior should look like). Or, if we also want to test it does not contain $alt, let's also add a positive-looking test. It's also a bit confusing to test the first call OR an empty string?
There was a problem hiding this comment.
Good call. I reverted these changes. They were artifacts from a different implementation, but I left them in because I thought they made the tests more robust. However, with your comment I see the downsides.
gax/test/unit/regapic.ts
Outdated
| @@ -237,7 +245,10 @@ describe('REGAPIC', () => { | |||
| ); | |||
| gaxGrpc.createStub(libraryService, stubOptions).then(libStub => { | |||
| libStub.createShelf(requestObject, {}, {}, (err?: {}) => { | |||
| assert.strictEqual(spy.getCall(0).returnValue?.queryString, ''); | |||
| assert.doesNotMatch( | |||
gax/test/unit/regapic.ts
Outdated
| .then(libStub => { | ||
| libStub.getShelf(requestObject, {}, {}, (err?: {}, result?: {}) => { | ||
| assert.match( | ||
| spy.getCall(0).returnValue?.queryString ?? '', |
There was a problem hiding this comment.
In general I don't think we should be unsure of what we're matching (I don't think we should be using the nullish coalescing operator in tests)
There was a problem hiding this comment.
I updated this. It was just a way to force the argument to be a string.
| @@ -569,4 +583,86 @@ describe('REGAPIC', () => { | |||
| }, /* catch: */ done); | |||
| }); | |||
| }); | |||
|
|
|||
There was a problem hiding this comment.
should we also add a test that confirms the response was minified? it seems like we're testing data integrity and that we're sending the prettyPrint request, but maybe we could also see if the actual size of the response is smaller. We have a similar test here that could be useful!
There was a problem hiding this comment.
I agree that this test would be useful, but I think it requires a system test that hits a real service endpoint. It's not clear to me how to implement that.
danielbankhead
added
the
owlbot:run
gcf-owl-bot
bot
removed
the
owlbot:run
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕