belongsToMany limit / offset broken with tedious

[schie]

The following bit of code added by pull request #55 does not work for Sequelize v3 BelongsToMany associations with tedious when including both limit and offset:

dataloader-sequelize/src/index.js

Line 136 in 13f322c

const limit = findOptions.offset && findOptions.offset > 0 ? [findOptions.limit, findOptions.offset] : findOptions.limit;

The query that is built with a limit of 10 and an offset of 2 will contain the following:

-- rest of qurey
 OFFSET 0 ROWS FETCH NEXT 10,2 ROWS ONLY

Instead of the expected:

 OFFSET 2 ROWS FETCH NEXT 10 ROWS ONLY

Any thoughts regarding resolution?

Thanks

[mickhansen]

The fix provided in #55 did seem a bit hacky, but groupedLimit doesn't have offset support.
For full dialect support we'll need support for groupedLimit.offset in Sequelize first it looks like.

/cc @nicholasio

[schie]

Update: This also does not work in sequelize v4 with tedious.

Looking into how grouped limit works now

[schie]

Update:
It looks like this was added to sequelize v5 with this PR sequelize/sequelize#9577

[nicholasio]

The fact the #55 worked was simply due to the fact the sequelize seems to join arrays with ',' which matches what the SQL dialect expects. So maybe we should avoid doing that for other dialects to avoid breaking queries since there's no support for that in v3 and v4.

[schie]

Update:
added a Pull Request to add functionality that is in sequelize v5 for sequelize v4 sequelize/sequelize#10149

@nicholasio, I agree; we should probably remove the changes.

@mickhansen, thoughts?

I can make the pull request to pull this out if needed.

[mickhansen]

Ideally we could check the sequelize version and dialect, if v3/4 and mssql, throw, if other dialect, rely on "bug". If v5, pass as option.

[mickhansen]

Or if @nicholasio is open to it, we revert #55 and cut a new major with just offset support for v5

[schie]

@mickhansen,

I think a reverting #55 makes sense.

My pull request, sequelize/sequelize#10149, to add offset to v4 was merged in, and it works out of the box with dataloader-sequelize@1.7.4.

Before any major version bumps, let me look into seeing if the change would work for v3 as well.

[nicholasio]

Reverting #55 makes sense to me since now we have official support for that!

[schie]

closing because of staleness

[schie]

@mickhansen, I thought this was resolved in the current version of this lib, but it is still broken. Do we have plans to revert this? I can make a PR if need be.

[mickhansen]

Hi @schie, sorry for not responding, been swamped. It's probably best to revert and just add offset to groupedLimit and let the user use a recent version of Sequelize. PR welcome.

[schie]

Can do.

Thanks!