Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass apiResponse along with errors #741

Merged
merged 2 commits into from
Jul 24, 2015

Conversation

stephenplusplus
Copy link
Contributor

As introduced in #706 (comment), this fixes a couple of things:

  1. "Decorate" request options in all cases.

    Previously, we were decorating requests only when we were making them automatically for the user. That means our UA header wasn't on the authorizedReqOpts that were passed to "onAuthorized(function (authorizedReqOpts) {})". So, this just decorates requests sooner so that anytime we say we "authorized a request", we also put on the UA string and cleaned up gcloud-node-only properties that may have leaked.

  2. Pass back apiResponse when there is an error.

    All of the callbacks our API accepts are advertised as receiving an "apiResponse" even in the event of an error. The problem was our universal response handler ("util.handleResp") was not giving back the response body if an error was caught, which is what becomes the "apiResponse". So now, it works as advertised.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 24, 2015
@callmehiphop
Copy link
Contributor

This might sound crazy, but should we go through the system-tests and add assertions to test that the proper arguments are being passed in?

Otherwise everything looks good to me!

@stephenplusplus
Copy link
Contributor Author

Not crazy! The argument passing is thoroughly tested in the unit tests. We ran into this problem because our unit tests are so decoupled that we stub what handleResp is supposed to return. So, we have validation that "if there is a response body provided, it is sent to the user's callback". The issue was just without our stubs, handleResp was only actually returning one argument if there was an error (the error, not the response). The user's callback would receive undefined for apiResp.

The leak was in handleResp, where we weren't testing properly what was happening when there was an error. Now that it always returns all of the arguments (?error, body, response), and we have it tested, we can safely call this fixed.

callmehiphop added a commit that referenced this pull request Jul 24, 2015
@callmehiphop callmehiphop merged commit db12149 into googleapis:master Jul 24, 2015
@callmehiphop
Copy link
Contributor

👍 thanks!

sofisl pushed a commit that referenced this pull request Jan 10, 2023
* fix: do not import the whole google-gax from proto JS (#1553)

fix: use google-gax v3.3.0
Source-Link: googleapis/synthtool@c73d112
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-nodejs:latest@sha256:b15a6f06cc06dcffa11e1bebdf1a74b6775a134aac24a0f86f51ddf728eb373e

* fix(deps): update google-gax

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Co-authored-by: Alexander Fenster <fenster@google.com>
sofisl pushed a commit that referenced this pull request Jan 10, 2023
🤖 I have created a release *beep* *boop*
---


## [4.2.0](googleapis/nodejs-dlp@v4.1.1...v4.2.0) (2022-09-22)


### Features

* Add Deidentify action ([#742](googleapis/nodejs-dlp#742)) ([27bb912](googleapis/nodejs-dlp@27bb912))


### Bug Fixes

* Do not import the whole google-gax from proto JS ([#1553](https://github.com/googleapis/nodejs-dlp/issues/1553)) ([#741](googleapis/nodejs-dlp#741)) ([655d6af](googleapis/nodejs-dlp@655d6af))
* Preserve default values in x-goog-request-params header ([#746](googleapis/nodejs-dlp#746)) ([7c53b9f](googleapis/nodejs-dlp@7c53b9f))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement. core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants