Order with Nulls Last

[corybuecker]

I posted a note in the AA Google Group to determine if this problem is actually a lack of understanding on my part. There was no answer there, and before I submit a pull request I want to make sure that I'm not missing something obvious.

When using PostgreSQL and AA, there is an issue with sorting a column that contains null values. PostgreSQL places null values before non-null values when descending order is specified. This is opposite of MySQL's behavior.

I've made the follow change to the sorting module, but I want to get some feedback before I submit it. Please let me know if there is a better way to approach this issue.

def sort_order(chain)
  params[:order] ||= active_admin_config.sort_order
  if params[:order] && params[:order] =~ /^([\w\_\.]+)_(desc|asc)$/
    column = $1
    order  = $2

    # if the database backend is PostgreSQL and the sort order is descending,
    # put null values after non-null values to emulate MySQL behavior
    if order == "desc" && ActiveRecord::Base.connection.instance_of?(ActiveRecord::ConnectionAdapters::PostgreSQLAdapter)
      order = "desc nulls last"
    end

    table  = active_admin_config.resource_table_name
    table_column = (column =~ /\./) ? column :
      "#{table}.#{active_admin_config.resource_quoted_column_name(column)}"

    chain.reorder("#{table_column} #{order}")
  else
    chain # just return the chain
  end
end

[seanlinsley]

That sounds fine to me, but what about ascending order?

It seems like we should definitely have tests for this... I wonder how you test based on the DB connection.

Also @thegreyjoy, I should note that it's perfectly acceptable to open a pull request in this stage, to get feedback. Also, Github Issues is really meant for bug reports :P

If you want to be fancy, you can create a pull request out of this existing Issue without creating a separate PR number.

[corybuecker]

Thanks for the link, good to know you can do that. I'm closing this and discussing it over in the PR.

[mattfordham]

Was this ever pulled in? I am having the same issue and the latest doesn't seem to fix it.

[corybuecker]

No, the solution I proposed wasn't ideal. You can get around the issue by overriding the method active_admin_collection in the controller that needs it. You could also probably put the override into an initializer.

def active_admin_collection
  params[:order] ||= active_admin_config.sort_order
  if params[:order] && params[:order] =~ /^([\w\_\.]+)_(desc|asc)$/
    column = $1
    order  = $2
    table  = active_admin_config.resource_table_name

    table_column = (column =~ /\./) ? column :
      "#{table}.#{active_admin_config.resource_quoted_column_name(column)}"

    super.reorder("#{table_column} #{order} nulls last")
  else
    super
  end
end

[epitron]

I just ran across this issue today. @corybuecker's patch is out of date, but I made a new one. I'd be interested if this was merged into the codebase!

[seanlinsley]

Is there a reason why anyone would desire the default behavior that Postgres provides? Should we make the behavior configurable in Active Admin?

ActiveAdmin::OrderClause really needs to be abstracted out into the orm folder so that we can have different logic for different databases. Though that might not happen as part of this ticket.

[Fivell]

ransack issue activerecord-hackery/ransack#443

[epitron]

I've been using a forked version with that 3 line patch for over a year and it's been working fine and dandy.

[Fivell]

test and feedback are appriciated