mirror: guess typenames and warn on mismatch (#1337)
Summary: The format of GitHub’s GraphQL object IDs is explicitly opaque, and so we must not introspect them in any way that would influence our results. But it seems reasonable to introspect these IDs solely for diagnostic purposes, enabling us to proactively detect GitHub’s contract violations while we still have useful information about the root cause. This commit adds an optional `guessTypename` option to the Mirror constructor, which accepts a function that attempts to guess an object’s typename based on its ID. If the guess differs from what the server claims, we continue on as before, but omit a console warning to help diagnose the issue more quickly. Resolves #1336. See that issue for details. Test Plan: Unit tests for `mirror.js` updated, retaining full coverage. To test manually, revert #1335, then load `quasarframework/quasar-cli`. Note that it emits the following warning before failing: > Warning: when setting Reaction["MDg6UmVhY3Rpb24zNDUxNjA2MQ=="].user: > object "MDEyOk9yZ2FuaXphdGlvbjQzMDkzODIw" looks like it should have > type "Organization", but the server claims that it has type "User" Unit tests for the GitHub typename guesser added as well. Running `yarn test --full` passes. wchargin-branch: mirror-guess-typenames
This commit is contained in:
William Chargin
GitHub
parent
8f9e967496
commit
7d3d24e0ec
|
|
@ -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<Schema.ObjectId>|}
|
||||
options?: {|
|
||||
+blacklistedIds?: $ReadOnlyArray<Schema.ObjectId>,
|
||||
+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});
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<any, void>);
|
||||
}
|
||||
beforeEach(() => {
|
||||
spyWarn().mockReset();
|
||||
});
|
||||
afterAll(() => {
|
||||
spyWarn().mockRestore();
|
||||
});
|
||||
|
||||
// Guess typenames from object IDs like "<Commit>#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: "<Reaction>#123"});
|
||||
const postQuery = jest.fn();
|
||||
postQuery.mockImplementationOnce(() =>
|
||||
Promise.resolve({
|
||||
owndata_0: [
|
||||
{
|
||||
__typename: "Reaction",
|
||||
id: "<Reaction>#123",
|
||||
content: "THUMBS_DOWN",
|
||||
user: {__typename: "User", id: "<Organization>#456"},
|
||||
},
|
||||
],
|
||||
})
|
||||
);
|
||||
postQuery.mockImplementationOnce(() =>
|
||||
Promise.resolve({
|
||||
owndata_0: [
|
||||
{
|
||||
__typename: "User",
|
||||
id: "<Organization>#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["<Reaction>#123"].user: ' +
|
||||
'object "<Organization>#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: "<Commit>#123"});
|
||||
const postQuery = jest.fn();
|
||||
postQuery.mockImplementationOnce(() =>
|
||||
Promise.resolve({
|
||||
owndata_0: [
|
||||
{
|
||||
__typename: "Commit",
|
||||
id: "<Commit>#123",
|
||||
oid: "9cba0e9e212a287ce26e8d7c2d273e1025c9f9bf",
|
||||
author: {
|
||||
date: "yesterday",
|
||||
user: {__typename: "User", id: "<Bot>#456"},
|
||||
},
|
||||
},
|
||||
],
|
||||
})
|
||||
);
|
||||
postQuery.mockImplementationOnce(() =>
|
||||
Promise.resolve({
|
||||
owndata_0: [
|
||||
{
|
||||
__typename: "User",
|
||||
id: "<Bot>#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["<Commit>#123"].author.user: ' +
|
||||
'object "<Bot>#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: "<Issue>#123"});
|
||||
const postQuery = jest.fn();
|
||||
postQuery.mockImplementationOnce(() =>
|
||||
Promise.resolve({
|
||||
owndata_0: [
|
||||
{
|
||||
__typename: "Issue",
|
||||
id: "<Issue>#123",
|
||||
url: "https://example.com/issue/123",
|
||||
author: null,
|
||||
repository: null,
|
||||
title: "My twelvtythird issue",
|
||||
},
|
||||
],
|
||||
node_0: {
|
||||
id: "<Issue>#123",
|
||||
comments: {
|
||||
totalCount: 1,
|
||||
pageInfo: {hasNextPage: false, endCursor: "cursor:comments@1"},
|
||||
nodes: [{__typename: "IssueComment", id: "<User>#456"}],
|
||||
},
|
||||
timeline: {
|
||||
totalCount: 0,
|
||||
pageInfo: {hasNextPage: false, endCursor: null},
|
||||
nodes: [],
|
||||
},
|
||||
},
|
||||
})
|
||||
);
|
||||
postQuery.mockImplementationOnce(() =>
|
||||
Promise.resolve({
|
||||
owndata_0: [
|
||||
{
|
||||
__typename: "IssueComment",
|
||||
id: "<User>#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 "<Issue>#123": ' +
|
||||
'object "<User>#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: "<Organization>#123"});
|
||||
expect(console.warn).toHaveBeenCalledTimes(1);
|
||||
expect(console.warn).toHaveBeenCalledWith(
|
||||
'Warning: object "<Organization>#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",
|
||||
|
|
|
|||
|
|
@ -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 =
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
});
|
||||
});
|
||||
});
|
||||
Loading…
Reference in New Issue