Warn when applying ordering to a query with pre-existing order #524
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hey!
This PR attempts to mitigate issue #522
I was thinking about removing the pre-existing
order_by
from queries, but Ecto docs mention that reading the internal structure of the query is somewhat OK, but modifying is a no go.https://hexdocs.pm/ecto/Ecto.Query.html#__struct__/0
Flop is not exactly an adapter, but I think similar logic applies.
Using the internal API of Ecto seems to me as a good trade for avoiding the confusion of accidentally passing a query with pre-existing
order_by
which leads to a very weird behaviour and a lot of confusion.We could potentially be more strict and even raise when we have a query with pre-existing
order_by
, but that would be a breaking change.Let me know what you think, I am happy to make any corrections you find necessary.