From fe50ca83f622095932bed68c8ac85bf7fea6da9b Mon Sep 17 00:00:00 2001 From: William Chargin Date: Thu, 1 Nov 2018 11:04:49 -0700 Subject: [PATCH] mirror: allow blacklisting objects by ID (#972) Summary: This enables us to deal with GraphQL remotes that violate their contract guarantees and provide a node of the wrong type. An instance in which the GitHub GraphQL API does this is documented here: A Mirror cache is only valid for a fixed set of blacklisted IDs. This is necessary to ensure consistency. If the set of blacklisted IDs changes, simply remove the existing database and download again from scratch. Test Plan: Unit tests added, with full coverage. wchargin-branch: mirror-blacklist --- src/graphql/mirror.js | 52 +++++++++++++---- src/graphql/mirror.test.js | 117 ++++++++++++++++++++++++++++++++++++- 2 files changed, 156 insertions(+), 13 deletions(-) diff --git a/src/graphql/mirror.js b/src/graphql/mirror.js index fd3d9a7..2e325b8 100644 --- a/src/graphql/mirror.js +++ b/src/graphql/mirror.js @@ -32,6 +32,7 @@ export class Mirror { +_db: Database; +_schema: Schema.Schema; +_schemaInfo: SchemaInfo; + +_blacklistedIds: {|+[Schema.ObjectId]: true|}; /** * Create a GraphQL mirror using the given database connection and @@ -39,21 +40,41 @@ export class Mirror { * * The connection must be to a database that either (a) is empty and * unused, or (b) has been previously used for a GraphQL mirror with - * an identical GraphQL schema. The database attached to the - * connection must not be modified by any other clients. In other - * words, passing a connection to this constructor entails transferring - * ownership of the attached database to this module. + * an identical GraphQL schema and options. The database attached to + * the connection must not be modified by any other clients. In other + * words, passing a connection to this constructor entails + * transferring ownership of the attached database to this module. * * If the database attached to the connection has been used with an * incompatible GraphQL schema or an outdated version of this module, * an error will be thrown and the database will remain unmodified. + * + * If a list of blacklisted IDs is provided, then any object with + * matching ID will be treated as `null` when it appears as the target + * of a node reference, nested node reference, or connection entry. + * This can be used to work around bugs in remote schemas. */ - constructor(db: Database, schema: Schema.Schema): void { + constructor( + db: Database, + schema: Schema.Schema, + options?: {|+blacklistedIds?: $ReadOnlyArray|} + ): void { if (db == null) throw new Error("db: " + String(db)); if (schema == null) throw new Error("schema: " + String(schema)); + const fullOptions = { + ...{blacklistedIds: []}, + ...(options || {}), + }; this._db = db; this._schema = schema; this._schemaInfo = _buildSchemaInfo(this._schema); + this._blacklistedIds = (() => { + const result: {|[Schema.ObjectId]: true|} = ({}: any); + for (const id of fullOptions.blacklistedIds) { + result[id] = true; + } + return result; + })(); this._initialize(); } @@ -153,23 +174,29 @@ export class Mirror { // interpreted. If you've made a change and you're not sure whether // it requires bumping the version, bump it: requiring some extra // one-time cache resets is okay; doing the wrong thing is not. - const blob = stringify({version: "MIRROR_v2", schema: this._schema}); + const blob = stringify({ + version: "MIRROR_v3", + schema: this._schema, + options: { + blacklistedIds: this._blacklistedIds, + }, + }); const db = this._db; _inTransaction(db, () => { // We store the metadata in a singleton table `meta`, whose unique row // has primary key `0`. Only the first ever insert will succeed; we - // are locked into the first schema. + // are locked into the first config. db.prepare( dedent`\ CREATE TABLE IF NOT EXISTS meta ( zero INTEGER PRIMARY KEY, - schema TEXT NOT NULL + config TEXT NOT NULL ) ` ).run(); const existingBlob: string | void = db - .prepare("SELECT schema FROM meta") + .prepare("SELECT config FROM meta") .pluck() .get(); if (existingBlob === blob) { @@ -177,10 +204,11 @@ export class Mirror { return; } else if (existingBlob !== undefined) { throw new Error( - "Database already populated with incompatible schema or version" + "Database already populated with " + + "incompatible schema, options, or version" ); } - db.prepare("INSERT INTO meta (zero, schema) VALUES (0, ?)").run(blob); + db.prepare("INSERT INTO meta (zero, config) VALUES (0, ?)").run(blob); // First, create those tables that are independent of the schema. const structuralTables = [ @@ -450,6 +478,8 @@ export class Mirror { ): Schema.ObjectId | null { if (result == null) { return null; + } else if (this._blacklistedIds[result.id]) { + return null; } else { const object = {typename: result.__typename, id: result.id}; this._nontransactionallyRegisterObject(object); diff --git a/src/graphql/mirror.test.js b/src/graphql/mirror.test.js index e4bdb8e..491510d 100644 --- a/src/graphql/mirror.test.js +++ b/src/graphql/mirror.test.js @@ -187,12 +187,12 @@ describe("graphql/mirror", () => { const data = fs.readFileSync(filename).toJSON(); expect(() => new Mirror(db, schema1)).toThrow( - "incompatible schema or version" + "incompatible schema, options, or version" ); expect(fs.readFileSync(filename).toJSON()).toEqual(data); expect(() => new Mirror(db, schema1)).toThrow( - "incompatible schema or version" + "incompatible schema, options, or version" ); expect(fs.readFileSync(filename).toJSON()).toEqual(data); @@ -200,6 +200,15 @@ describe("graphql/mirror", () => { expect(fs.readFileSync(filename).toJSON()).toEqual(data); }); + it("rejects when the set of blacklisted IDs has changed", () => { + const db = new Database(":memory:"); + const schema = Schema.schema({A: Schema.object({id: Schema.id()})}); + new Mirror(db, schema, {blacklistedIds: []}); + expect(() => { + new Mirror(db, schema, {blacklistedIds: ["ominous"]}); + }).toThrow("incompatible schema, options, or version"); + }); + it("rejects a schema with SQL-unsafe type name", () => { const s = Schema; const schema0 = s.schema({ @@ -1571,6 +1580,64 @@ describe("graphql/mirror", () => { ); }).toThrow("FOREIGN KEY constraint failed"); }); + it("omits connection entries corresponding to blacklisted IDs", () => { + const db = new Database(":memory:"); + const mirror = new Mirror(db, buildGithubSchema(), { + blacklistedIds: ["ominous"], + }); + const updateId = mirror._createUpdate(new Date(123)); + mirror.registerObject({typename: "Repository", id: "repo"}); + mirror._updateConnection(updateId, "repo", "issues", { + totalCount: 3, + nodes: [ + { + __typename: "Issue", + id: "normal", + }, + { + __typename: "Issue", + id: "ominous", + }, + { + __typename: "Issue", + id: "pristine", + }, + ], + pageInfo: { + hasNextPage: false, + endCursor: "cursor", + }, + }); + expect( + db + .prepare( + "SELECT * FROM connection_entries ORDER BY connection_id, idx" + ) + .all() + ).toEqual([ + { + rowid: expect.anything(), + connection_id: expect.anything(), + idx: 1, + child_id: "normal", + }, + { + rowid: expect.anything(), + connection_id: expect.anything(), + idx: 2, + child_id: null, + }, + { + rowid: expect.anything(), + connection_id: expect.anything(), + idx: 3, + child_id: "pristine", + }, + ]); + expect(() => { + mirror.extract("ominous"); + }).toThrow('No such object: "ominous"'); + }); it("properly updates under various circumstances", () => { const db = new Database(":memory:"); const mirror = new Mirror(db, buildGithubSchema()); @@ -2183,6 +2250,52 @@ describe("graphql/mirror", () => { const post = getState(); expect(post).toEqual(pre); }); + it("omits node references corresponding to blacklisted IDs", () => { + const db = new Database(":memory:"); + const mirror = new Mirror(db, buildGithubSchema(), { + blacklistedIds: ["ominous"], + }); + const updateId = mirror._createUpdate(new Date(123)); + mirror.registerObject({typename: "IssueComment", id: "comment"}); + mirror.registerObject({typename: "Commit", id: "commit"}); + mirror._updateOwnData(updateId, [ + { + __typename: "IssueComment", + id: "comment", + body: "hmm", + author: {__typename: "User", id: "ominous"}, + }, + ]); + mirror._updateOwnData(updateId, [ + { + __typename: "Commit", + id: "commit", + oid: "deadbeef", + author: { + date: "today", + user: {__typename: "User", id: "ominous"}, + }, + }, + ]); + expect(mirror.extract("comment")).toEqual({ + __typename: "IssueComment", + id: "comment", + body: "hmm", + author: null, + }); + expect(mirror.extract("commit")).toEqual({ + __typename: "Commit", + id: "commit", + oid: "deadbeef", + author: { + date: "today", + user: null, + }, + }); + expect(() => { + mirror.extract("ominous"); + }).toThrow('No such object: "ominous"'); + }); it("snapshot test for actual GitHub queries", () => { // This test emits as a snapshot a valid query against GitHub's // GraphQL API. You can copy-and-paste the snapshot into