feathers-sequelize icon indicating copy to clipboard operation
feathers-sequelize copied to clipboard

Sorting should detect attributes in service model

Open miguelpeixe opened this issue 7 years ago • 2 comments

When sorting a sequelize query using query built attributes, we need to use sequelize.col('attribute_name') instead of a string, so sequelize won't prefix with the table name ("table_name"."attribute_name").

getOrder util method should look inside the model for the sorted attributes to choose between using string or sequelize.col method.

Steps to reproduce

Lets say you have users with a hasMany association to votes that you'd like to populate the count through a hook and let the client choose sorting parameters.

const association = {
  attributes: [
    'id',
    'name',
    [
      sequelize.fn('COUNT', sequelize.col('votes.id')),
      'count'
    ]
  ],
  include: [
    {
      model: sequelize.models.votes,
      attributes: [],
      duplicating: false
    }
  ],
  group: ['users.id']
}

Part of the generated query will contain the following selection: COUNT("votes"."id") AS count.

Client sorting by count would look like this:

service.find({
  query: {
    '$sort': {
      count: -1
    }
  }
});

The generated query will ORDER BY "users"."count" DESC which does not exist.

Expected behavior

Detect if sorting attributes are part of the model, using sequelize.col method when they are not.

Actual behavior

Every sort attribute is built to sequelize query as a string.

miguelpeixe avatar Oct 26 '17 15:10 miguelpeixe

Is there any update about this issue, or is there a workarround to solve this ? I am trying to order by a literal, and it's building the query as if the literal were a column of the table.

maiconmichelon avatar Jul 16 '20 15:07 maiconmichelon

I like this. It is definitely a limitation of the current implementation. Although you can handle this my manually setting the params.sequelize.orderBy, I do think feathers-sequelize should support this OOTB. We will need to test how the $nested.query.syntax$ is affected and work around that. That is currently supported, so we will need to be sure that the update does not break it.

DaddyWarbucks avatar Aug 24 '21 17:08 DaddyWarbucks