Skip to content

Conversation

@falkenhawk
Copy link
Contributor

@falkenhawk falkenhawk commented Apr 22, 2023

ported from ovos#12

This is a fix for an edge case, where a query context could be already set in a Model.query() call or in a custom QueryBuilder's constructor.

In our case we are defining columns with a custom @DefaultOrderBy(columns) decorator, they are stored to query's context after query builder is instantiated, and eventually are read in query.onBuild() hook to apply sorting for a query, if none other was provided to it in the meantime.

But when coupled with fetching graphs, where both parent and children models have @DefaultOrderBy configured, child model's query context (eager query to fetch models for a relation) was being completely overwritten with parent's query context, and that, in our case, resulted in using invalid columns to be passed to orderBy(), resulting in a sql error.


Note: this may look quite specific to our use case, but still, imo, query context passed from the parent should be already available at construction/instantiation of a child query, and not to be overwritten at later point.

For better picture, I attach the aforementioned DefaultOrderBy decorator/plugin:

import { Knex } from 'knex';
import { Model, QueryBuilder } from 'objection';

type OrderByDirection = 'asc' | 'desc';
type OrderByFunction<QB extends QueryBuilder<any>> = (q: QB) => QB | void;
type OrderByColumn<QB extends QueryBuilder<any> = QueryBuilder<any>> =
  | string
  | [string, OrderByDirection?]
  | OrderByFunction<QB>;

const DefaultOrderBy = (orderBy?: OrderByColumn | OrderByColumn[]) => {
  return <T extends Constructor<Model>>(ModelClass: T) =>
    class extends ModelClass {
      static query(trxOrKnex?: Knex) {
        // @ts-ignore
        return ensureDefaultOrderBy(super.query(trxOrKnex), orderBy);
      }
    };
};

export function ensureDefaultOrderBy<QB extends QueryBuilder<any>>(
  query: QB,
  orderBy?: OrderByColumn<QB> | OrderByColumn<QB>[],
  overwrite: Boolean = true
) {
  const hasDefaultOrderBy = !!query.context().defaultOrderBy;
  if (hasDefaultOrderBy && !overwrite) {
    return query;
  }

  let columns = Array.isArray(orderBy) ? orderBy : orderBy !== undefined ? [orderBy] : [];
  if (columns.length === 2 && typeof columns[0] === 'string' && (columns[1] === 'asc' || columns[1] === 'desc')) {
    // nest [column, direction] into [[column, direction]];
    // @ts-ignore
    columns = [columns];
  }
  if (!columns.length) {
    const modelClass = query.modelClass();
    // if no columns given, order by primary key columns by default
    columns = Array.isArray(modelClass.idColumn) ? modelClass.idColumn : [modelClass.idColumn];
  }

  query.context({
    defaultOrderBy: columns,
  });

  return query.onBuild(query => {
    // exclude non-select queries or queries with first() or orderBy() already defined
    // @ts-ignore No typings for has
    if (!query.isFind() || query.has(/orderBy/) || query.has('first')) {
      return;
    }

    const columns = query.context().defaultOrderBy;
    if (!columns || !columns.length) {
      return;
    }

    const tableRef = query.tableRefFor(query.modelClass());

    for (const col of columns) {
      if (Array.isArray(col)) {
        query.orderBy(`${tableRef}.${col[0]}`, col[1]);
      } else if (typeof col === 'function') {
        col(query);
      } else {
        query.orderBy(`${tableRef}.${col}`);
      }
    }
  });
}

export default DefaultOrderBy;

This is a fix for an edge case, where a query context could be already set in a `Model.query()` call or in a custom QueryBuilder's constructor.
In our case we are defining columns with a custom `@DefaultOrderBy(columns)` decorator, they are stored to query's context after query builder is instantiated, and eventually are read in `query.onBuild()` hook to apply sorting for a query, if none other was provided to it in the meantime.
But when coupled with fetching graphs, where both parent and children models have `@DefaultOrderBy` configured, child model's query context (eager query to fetch models for a relation) was being completely overwritten with parent's query context, and that, in our case, resulted in using invalid columns to be passed to `orderBy()`, resulting in a sql error.
@lehni
Copy link
Collaborator

lehni commented May 19, 2023

LGTM, thanks!

@lehni lehni merged commit 799fe91 into Vincit:master May 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants