Add an option to ignore queries with conditional expressions #286
Comments
|
@gajus hey, nice to see you here! 👋 What do you think about the following (this is what we did on a small-medium size codebase, where we also experienced many reported problems):
I think that adding an option to ignore such errors would have the downside of making it easier to make mistakes in future... 🤔 (not to mention moving further away from a zero-config codebase for SafeQL) |
That's not something we are planning to do, so it would be nice to have an escape hatch for those who are willing to accept the compromise (of skipping those queries). Using ESLint disable is an option, but it would mean polluting the codebase, which I am not fan of. We would probably just patch safeql to fit our use case. For context, I think that conversation is a bit simplistic in terms of how it evaluates the ability to express conditional fragments in raw SQL. Here is just one example of what such a query could look like: export const resolve = resolverBuilders.Query.adminUserSearch()
.authenticated()
.authorized('SEARCH_USERS')
.resolve(async (_root, args, context, info) => {
const { after, filter, first, search } = args;
const { loaders, pool, services } = context;
let googlePlaceId: null | number;
if (filter?.locationGooglePlaceId) {
googlePlaceId = await upsertGooglePlace(
services.googlePlace,
pool,
filter.locationGooglePlaceId,
);
}
const matchOnExactEmail = search && isEmailSyntacticallyValid(search);
return await loaders.UserSearchResults.load({
cursor: after,
info,
limit: first || 200,
orderBy: (columns) => {
const orderBy: Array<[SqlToken, 'ASC' | 'DESC']> = [];
if (search && !matchOnExactEmail) {
orderBy.push([
sql.unsafe`(${columns.name} <-> ${search}) - (${columns.name} ilike ${search} || '%')::integer`,
'ASC',
]);
}
if (googlePlaceId) {
orderBy.push([
sql.unsafe`
ST_Distance(
(
SELECT
ST_Centroid(ST_MakeLine(gp1.viewport_southwest, gp1.viewport_northeast))
FROM google_place gp1
WHERE
gp1.id = ${googlePlaceId}
),
(
SELECT
ST_Centroid(ST_MakeLine(gp1.viewport_southwest, gp1.viewport_northeast))
FROM google_place gp1
WHERE
gp1.id = ${columns.googlePlaceId}
)
)
`,
'ASC',
]);
}
orderBy.push([columns.id, 'ASC']);
return orderBy;
},
where: (columns) => {
const where = [];
if (search) {
if (matchOnExactEmail) {
where.push(sql.fragment`${columns.emailAddress} ilike ${search}`);
} else {
where.push(sql.fragment`(${columns.name} <-> ${search} < 0.9)`);
}
}
if (filter) {
const {
hasBeenFeaturedOnSocial,
hasRating,
isCommunitySlackMember,
isSocialFeatureEligible,
isTopContractor,
maxAutoRating,
maxLastRatedAt,
maxNumberOfPortfolioProjects,
maxNumberOfProductizedServices,
maxRating,
minAutoRating,
minLastRatedAt,
minNumberOfPortfolioProjects,
minNumberOfProductizedServices,
minRating,
profileUpdatedSinceBeingRated,
roleName,
status,
} = filter;
if (typeof hasRating === 'boolean') {
where.push(sql.fragment`${columns.hasRating} = ${hasRating}`);
}
if (typeof isCommunitySlackMember === 'boolean') {
where.push(
sql.unsafe`${columns.isCommunitySlackMember} = ${isCommunitySlackMember}`,
);
}
if (typeof isSocialFeatureEligible === 'boolean') {
where.push(
sql.unsafe`${columns.isSocialFeatureEligible} = ${isSocialFeatureEligible}`,
);
}
if (typeof hasBeenFeaturedOnSocial === 'boolean') {
where.push(
sql.unsafe`${columns.hasBeenFeaturedOnSocial} = ${hasBeenFeaturedOnSocial}`,
);
}
if (typeof isTopContractor === 'boolean') {
where.push(
sql.unsafe`${columns.isTopContractor} = ${isTopContractor}`,
);
}
if (typeof minNumberOfPortfolioProjects === 'number') {
where.push(
sql.unsafe`${columns.numberOfPortfolioProjects} >= ${minNumberOfPortfolioProjects}`,
);
}
if (typeof maxNumberOfPortfolioProjects === 'number') {
where.push(
sql.unsafe`${columns.numberOfPortfolioProjects} <= ${maxNumberOfPortfolioProjects}`,
);
}
if (typeof minNumberOfProductizedServices === 'number') {
where.push(
sql.unsafe`${columns.numberOfProductizedServices} >= ${minNumberOfProductizedServices}`,
);
}
if (typeof maxNumberOfProductizedServices === 'number') {
where.push(
sql.unsafe`${columns.numberOfProductizedServices} <= ${maxNumberOfProductizedServices}`,
);
}
if (typeof minAutoRating === 'number') {
where.push(sql.fragment`${columns.autoRating} >= ${minAutoRating}`);
}
if (typeof maxAutoRating === 'number') {
where.push(sql.fragment`${columns.autoRating} <= ${maxAutoRating}`);
}
if (typeof minRating === 'number') {
where.push(sql.fragment`${columns.manualRating} >= ${minRating}`);
}
if (typeof maxRating === 'number') {
where.push(sql.fragment`${columns.manualRating} <= ${maxRating}`);
}
if (typeof minLastRatedAt === 'string') {
where.push(
sql.fragment`${columns.lastRatedAt} >= ${minLastRatedAt}`,
);
}
if (typeof maxLastRatedAt === 'string') {
where.push(
sql.fragment`${columns.lastRatedAt} <= ${maxLastRatedAt}`,
);
}
if (profileUpdatedSinceBeingRated === true) {
where.push(
sql.unsafe`${columns.updatedAt} > ${columns.lastRatedAt}`,
);
}
if (status && status.length >= 1) {
const condition = [];
if (status.includes('APPROVED')) {
condition.push(
sql.unsafe`NOT('{"FLAGGED_USER","BLOCKED_USER"}'::text[] && ${columns.userGroupNids})`,
);
}
if (status.includes('FLAGGED')) {
condition.push(
sql.unsafe`'FLAGGED_USER' = ANY(${columns.userGroupNids})`,
);
}
if (status.includes('BLOCKED')) {
condition.push(
sql.unsafe`'BLOCKED_USER' = ANY(${columns.userGroupNids})`,
);
}
where.push(
condition.length
? sql.unsafe`(${sql.join(condition, sql.fragment` OR `)})`
: sql.fragment`true`,
);
}
if (roleName) {
where.push(sql.fragment`
exists(
SELECT
*
FROM professional_role_user_account prua1
INNER JOIN professional_role pr1 ON
prua1.professional_role_id = pr1.id
WHERE
prua1.user_account_id = ${columns.id}
AND pr1.name ILIKE (${roleName} || '%')
)
`);
}
if (googlePlaceId) {
where.push(sql.fragment`
ST_Intersects(
(
SELECT
ST_MakeEnvelope(
ST_X(gp1.viewport_southwest),
ST_Y(gp1.viewport_southwest),
ST_X(gp1.viewport_northeast),
ST_Y(gp1.viewport_northeast),
4326
)
FROM google_place gp1
WHERE
gp1.id = ${googlePlaceId}
),
(
SELECT
ST_MakeEnvelope(
ST_X(gp1.viewport_southwest),
ST_Y(gp1.viewport_southwest),
ST_X(gp1.viewport_northeast),
ST_Y(gp1.viewport_northeast),
4326
)
FROM google_place gp1
WHERE
gp1.id = ${columns.googlePlaceId}
)
)
`);
}
}
return where.length
? sql.unsafe`${sql.join(where, sql.fragment` AND `)}`
: sql.fragment`true`;
},
});
});
I get that. I have strong opinions about codebases too, so not surprised to face resistance. The sole purpose of this issue is to highlight the need of such option for real-world projects that go beyond simple queries. |
|
I understand that the above-mentioned issue here revolves around conditional syntax, but I feel like it's actually more of how SafeQL could be integrated with large codebase as seamless as possible. This is an important and valuable discussion. I’d like to address a few points about this specific use case: Using Implicitly allowing queries with conditional syntax to bypass lint checks could result in runtime errors and make it unclear whether a query is properly linted. I’m open to ideas on how to address this, perhaps with an equivalent of My current recommendation is to incrementally adopt SafeQL. For example, when I integrated SafeQL in a medium-sized codebase (~300 statements), this approach allowed me to keep the PRs small and manageable. However, I understand this approach may not suit everyone. In any case, I'm open for suggestions on how to ease the adoption of SafeQL. |
|
so this is how we had to patch safeql to make it work for Contra with Slonik. diff --git a/dist/index.mjs b/dist/index.mjs
index 148393dd953dc663cbe65163523671e43cfe3baf..e172d5143a38ad5cb5f314078ba3d5508cc3e00c 100644
--- a/dist/index.mjs
+++ b/dist/index.mjs
@@ -293,6 +293,22 @@ const TSUtils = {
}
};
+function mapSlonikArray(expression) {
+ // We are expecting the expression to be a call expression with either string literal or fragment argument.
+ if (expression.type !== 'CallExpression') {
+ throw new Error('Expected CallExpression');
+ }
+ if (expression.arguments[1]?.type === 'Literal') {
+ // sql.array([], 'int4') -> "int4"[]
+ return `"${expression.arguments[1]?.value}"[]`;
+ } else if (expression.arguments[1]?.type === 'TaggedTemplateExpression') {
+ // sql.array([], sql.fragment`int4[]`)) -> int4[]
+ return expression.arguments[1].quasi.quasis[0].value.raw;
+ } else {
+ throw new Error('Unexpected arguments of ArrayExpression')
+ }
+}
+
function mapTemplateLiteralToQueryText(quasi, parser, checker, options) {
let $idx = 0;
let $queryText = "";
@@ -310,7 +326,11 @@ function mapTemplateLiteralToQueryText(quasi, parser, checker, options) {
return E.left(InvalidQueryError.of(pgType.left, expression));
}
const pgTypeValue = pgType.right;
- $queryText += pgTypeValue === null ? `$${++$idx}` : `$${++$idx}::${pgTypeValue}`;
+ if (pgTypeValue === 'array') {
+ $queryText += `$${++$idx}::${mapSlonikArray(expression)}`;
+ } else {
+ $queryText += pgTypeValue === null ? `$${++$idx}` : `$${++$idx}::${pgTypeValue}`;
+ }
}
return E.right($queryText);
}
@@ -677,6 +697,64 @@ function reportCheck(params) {
}),
E.fold(
(error) => {
+ // Example:
+ // ${emailIsPublic ? null : emailDomain},
+ if (error.toString().includes('Unsupported conditional expression flags')) {
+ return null;
+ }
+
+ // Example:
+ // ${sql.join(
+ // embeddedQueries.map((query) => sql.fragment``),
+ // sql.fragment`UNION`,
+ // )}
+ if (error.toString().includes('The type "ListSqlToken" has no corresponding PostgreSQL type.')) {
+ return null;
+ }
+
+ // Example:
+ // ${sql.fragment`ST_SETSRID(ST_POINT(${long},${lat}), 4326)`}
+ if (error.toString().includes('The type "SqlFragmentToken" has no corresponding PostgreSQL type.')) {
+ return null;
+ }
+
+ // Example
+ // const where: FragmentSqlToken = sql.fragment``;
+ // ${where}
+ if (error.toString().includes('The type "FragmentSqlToken" has no corresponding PostgreSQL type.')) {
+ return null;
+ }
+
+ // Example:
+ // ${sql.unnest([], ['int4', 'int4'])}
+ if (error.toString().includes('The type "UnnestSqlToken" has no corresponding PostgreSQL type.')) {
+ return null;
+ }
+
+ // Example:
+ // ${sql.identifier(['bar', 'baz'])}
+ if (error.toString().includes('The type "IdentifierSqlToken" has no corresponding PostgreSQL type.')) {
+ return null;
+ }
+
+ // Example:
+ // ${sql.type(FooType)`SELECT * FROM foo`}
+ if (error.toString().includes('The type "QuerySqlToken')) {
+ return null;
+ }
+
+ // Example:
+ // ${input.expiresAt ?? sql.fragment`now() + INTERVAL '30 days'`},
+ if (error.toString().includes('Unsupported union type')) {
+ return null;
+ }
+
+ // Example:
+ // ${whereExpressions.length ? sql.join(whereExpressions, sql.fragment` AND `) : sql.fragment`true`}
+ if (error.toString().includes('Union types must be of the same type')) {
+ return null;
+ }
+
return match(error).with({ _tag: "InvalidConfigError" }, (error2) => {
return reportInvalidConfig({ context, error: error2, tag });
}).with({ _tag: "DuplicateColumnsError" }, (error2) => {again, that's a large codebase, so lots of edge cases, but it gives a perspective of a what a complete support Slonik would require. |
I saw the thread where the decision was made to not support conditional queries (#100), and I understand the rationale.
However, I am attempting to adopt safeql in a huge codebase, and this is causing a flood of errors.
I think a reasonable solution would be to just add an option to ignore errors / queries that contain conditional expressions.
The text was updated successfully, but these errors were encountered: