Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: overwriting already existing query's context in childQueryOf #2409

Merged
merged 1 commit into from
May 19, 2023

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