fix: added missing 'ORDER BY' before 'OFFSET'

[achrinza]

Signed-off-by: Rifa Achrinza [email protected]

Related: loopbackio/loopback-next#4160

If the supportsOffsetFetch is enabled, the generated SQL query is invalid as it does not comply with the expected syntax of a SELECT - ORDER BY Clause.

This PR resolves this issue by prepending the missing ORDER BY to OFFSET.

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

• npm test passes on your machine
• New tests added or existing tests modified to cover all changes
• Code conforms with the style guide
• Commit messages are following our guidelines

[slnode]

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

[frbuceta]

@slnode test please

[frbuceta]

How do you see this?

[bajtos]

@achrinza thank you for the pull request. Is there any reasonably-easy way how to test this change?

Ideally, we should test in integration style, write a test that creates a datasource with supportsOffsetFetch option enabled and runs a query that will trigger the problematic code path you are fixing.

Having said that, I don't know what version of MSSQL we use on our CI server, therefore this may not work.

The second best option is to write a unit-test that will verify the SQL statement created by MsSQL.prototype.applyPagination. Ideally, there should be two tests - one for supportsOffsetFetch enabled, another for supportsOffsetFetch disabled.

@raymondfeng thoughts?

[achrinza]

@bajtos I think ideally we should create a unit test to ensure a properly-generated query then put an integration test to make sure it works with MSSQL.

I'm a bit busy this week, but I'll look into it when possible.

[achrinza]

Updated top comment to reflect that this PR does not resolve loopbackio/loopback-next#4160.

However, this PR still does resolve the malformed T-SQL query hence should be kept open.

See: loopbackio/loopback-next#4160 (comment)

[frbuceta]

How can I contribute to this PR and upload changes?

[achrinza]

Hi @frbuceta, apologies for not being able to continue work on this PR. AFAIK, there's only the integration tests that are missing.

There are docs on how to contribute and CONTRIBUTING.md which may be helpful.

For this, you can fork the main repo, apply the changes I've made, write the tests and then create a new Pull Request.

[achrinza]

Closing this in favour of #220.

[frbuceta]

Hi @frbuceta, apologies for not being able to continue work on this PR. AFAIK, there's only the integration tests that are missing.

There are docs on how to contribute and CONTRIBUTING.md which may be helpful.

For this, you can fork the main repo, apply the changes I've made, write the tests and then create a new Pull Request.

I will try to create the tests. I'm going to mention you as an author in my PR if you don't think it's wrong.