[go: nahoru, domu]
Skip to content

Commit

Permalink
remove request module from database helpers (firebase#2828)
Browse files Browse the repository at this point in the history 
* replace request usage with apiv2

* formatting

* use special handling for non-json response

* fix database get to be more resiliant

* add database tests to client-integration

* changelog

* resolve in the else case too

* Update scripts/client-integration-tests/tests.ts

Co-authored-by: Yuchen Shi <yuchenshi@google.com>

* formatting

Co-authored-by: Yuchen Shi <yuchenshi@google.com>
  • Loading branch information
@bkendall @yuchenshi
bkendall and yuchenshi committed Nov 17, 2020
1 parent eda3c10 commit f88920f
Show file tree
Hide file tree
Showing 7 changed files with 95 additions and 99 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- Fixes issue where `database:get` would not completely finish writing to the output file.
30 changes: 29 additions & 1 deletion scripts/client-integration-tests/tests.ts
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
import { expect } from "chai";
import { join } from "path";
import { writeFileSync, unlinkSync } from "fs";
import { readFileSync, writeFileSync, unlinkSync } from "fs";
import * as uuid from "uuid";
import * as tmp from "tmp";

import firebase = require("../../src");

tmp.setGracefulCleanup();

// Typescript doesn't like calling functions on `firebase`.
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const client: any = firebase;
Expand Down Expand Up @@ -85,3 +89,27 @@ describe("apps:sdkconfig", () => {
expect(config.sdkConfig.appId).to.equal(appID);
});
});

describe("database:set|get|remove", () => {
it("should be able to interact with the database", async () => {
const opts = { project: process.env.FBTOOLS_TARGET_PROJECT };
const path = `/${uuid()}`;
const data = { foo: "bar" };

await client.database.set(
path,
Object.assign({ data: JSON.stringify(data), confirm: true }, opts)
);

// Have to read to a file in order to get data.
const file = tmp.fileSync();

await client.database.get(path, Object.assign({ output: file.name }, opts));
expect(JSON.parse(readFileSync(file.name).toString())).to.deep.equal(data);

await client.database.remove(path, Object.assign({ confirm: true }, opts));

await client.database.get(path, Object.assign({ output: file.name }, opts));
expect(JSON.parse(readFileSync(file.name, "utf-8"))).to.equal(null);
}).timeout(10 * 1e3);
});
32 changes: 18 additions & 14 deletions src/commands/database-get.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,19 +128,23 @@ export default new Command("database:get <path>")
throw responseToError({ statusCode: res.status }, d);
}

res.body.pipe(outStream);
// Tack on a single newline at the end of the stream.
res.body.once("end", () => {
if (outStream === process.stdout) {
// `stdout` can simply be written to.
outStream.write("\n");
} else if (outStream instanceof fs.WriteStream) {
// .pipe closes the output file stream, so we need to re-open the file.
const s = fs.createWriteStream(options.output, { flags: "a" });
s.write("\n");
s.close();
} else {
logger.debug("[database:get] Could not write line break to outStream");
}
res.body.pipe(outStream, { end: false });

return new Promise((resolve) => {
// Tack on a single newline at the end of the stream.
res.body.once("end", () => {
if (outStream === process.stdout) {
// `stdout` can simply be written to.
outStream.write("\n");
resolve();
} else if (outStream instanceof fs.WriteStream) {
outStream.write("\n");
outStream.on("close", () => resolve());
outStream.close();
} else {
logger.debug("[database:get] Could not write line break to outStream");
resolve();
}
});
});
});
51 changes: 13 additions & 38 deletions src/database/listRemote.ts
View file
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
import * as request from "request";
import { Response } from "request";
import * as responseToError from "../responseToError";
import * as utils from "../utils";
import { FirebaseError } from "../error";
import { Client } from "../apiv2";
import { URL } from "url";
import * as logger from "../logger";
import * as api from "../api";
import * as utils from "../utils";

export interface ListRemote {
/**
Expand All @@ -24,15 +21,20 @@ export interface ListRemote {
}

export class RTDBListRemote implements ListRemote {
constructor(private instance: string, private host: string) {}
private apiClient: Client;

constructor(private instance: string, private host: string) {
const url = new URL(`${utils.addSubdomain(this.host, this.instance)}`);
this.apiClient = new Client({ urlPrefix: url.origin, auth: true });
}

async listPath(
path: string,
numSubPath: number,
startAfter?: string,
timeout?: number
): Promise<string[]> {
const url = `${utils.addSubdomain(this.host, this.instance)}${path}.json`;
const url = new URL(`${utils.addSubdomain(this.host, this.instance)}${path}.json`);

const params: any = {
shallow: true,
Expand All @@ -46,37 +48,10 @@ export class RTDBListRemote implements ListRemote {
}

const t0 = Date.now();
const reqOptionsWithToken = await api.addRequestHeaders({ url });
reqOptionsWithToken.qs = params;
const paths = await new Promise<string[]>((resolve, reject) => {
request.get(reqOptionsWithToken, (err: Error, res: Response, body: any) => {
if (err) {
return reject(
new FirebaseError("Unexpected error while listing subtrees", {
exit: 2,
original: err,
})
);
} else if (res.statusCode >= 400) {
return reject(responseToError(res, body));
}
let data;
try {
data = JSON.parse(body);
} catch (e) {
return reject(
new FirebaseError("Malformed JSON response in shallow get ", {
exit: 2,
original: e,
})
);
}
if (data) {
return resolve(Object.keys(data));
}
return resolve([]);
});
const res = await this.apiClient.get<{ [key: string]: unknown }>(url.pathname, {
queryParams: params,
});
const paths = Object.keys(res.body);
const dt = Date.now() - t0;
logger.debug(`[database] sucessfully fetched ${paths.length} path at ${path} ${dt}`);
return paths;
Expand Down
11 changes: 6 additions & 5 deletions src/database/remove.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@ export default class DatabaseRemove {
/**
* Construct a new RTDB delete operation.
*
* @constructor
* @param instance RTBD instance ID.
* @param path path to delete.
* @param host db host.
*/
constructor(instance: string, path: string, host: string) {
this.path = path;
Expand Down Expand Up @@ -58,11 +58,12 @@ export default class DatabaseRemove {
*
* listNumSubPath starts with INITIAL_LIST_NUM_SUB_PATH and grow expontentially until MAX_LIST_NUM_SUB_PATH.
*
* @param path path to delete
* @return true if this path is small (Does not exceed writeSizeLimit of tiny)
*/
private async deletePath(path: string): Promise<boolean> {
if (await this.deleteJobStack.run(() => this.remote.deletePath(path))) {
return Promise.resolve(true);
return true;
}
let listNumSubPath = INITIAL_LIST_NUM_SUB_PATH;
// The range of batchSize to gradually narrow down.
Expand All @@ -74,7 +75,7 @@ export default class DatabaseRemove {
this.listRemote.listPath(path, listNumSubPath)
);
if (subPathList.length === 0) {
return Promise.resolve(false);
return false;
}
const chunks = chunkList(subPathList, batchSize);
let nSmallChunks = 0;
Expand Down Expand Up @@ -115,11 +116,11 @@ export default class DatabaseRemove {
return this.deletePath(pathLib.join(path, subPaths[0]));
}
if (await this.deleteJobStack.run(() => this.remote.deleteSubPath(path, subPaths))) {
return Promise.resolve(true);
return true;
}
const mid = Math.floor(subPaths.length / 2);
await this.deleteSubPath(path, subPaths.slice(0, mid));
await this.deleteSubPath(path, subPaths.slice(mid));
return Promise.resolve(false);
return false;
}
}
65 changes: 26 additions & 39 deletions src/database/removeRemote.ts
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
import * as request from "request";
import { Response } from "request";
import * as utils from "../utils";
import { FirebaseError } from "../error";
import { Client } from "../apiv2";
import { URL } from "url";
import * as logger from "../logger";
import * as api from "../api";
import * as utils from "../utils";

export interface RemoveRemote {
/**
Expand All @@ -23,10 +21,14 @@ export interface RemoveRemote {
export class RTDBRemoveRemote implements RemoveRemote {
private instance: string;
private host: string;
private apiClient: Client;

constructor(instance: string, host: string) {
this.instance = instance;
this.host = host;

const url = new URL(utils.getDatabaseUrl(this.host, this.instance, "/"));
this.apiClient = new Client({ urlPrefix: url.origin, auth: true });
}

deletePath(path: string): Promise<boolean> {
Expand All @@ -41,41 +43,26 @@ export class RTDBRemoveRemote implements RemoveRemote {
return this.patch(path, body, `${subPaths.length} subpaths`);
}

private patch(path: string, body: any, note: string): Promise<boolean> {
private async patch(path: string, body: any, note: string): Promise<boolean> {
const t0 = Date.now();
return new Promise((resolve, reject) => {
const url = utils.getDatabaseUrl(
this.host,
this.instance,
path + ".json?print=silent&writeSizeLimit=tiny"
);
return api
.addRequestHeaders({
url,
body,
json: true,
})
.then((reqOptionsWithToken) => {
request.patch(reqOptionsWithToken, (err: Error, res: Response, resBody: any) => {
if (err) {
return reject(
new FirebaseError(`Unexpected error while removing data at ${path}`, {
exit: 2,
original: err,
})
);
}
const dt = Date.now() - t0;
if (res.statusCode >= 400) {
logger.debug(
`[database] Failed to remove ${note} at ${path} time: ${dt}ms, will try recursively chunked deletes.`
);
return resolve(false);
}
logger.debug(`[database] Sucessfully removed ${note} at ${path} time: ${dt}ms`);
return resolve(true);
});
});
const url = new URL(utils.getDatabaseUrl(this.host, this.instance, path + ".json"));
const queryParams = { print: "silent", writeSizeLimit: "tiny" };
const res = await this.apiClient.request({
method: "PATCH",
path: url.pathname,
body,
queryParams,
responseType: "stream",
resolveOnHTTPError: true,
});
const dt = Date.now() - t0;
if (res.status >= 400) {
logger.debug(
`[database] Failed to remove ${note} at ${path} time: ${dt}ms, will try recursively chunked deletes.`
);
return false;
}
logger.debug(`[database] Sucessfully removed ${note} at ${path} time: ${dt}ms`);
return true;
}
}
4 changes: 2 additions & 2 deletions src/test/database/listRemote.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@ import { expect } from "chai";
import * as nock from "nock";

import * as utils from "../../utils";
import * as api from "../../api";
import { realtimeOrigin } from "../../api";
import { RTDBListRemote } from "../../database/listRemote";
const HOST = "https://firebaseio.com";

describe("ListRemote", () => {
const instance = "fake-db";
const remote = new RTDBListRemote(instance, HOST);
const serverUrl = utils.addSubdomain(api.realtimeOrigin, instance);
const serverUrl = utils.addSubdomain(realtimeOrigin, instance);

afterEach(() => {
nock.cleanAll();
Expand Down

0 comments on commit f88920f

Please sign in to comment.