Find() generates an invalid query with pagination but no order.

[AbsurdusAdeptus]

Right now the find method will generate an invalid SQL statement when pagination defaults are set or provided by the query and an order\sort value is not (as is created by the feathersjs generators). The generated SQL has FETCH NEXT and OFFSET but no order by. Sequelize is supposed to handle this by defaulting to the Model's primary key field, but because feathers-sequelize always provides at least an empty array for the order filter in it's query, it fails. A simple early out here /src/utils.js#L36 to return null or undefined resolves the issue. Or it could be fixed here /src/index.js#L30 by checking for the empty array before adding it to the query. Any preferences?

[daffl]

That makes sense. Do you have simple query that breaks? I'm wondering why the tests don't seem to be covering that.

[AbsurdusAdeptus]

I completely forgot to say that this is a problem I'm seeing using the MSSQL dialect (via tedious). The tests run with SQLite it seems, so that might explain it. The simple options of:

 const options = {
    Model: message(app.get('sequelize')),
    paginate: {
      default: 5,
      max: 25
    }
  };

and then just doing any empty final all query like http://localhost:3030/messages will trigger this crash:

Executing (default): SELECT [id], [text], [createdAt], [updatedAt] FROM [messages] AS [messages] OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY;
error: Route: /messages - Invalid usage of the option NEXT in the FETCH statement.
info: SequelizeDatabaseError: Invalid usage of the option NEXT in the FETCH statement.

This is an excerpt from the source code of the FeathersJS chat application. Again, configured to use this service with the MSSQLm dialect.

I'm planning on creating an issue over there (on sequelize) to address why their code sees not to add an order by when an empty array is passed, but not the limit\offset code.

[daffl]

Ah, I don't have an easy way to test it with an MSSQL database 😞 I'm definitely not opposed to adding the fix you suggested but could that be more of a Sequelize bug?

[ekryski]

Yeah it sounds like more of a sequelize bug. Unfortunately, it's not that easy for us to test MSSQL in an automated fashion and we haven't made it a priority because in reality we are deferring to sequelize for anything DB specific.

[ekryski]

I'm going to close this but @AbsurdusAdeptus do you have link to the sequelize issue so that we can keep tabs on it? (Looks like they have a few over there)

[tjme]

I'm hitting this issue. A possible workaround (that's working for me, for now) is to add a before hook to each service, e.g. (add into src/services/message/index.js):

messageService.before({ find(hook) { if (!hook.params.query.$sort) { hook.params.query.$sort={id: 1} } } })

[zgudino]

Another possible workaround is to add a sequelize beforeFind hook to your model.

module.exports = function (sequelize) {
    const members = sequelize.define('members', {
        id: {
            type: Sequelize.INTEGER,
            field: 'idCliente'
        },
        govId: {
            type: Sequelize.INTEGER,
            alloNull: false,
            field: 'Cedula',
            primaryKey: true
        },
        firstName: {
            type: Sequelize.STRING,
            field: 'Nombre'
        },
        // ...
    );
    return members.beforeFind(model => model.order.push(['id', 'ASC']));
};

Not as clever as @tjme workaround but it works.

[daffl]

I added documentation for this to feathers-sequelize at https://github.com/feathersjs-ecosystem/feathers-sequelize#working-with-mssql