Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add NULL FIRST and NULL LAST for most databases #1463

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tttffff
Copy link

@tttffff tttffff commented Nov 17, 2023

This PR changes how NULL FIRST and NULL LAST is implemented for Postgresql and also allows it to be used by other DB's. Instead of using SQL fragments, it uses Arel methods.

This is related to #1373, but does not actually fix the issue for MySQL.
I may look into a PR for that at some point in the future. For example, it could be fixed with a conditional on ::ActiveRecord::Base.connection.adapter_name and SQL fragments for MySQL.

If MySQL users add this configuration, they will get ActiveRecord::StatementInvalid errors in some circumstances, in some other other circumstances, they may get unexpected ordering. Becuase of this, I have stated on the documentation that it does not work for MySQL. This has the advantage of just working if Arel is updated to support for MySQL. See rails/rails#50078 for a longer description of how Arel handles null_first/null_last for MySQL at the moment.

No longer just Postgresql
Does not work with MySQL
@tttffff tttffff force-pushed the fields_sort_option_for_other_dbs branch from 47f0327 to 1efc802 Compare November 17, 2023 00:08
@@ -614,19 +614,19 @@ def remove_quotes_and_backticks(str)
expect(@s.result.first.id).to eq 1
end

it "PG's sort option", if: ::ActiveRecord::Base.connection.adapter_name == "PostgreSQL" do
it "fields sort option", if: ::ActiveRecord::Base.connection.adapter_name != "Mysql2" do
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If Arel does implement working nulls_first() and nulls_last() for MySQL, or if Ransack is ever updated to implement it, then we will need specific MySQL tests as the generated SQL will be different.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant