Steps to reproduce
Create a new project, register a service, create a global hook and override createQuery to add join columns.
export class ChatMessagesService extends KnexService<
ChatMessagesResult,
ChatMessagesData,
ChatMessagesParams
> {
createQuery(params: ChatMessagesParams): any {
const query = super.createQuery(params)
query
.join('users', 'chat_messages.userId', 'users.id')
.join('user_schools', 'users.id', 'user_schools.userId')
.select('user_schools.schoolId as schoolId')
return query
}
}implement a softDelete hook as follow:
export const softDeleteRead = async (context: HookContext) => {
const { method, params, path, type } = context
const { query = {} } = params
if (type !== 'before') {
return context
}
if (path === 'authentication') {
return context
}
if (method === 'remove') {
return context
}
context.params.query = {
...query,
deletedAt: null,
}
return context
}register the softDelete hook at the app level
// Register hooks that run on all service methods
app.hooks({
around: {
all: [logErrorHook],
},
before: {
all: [softDeleteRead], // <====== HERE
create: [createdAt, deletedAt],
update: [notAllowed],
patch: [updatedAt],
},
after: {},
error: {},
})
// Register application setup and teardown hooks here
app.hooks({
setup: [],
teardown: [],
})Expected behavior
when using find I am expecting to get the message results along with the schoolId populated from the join query
Actual behavior
an error is thrown as the column deletedAt is not prefixed with the table name and the column become ambiguous as the deletedAt column is present on all tables
System configuration
using 5.0.0-pre.29 as I was not able to easily upgrade to 5.0.x, but after reading the actual knex adapter, it seems that the issue is also present on latest 5.0.5
I have fixed the issue locally with a patch:
diff --git a/node_modules/@feathersjs/knex/lib/adapter.js b/node_modules/@feathersjs/knex/lib/adapter.js
index 4765ae0..4283485 100644
--- a/node_modules/@feathersjs/knex/lib/adapter.js
+++ b/node_modules/@feathersjs/knex/lib/adapter.js
@@ -58,13 +58,16 @@ class KnexAdapter extends adapter_commons_1.AdapterBase {
return schema ? Model.withSchema(schema).table(table) : Model(table);
}
knexify(knexQuery, query = {}, parentKey) {
+ const { table, id } = this;
const knexify = this.knexify.bind(this);
return Object.keys(query || {}).reduce((currentQuery, key) => {
const value = query[key];
if (commons_1._.isObject(value)) {
return knexify(currentQuery, value, key);
}
- const column = parentKey || key;
+ const columnRaw = parentKey || key
+ const column = columnRaw.includes('.') ? columnRaw : `${table}.${columnRaw}`
+
const method = METHODS[key];
if (method) {
if (key === '$or' || key === '$and') {
@@ -92,8 +95,9 @@ class KnexAdapter extends adapter_commons_1.AdapterBase {
const builder = this.db(params);
// $select uses a specific find syntax, so it has to come first.
if (filters.$select) {
+ const select = filters.$select.map((column) => (column.includes('.') ? column : `${table}.${column}`))
// always select the id field, but make sure we only select it once
- builder.select(...new Set([...filters.$select, `${table}.${id}`]));
+ builder.select(...new Set([...select, `${table}.${id}`]));
}
else {
builder.select(`${table}.*`);
diff --git a/node_modules/@feathersjs/knex/src/adapter.ts b/node_modules/@feathersjs/knex/src/adapter.ts
index a3f3495..cce6972 100644
--- a/node_modules/@feathersjs/knex/src/adapter.ts
+++ b/node_modules/@feathersjs/knex/src/adapter.ts
@@ -77,6 +77,7 @@ export class KnexAdapter<
}
Steps to reproduce
Create a new project, register a service, create a global hook and override createQuery to add join columns.
implement a softDelete hook as follow:
register the softDelete hook at the app level
Expected behavior
when using
findI am expecting to get the message results along with theschoolIdpopulated from the join queryActual behavior
an error is thrown as the column
deletedAtis not prefixed with the table name and the column become ambiguous as thedeletedAtcolumn is present on all tablesSystem configuration
using
5.0.0-pre.29as I was not able to easily upgrade to5.0.x, but after reading the actual knex adapter, it seems that the issue is also present on latest5.0.5I have fixed the issue locally with a patch: