diff --git a/src/graphql/mirror.js b/src/graphql/mirror.js index ae68a74..130c804 100644 --- a/src/graphql/mirror.js +++ b/src/graphql/mirror.js @@ -34,6 +34,7 @@ export class Mirror { +_schema: Schema.Schema; +_schemaInfo: SchemaInfo; +_blacklistedIds: {|+[Schema.ObjectId]: true|}; + +_guessTypename: (Schema.ObjectId) => Schema.Typename | null; /** * Create a GraphQL mirror using the given database connection and @@ -54,16 +55,30 @@ export class Mirror { * 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. + * + * If `guessTypename` is provided, it should attempt to guess the + * typename of an object given its ID, returning `null` if no guess + * can be made. This will be used as a cross-check against results + * returned from the server to provide an early warning in cases where + * the results differ, potentially due to server-side contract + * violations. This option only affects console warnings, not the + * final state of the mirror. */ constructor( db: Database, schema: Schema.Schema, - options?: {|+blacklistedIds?: $ReadOnlyArray|} + options?: {| + +blacklistedIds?: $ReadOnlyArray, + +guessTypename?: (Schema.ObjectId) => Schema.Typename | null, + |} ): void { if (db == null) throw new Error("db: " + String(db)); if (schema == null) throw new Error("schema: " + String(schema)); const fullOptions = { - ...{blacklistedIds: []}, + ...{ + blacklistedIds: [], + guessTypename: () => null, + }, ...(options || {}), }; this._db = db; @@ -76,6 +91,7 @@ export class Mirror { } return result; })(); + this._guessTypename = fullOptions.guessTypename; this._initialize(); } @@ -373,7 +389,14 @@ export class Mirror { +id: Schema.ObjectId, |}): void { _inTransaction(this._db, () => { - this._nontransactionallyRegisterObject(object); + this._nontransactionallyRegisterObject(object, (guess) => { + const s = JSON.stringify; + const message = + `object ${s(object.id)} ` + + `looks like it should have type ${s(guess)}, ` + + `not ${s(object.typename)}`; + return message; + }); }); } @@ -382,10 +405,13 @@ export class Mirror { * methods may call this method as a subroutine in a larger * transaction. */ - _nontransactionallyRegisterObject(object: {| - +typename: Schema.Typename, - +id: Schema.ObjectId, - |}): void { + _nontransactionallyRegisterObject( + object: {| + +typename: Schema.Typename, + +id: Schema.ObjectId, + |}, + guessMismatchMessage: (guess: Schema.Typename) => string + ): void { const db = this._db; const {typename, id} = object; @@ -414,6 +440,11 @@ export class Mirror { ); } + const guess = this._guessTypename(object.id); + if (guess != null && guess !== object.typename) { + console.warn("Warning: " + guessMismatchMessage(guess)); + } + this._db .prepare( dedent`\ @@ -475,7 +506,8 @@ export class Mirror { * See: `registerObject`. */ _nontransactionallyRegisterNodeFieldResult( - result: NodeFieldResult + result: NodeFieldResult, + context: () => string ): Schema.ObjectId | null { if (result == null) { return null; @@ -483,7 +515,14 @@ export class Mirror { return null; } else { const object = {typename: result.__typename, id: result.id}; - this._nontransactionallyRegisterObject(object); + this._nontransactionallyRegisterObject(object, (guess) => { + const s = JSON.stringify; + const message = + `object ${s(object.id)} ` + + `looks like it should have type ${s(guess)}, ` + + `but the server claims that it has type ${s(object.typename)}`; + return `${context()}: ${message}`; + }); return object.id; } } @@ -1078,7 +1117,16 @@ export class Mirror { ` ); for (const node of queryResult.nodes) { - const childId = this._nontransactionallyRegisterNodeFieldResult(node); + const childId = this._nontransactionallyRegisterNodeFieldResult( + node, + () => { + const s = JSON.stringify; + return ( + "when processing " + + `${s(fieldname)} connection of object ${s(objectId)}` + ); + } + ); const idx = nextIndex++; addEntry.run({connectionId, idx, childId}); } @@ -1401,8 +1449,13 @@ export class Mirror { `of type ${s(typename)} (got ${(link: empty)})` ); } - const childId = this._nontransactionallyRegisterNodeFieldResult(link); const parentId = entry.id; + const childId = this._nontransactionallyRegisterNodeFieldResult( + link, + () => + "when setting " + + `${typename}[${JSON.stringify(parentId)}].${fieldname}` + ); updateLink({parentId, fieldname, childId}); } @@ -1429,11 +1482,14 @@ export class Mirror { `of type ${s(typename)} (got ${(link: empty)})` ); } - const childId = this._nontransactionallyRegisterNodeFieldResult( - link - ); const fieldname = `${nestFieldname}.${childFieldname}`; const parentId = entry.id; + const childId = this._nontransactionallyRegisterNodeFieldResult( + link, + () => + "when setting " + + `${typename}[${JSON.stringify(parentId)}].${fieldname}` + ); updateLink({parentId, fieldname, childId}); } } diff --git a/src/graphql/mirror.test.js b/src/graphql/mirror.test.js index 554ef93..44e5e11 100644 --- a/src/graphql/mirror.test.js +++ b/src/graphql/mirror.test.js @@ -72,6 +72,11 @@ describe("graphql/mirror", () => { body: s.primitive(), author: s.node("Actor"), }), + Reaction: s.object({ + id: s.id(), + content: s.primitive(s.nonNull("ReactionContent")), + user: s.node("User"), + }), Commit: s.object({ id: s.id(), oid: s.primitive(), @@ -145,6 +150,7 @@ describe("graphql/mirror", () => { "connections", "connection_entries", // Primitive data tables per OBJECT type (no UNIONs) + "primitives_Reaction", "primitives_Repository", "primitives_Issue", "primitives_IssueComment", @@ -3248,6 +3254,215 @@ describe("graphql/mirror", () => { }); }); }); + + describe("end-to-end typename guessing", () => { + beforeAll(() => { + jest.spyOn(console, "warn").mockImplementation(() => {}); + }); + function spyWarn(): JestMockFn<[string], void> { + return ((console.warn: any): JestMockFn); + } + beforeEach(() => { + spyWarn().mockReset(); + }); + afterAll(() => { + spyWarn().mockRestore(); + }); + + // Guess typenames from object IDs like "#123". + function guessTypename( + objectId: Schema.ObjectId + ): Schema.Typename | null { + const match = objectId.match(/^<([A-Za-z_-]*)>#[0-9]*$/); + return match ? match[1] : null; + } + + function buildMirror() { + const db = new Database(":memory:"); + const schema = buildGithubSchema(); + return new Mirror(db, schema, {guessTypename}); + } + + it("catches bad top-level links", async () => { + const mirror = buildMirror(); + // Reaction.user can actually be an organization. + mirror.registerObject({typename: "Reaction", id: "#123"}); + const postQuery = jest.fn(); + postQuery.mockImplementationOnce(() => + Promise.resolve({ + owndata_0: [ + { + __typename: "Reaction", + id: "#123", + content: "THUMBS_DOWN", + user: {__typename: "User", id: "#456"}, + }, + ], + }) + ); + postQuery.mockImplementationOnce(() => + Promise.resolve({ + owndata_0: [ + { + __typename: "User", + id: "#456", + url: "https://example.com/org/456", + login: "org456", + }, + ], + }) + ); + postQuery.mockImplementationOnce(() => + Promise.reject("Should not get here.") + ); + await mirror.update(postQuery, { + nodesOfTypeLimit: 2, + nodesLimit: 3, + connectionLimit: 4, + connectionPageSize: 5, + since: new Date(0), + now: () => new Date(0), + }); + expect(console.warn).toHaveBeenCalledTimes(1); + expect(console.warn).toHaveBeenCalledWith( + 'Warning: when setting Reaction["#123"].user: ' + + 'object "#456" looks like it should have ' + + 'type "Organization", but the server claims that it has ' + + 'type "User"' + ); + }); + + it("catches bad nested links", async () => { + const mirror = buildMirror(); + // GitActor.user can actually be a bot. + mirror.registerObject({typename: "Commit", id: "#123"}); + const postQuery = jest.fn(); + postQuery.mockImplementationOnce(() => + Promise.resolve({ + owndata_0: [ + { + __typename: "Commit", + id: "#123", + oid: "9cba0e9e212a287ce26e8d7c2d273e1025c9f9bf", + author: { + date: "yesterday", + user: {__typename: "User", id: "#456"}, + }, + }, + ], + }) + ); + postQuery.mockImplementationOnce(() => + Promise.resolve({ + owndata_0: [ + { + __typename: "User", + id: "#456", + url: "https://example.com/bot/456", + login: "bot456", + }, + ], + }) + ); + postQuery.mockImplementationOnce(() => + Promise.reject("Should not get here.") + ); + await mirror.update(postQuery, { + nodesOfTypeLimit: 2, + nodesLimit: 3, + connectionLimit: 4, + connectionPageSize: 5, + since: new Date(0), + now: () => new Date(0), + }); + expect(console.warn).toHaveBeenCalledTimes(1); + expect(console.warn).toHaveBeenCalledWith( + 'Warning: when setting Commit["#123"].author.user: ' + + 'object "#456" looks like it should have type "Bot", ' + + 'but the server claims that it has type "User"' + ); + }); + + it("catches bad connection entries", async () => { + const mirror = buildMirror(); + // Suppose that Issue.comments could actually contain Reactions. + mirror.registerObject({typename: "Issue", id: "#123"}); + const postQuery = jest.fn(); + postQuery.mockImplementationOnce(() => + Promise.resolve({ + owndata_0: [ + { + __typename: "Issue", + id: "#123", + url: "https://example.com/issue/123", + author: null, + repository: null, + title: "My twelvtythird issue", + }, + ], + node_0: { + id: "#123", + comments: { + totalCount: 1, + pageInfo: {hasNextPage: false, endCursor: "cursor:comments@1"}, + nodes: [{__typename: "IssueComment", id: "#456"}], + }, + timeline: { + totalCount: 0, + pageInfo: {hasNextPage: false, endCursor: null}, + nodes: [], + }, + }, + }) + ); + postQuery.mockImplementationOnce(() => + Promise.resolve({ + owndata_0: [ + { + __typename: "IssueComment", + id: "#456", + body: "dubious", + author: null, + }, + ], + }) + ); + postQuery.mockImplementationOnce(() => + Promise.reject("Should not get here.") + ); + await mirror.update(postQuery, { + nodesOfTypeLimit: 2, + nodesLimit: 3, + connectionLimit: 4, + connectionPageSize: 5, + since: new Date(0), + now: () => new Date(0), + }); + expect(console.warn).toHaveBeenCalledTimes(1); + expect(console.warn).toHaveBeenCalledWith( + 'Warning: when processing "comments" connection ' + + 'of object "#123": ' + + 'object "#456" looks like it should have type "User", ' + + 'but the server claims that it has type "IssueComment"' + ); + }); + + it("catches bad manual registrations", () => { + const mirror = buildMirror(); + mirror.registerObject({typename: "User", id: "#123"}); + expect(console.warn).toHaveBeenCalledTimes(1); + expect(console.warn).toHaveBeenCalledWith( + 'Warning: object "#123" looks like it should have ' + + 'type "Organization", not "User"' + ); + }); + + it("ignores null guesses", () => { + const mirror = buildMirror(); + mirror.registerObject({typename: "User", id: "~~unorthodox~~"}); + expect(console.warn).not.toHaveBeenCalled(); + }); + }); }); describe("_buildSchemaInfo", () => { @@ -3256,6 +3471,7 @@ describe("graphql/mirror", () => { expect(Object.keys(result.objectTypes).sort()).toEqual( Array.from( new Set([ + "Reaction", "Repository", "Issue", "IssueComment", diff --git a/src/plugins/github/fetchGithubRepo.js b/src/plugins/github/fetchGithubRepo.js index bbf2e16..c901946 100644 --- a/src/plugins/github/fetchGithubRepo.js +++ b/src/plugins/github/fetchGithubRepo.js @@ -54,7 +54,10 @@ export default async function fetchGithubRepo( // equals signs in file names. const dbFilename = `mirror_${Buffer.from(resolvedId).toString("hex")}.db`; const db = new Database(path.join(cacheDirectory, dbFilename)); - const mirror = new Mirror(db, schema(), {blacklistedIds: BLACKLISTED_IDS}); + const mirror = new Mirror(db, schema(), { + blacklistedIds: BLACKLISTED_IDS, + guessTypename: _guessTypename, + }); mirror.registerObject({typename: "Repository", id: resolvedId}); // These are arbitrary tuning parameters. @@ -76,6 +79,23 @@ export default async function fetchGithubRepo( return ((mirror.extract(resolvedId): any): Repository); } +// GitHub object IDs are urlsafe-base64-encoded strings that decode to +// ASCII strings of the form "123:Typename4567[...]", where the "123" +// numbers are a function only of the typename and the "4567" numbers +// are the object's database ID, and the "[...]" is either empty or a +// further section like ":commithash" for commits. +// +// See tests for `_guessTypename` for some example object IDs. +const GITHUB_ID_TYPENAME_PATTERN = /^[0-9]*:([a-z0-9_-]*[a-z_-])[0-9]+(?:[^a-z0-9_-].*)?$/i; + +export function _guessTypename( + objectId: Schema.ObjectId +): Schema.Typename | null { + const decodedId = Buffer.from(objectId, "base64").toString("utf-8"); + const match = decodedId.match(GITHUB_ID_TYPENAME_PATTERN); + return match ? match[1] : null; +} + const GITHUB_GRAPHQL_SERVER = "https://api.github.com/graphql"; type GithubResponseError = diff --git a/src/plugins/github/fetchGithubRepo.test.js b/src/plugins/github/fetchGithubRepo.test.js new file mode 100644 index 0000000..dd6f5e5 --- /dev/null +++ b/src/plugins/github/fetchGithubRepo.test.js @@ -0,0 +1,29 @@ +// @flow + +import {_guessTypename} from "./fetchGithubRepo"; + +describe("plugins/github/fetchGithubRepo", () => { + describe("_guessTypename", () => { + it("guesses a User typename", () => { + // Simple case. + const id = "MDQ6VXNlcjQzMTc4MDY="; + expect(_guessTypename(id)).toEqual("User"); + }); + it("guesses a Commit typename", () => { + // Multiple decoded parts. + const id = + "MDY6Q29tbWl0MTIwMTQ1NTcwOjljYmEwZTllMjEyYTI4N2NlMjZlOGQ3YzJkMjczZTEwMjVjOWY5YmY="; + expect(_guessTypename(id)).toEqual("Commit"); + }); + it("guesses an X509Certificate typename", () => { + // Numbers in the middle of the typename. + // (I made this object ID up; I couldn't find a real one.) + const id = "MDEyOlg1MDlDZXJ0aWZpY2F0ZTEyMzQ1"; + expect(_guessTypename(id)).toEqual("X509Certificate"); + }); + it("fails cleanly on an unknown ID format", () => { + const id = ":spooky:"; + expect(_guessTypename(id)).toBe(null); + }); + }); +});