Skip to content

Commit

Permalink
feat(debugging): massively improve name collision errors (graphile#292)
Browse files Browse the repository at this point in the history
Reveals where the origin of two conflictingly named fields or types has come from, and if possible gives a hint as to how to solve the conflict.

- Fixes a bug in the inflector where a smart comment override for a single relation is applied both forwards and backwards
- Adds loads more information to `newWithHooks` and `extend` calls
- Fixes some bugs where fields can be accidentally overwritten if they have the same name - now causes a conflict instead like it should have all along
- Added a useful utility function for turning a Pg introspection type (PgClass, PgProc, PgAttribute, ...) into human readable text
- Added a useful utility function for outputting tweaked smart-comment SQL, for use in hints
- Added `comment` to relevant introspection types to store the original comment (in addition to the existing parsed description/tags)
- Gives constraint introspection types a direct reference to their associated class
- Added a `status` object to `Build` that can be used to track what the currently running hook is (useful in error messages).
- Added internal `dontSwallowErrors` setting; this is not an official interface so don't rely on it yet but it's a handy way (especially in tests) to prevent PostGraphile deliberately swallowing errors. We might wrap this and a few other settings up into a `--strict` flag in future.
  • Loading branch information
benjie authored Aug 31, 2018
1 parent 7a40d56 commit a6b4dd9
Show file tree
Hide file tree
Showing 38 changed files with 1,688 additions and 603 deletions.
2 changes: 2 additions & 0 deletions packages/graphile-build-pg/res/introspection-query.sql
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ with
pro.proname as "name",
dsc.description as "description",
pro.pronamespace as "namespaceId",
nsp.nspname as "namespaceName",
pro.proisstrict as "isStrict",
pro.proretset as "returnsSet",
case
Expand All @@ -56,6 +57,7 @@ with
from
pg_catalog.pg_proc as pro
left join pg_catalog.pg_description as dsc on dsc.objoid = pro.oid
left join pg_catalog.pg_namespace as nsp on nsp.oid = pro.pronamespace
where
pro.pronamespace in (select "id" from namespace) and
-- Currently we don’t support functions with variadic arguments. In the
Expand Down
346 changes: 199 additions & 147 deletions packages/graphile-build-pg/src/plugins/PgBackwardRelationPlugin.js

Large diffs are not rendered by default.

157 changes: 157 additions & 0 deletions packages/graphile-build-pg/src/plugins/PgBasicsPlugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ import omit, {
import makeProcField from "./makeProcField";
import parseIdentifier from "../parseIdentifier";
import viaTemporaryTable from "./viaTemporaryTable";
import chalk from "chalk";
import pickBy from "lodash/pickBy";

const defaultPgColumnFilter = (_attr, _build, _context) => true;
type Keys = Array<{
Expand All @@ -34,6 +36,8 @@ type Keys = Array<{
schema: ?string,
}>;

const identity = _ => _;

export function preventEmptyResult<
// eslint-disable-next-line flowtype/no-weak-types
O: { [key: string]: (...args: Array<any>) => string }
Expand Down Expand Up @@ -130,6 +134,62 @@ function omitWithRBACChecks(
return omit(entity, permission);
}

function describePgEntity(entity, includeAlias = true) {
const getAlias = !includeAlias
? () => ""
: () => {
const tags = pickBy(
entity.tags,
(value, key) => key === "name" || key.endsWith("Name")
);
if (Object.keys(tags).length) {
return ` (with smart comments: ${chalk.bold(
Object.keys(tags)
.map(t => `@${t} ${tags[t]}`)
.join(" | ")
)})`;
}
return "";
};

try {
if (entity.kind === "constraint") {
return `constraint ${chalk.bold(
`"${entity.name}"`
)} on ${describePgEntity(entity.class, false)}${getAlias()}`;
} else if (entity.kind === "class") {
// see pg_class.relkind https://www.postgresql.org/docs/10/static/catalog-pg-class.html
const kind =
{
c: "composite type",
f: "foreign table",
p: "partitioned table",
r: "table",
v: "view",
m: "materialized view",
}[entity.classKind] || "table-like";
return `${kind} ${chalk.bold(
`"${entity.namespaceName}"."${entity.name}"`
)}${getAlias()}`;
} else if (entity.kind === "procedure") {
return `function ${chalk.bold(
`"${entity.namespaceName}"."${entity.name}"(...args...)`
)}${getAlias()}`;
} else if (entity.kind === "attribute") {
return `column ${chalk.bold(`"${entity.name}"`)} on ${describePgEntity(
entity.class,
false
)}${getAlias()}`;
}
} catch (e) {
// eslint-disable-next-line no-console
console.error("Error occurred while attempting to debug entity:", entity);
// eslint-disable-next-line no-console
console.error(e);
}
return `entity of kind '${entity.kind}' with oid '${entity.oid}'`;
}

export default (function PgBasicsPlugin(
builder,
{
Expand All @@ -151,6 +211,81 @@ export default (function PgBasicsPlugin(
pgMakeProcField: makeProcField,
pgParseIdentifier: parseIdentifier,
pgViaTemporaryTable: viaTemporaryTable,
describePgEntity,
sqlCommentByAddingTags: (entity, tagsToAdd) => {
// NOTE: this function is NOT intended to be SQL safe; it's for
// displaying in error messages. Nonetheless if you find issues with
// SQL compatibility, please send a PR or issue.

// Ref: https://www.postgresql.org/docs/current/static/sql-syntax-lexical.html#SQL-BACKSLASH-TABLE
const escape = str =>
str.replace(
/['\\\b\f\n\r\t]/g,
chr =>
({
"\b": "\\b",
"\f": "\\f",
"\n": "\\n",
"\r": "\\r",
"\t": "\\t",
}[chr] || "\\" + chr)
);

// tagsToAdd is here twice to ensure that the keys in tagsToAdd come first, but that they also "win" any conflicts.
const tags = Object.assign({}, tagsToAdd, entity.tags, tagsToAdd);

const description = entity.description;
const tagsSql = Object.keys(tags)
.reduce((memo, tag) => {
const tagValue = tags[tag];
const valueArray = Array.isArray(tagValue) ? tagValue : [tagValue];
const highlightOrNot = tag in tagsToAdd ? chalk.bold : identity;
valueArray.forEach(value => {
memo.push(
highlightOrNot(
`@${escape(escape(tag))}${
value === true ? "" : " " + escape(escape(value))
}`
)
);
});
return memo;
}, [])
.join("\\n");
const commentValue = `E'${tagsSql}${
description ? "\\n" + escape(description) : ""
}'`;
let sqlThing;
if (entity.kind === "class") {
const identifier = `"${entity.namespaceName}"."${entity.name}"`;
if (entity.classKind === "r") {
sqlThing = `TABLE ${identifier}`;
} else if (entity.classKind === "v") {
sqlThing = `VIEW ${identifier}`;
} else if (entity.classKind === "m") {
sqlThing = `MATERIALIZED VIEW ${identifier}`;
} else {
sqlThing = `PLEASE_SEND_A_PULL_REQUEST_TO_FIX_THIS ${identifier}`;
}
} else if (entity.kind === "attribute") {
sqlThing = `COLUMN "${entity.class.namespaceName}"."${
entity.class.name
}"."${entity.name}"`;
} else if (entity.kind === "procedure") {
sqlThing = `FUNCTION "${entity.namespaceName}"."${
entity.name
}"(...arg types go here...)`;
} else if (entity.kind === "constraint") {
// TODO: TEST!
sqlThing = `CONSTRAINT "${entity.name}" ON "${
entity.class.namespaceName
}"."${entity.class.name}"`;
} else {
sqlThing = `UNKNOWN_ENTITY_PLEASE_SEND_A_PULL_REQUEST`;
}

return `COMMENT ON ${sqlThing} IS ${commentValue};`;
},
});
});

Expand Down Expand Up @@ -384,6 +519,25 @@ export default (function PgBasicsPlugin(
.join("-and-")}`
);
},
singleRelationByKeysBackwards(
detailedKeys: Keys,
table: PgClass,
_foreignTable: PgClass,
constraint: PgConstraint
) {
if (constraint.tags.foreignSingleFieldName) {
return constraint.tags.foreignSingleFieldName;
}
if (constraint.tags.foreignFieldName) {
return constraint.tags.foreignFieldName;
}
return this.singleRelationByKeys(
detailedKeys,
table,
_foreignTable,
constraint
);
},
manyRelationByKeys(
detailedKeys: Keys,
table: PgClass,
Expand All @@ -405,6 +559,9 @@ export default (function PgBasicsPlugin(
_foreignTable: PgClass,
constraint: PgConstraint
) {
if (constraint.tags.foreignSimpleFieldName) {
return constraint.tags.foreignSimpleFieldName;
}
if (constraint.tags.foreignFieldName) {
return constraint.tags.foreignFieldName;
}
Expand Down
Loading

0 comments on commit a6b4dd9

Please sign in to comment.