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

Preserve response body on api requests with XML response types. #4180

Merged
merged 7 commits into from
Feb 17, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -4,4 +4,5 @@
- Fixes broken functions:config:clone command (#4173).
- Fixes issue where `auth:import` would fail when reading a JSON file. (#4157)
- Fixes issue where custom claims added in Auth Emulator UI was not properly shown.
- Improves handling of API requests with XML responses (#4180).
- Updates the underlying request library in Hosting deploys and uses project-scoped URLs. (#2558)
4 changes: 3 additions & 1 deletion src/apiv2.ts
Expand Up @@ -23,7 +23,7 @@ interface BaseRequestOptions<T> extends VerbOptions {
method: HttpMethod;
path: string;
body?: T | string | NodeJS.ReadableStream;
responseType?: "json" | "stream";
responseType?: "json" | "stream" | "xml";
redirect?: "error" | "follow" | "manual";
compress?: boolean;
}
Expand Down Expand Up @@ -415,6 +415,8 @@ export class Client {
throw new FirebaseError(`Unable to parse JSON: ${err}`);
}
}
} else if (options.responseType === "xml") {
body = (await res.text()) as unknown as ResT;
} else if (options.responseType === "stream") {
body = res.body as unknown as ResT;
} else {
Expand Down
1 change: 1 addition & 0 deletions src/gcp/storage.ts
Expand Up @@ -158,6 +158,7 @@ export async function upload(
method: "PUT",
path: url.pathname,
queryParams: url.searchParams,
responseType: "xml",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgive me a silly question: could we not just add accept: application/json to get JSON responses from this API? Or does it exclusively respond in XML?

It feels a little weird to add support for XML for a single place - not that it's wrong, but everywhere else returns JSON, so it's an outlier...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's strictly an XML endpoint we are using since the URL returned by GCF when we call "generateUploadURL" works against it:

https://cloud.google.com/storage/docs/xml-api/overview

We could do some work to move everything to JSON API

https://cloud.google.com/storage/docs/json_api/v1/buckets/patch

But that's gonna be a lot more work than this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Looks like XML is the encoding of choice over JSON in the storage API world)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoops more correct answer - signed URL only supports XML API:

https://cloud.google.com/storage/docs/access-control/signed-urls

headers: {
"content-type": "application/zip",
...extraHeaders,
Expand Down
24 changes: 16 additions & 8 deletions src/responseToError.js
Expand Up @@ -4,18 +4,26 @@ const _ = require("lodash");
const { FirebaseError } = require("./error");

module.exports = function (response, body) {
if (typeof body === "string" && response.statusCode === 404) {
body = {
error: {
message: "Not Found",
},
};
}

if (response.statusCode < 400) {
return null;
}

if (typeof body === "string") {
if (response.statusCode === 404) {
body = {
error: {
message: "Not Found",
},
};
} else {
body = {
error: {
message: body,
},
};
}
}

if (typeof body !== "object") {
try {
body = JSON.parse(body);
Expand Down
30 changes: 30 additions & 0 deletions src/test/apiv2.spec.ts
Expand Up @@ -394,6 +394,36 @@ describe("apiv2", () => {
expect(nock.isDone()).to.be.true;
});

it("should preserve XML messages", async () => {
const xml = "<?xml version='1.0' encoding='UTF-8'?><Message>Hello!</Message>";
nock("https://example.com").get("/path/to/foo").reply(200, xml);

const c = new Client({ urlPrefix: "https://example.com" });
const r = await c.request({
method: "GET",
path: "/path/to/foo",
responseType: "xml",
});
expect(r.body).to.deep.equal(xml);
expect(nock.isDone()).to.be.true;
});

it("should preserve XML messages on error", async () => {
const xml =
"<?xml version='1.0' encoding='UTF-8'?><Error><Code>EntityTooLarge</Code></Error>";
nock("https://example.com").get("/path/to/foo").reply(400, xml);

const c = new Client({ urlPrefix: "https://example.com" });
await expect(
c.request({
method: "GET",
path: "/path/to/foo",
responseType: "xml",
})
).to.eventually.be.rejectedWith(FirebaseError, /EntityTooLarge/);
expect(nock.isDone()).to.be.true;
});

describe("with a proxy", () => {
let proxyServer: Server;
let targetServer: Server;
Expand Down