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

Ordering issue with has_many and "nulls last" #11571

Closed
jgadbois opened this issue Jul 23, 2013 · 6 comments
Closed

Ordering issue with has_many and "nulls last" #11571

jgadbois opened this issue Jul 23, 2013 · 6 comments

Comments

@jgadbois
Copy link

Using Rails 4 and Postgres. Invalid sql is generated with a custom has_many order clause when accessing .last

#models/profile.rb
class Profile < ActiveRecord::Base
  has_many :offers, -> { where(:deleted_at=>nil).order('featured ASC nulls last, id DESC') }
end

# models/offer.rb
class Offer < ActiveRecord::Base
  belongs_to :profile
end

## rails c

# works
>> AuthorProfile.first.offers.first 

# SQL error
>> AuthorProfile.first.offers.last

ActiveRecord::StatementInvalid: PG::Error: ERROR:  syntax error at or near "DESC"
LINE 1: ...ted_at" IS NULL  ORDER BY featured ASC nulls last DESC, id A...
                                                             ^
: SELECT  "offers".* FROM "offers"  WHERE "offers"."profile_id" = $1 AND "offers"."deleted_at" IS NULL  ORDER BY featured ASC nulls last DESC, id ASC LIMIT 1
@pftg
Copy link
Contributor

pftg commented Jul 23, 2013

Confirm for 4.0.0, 3.2.13 and for master:

unless File.exists?('Gemfile')
  File.write('Gemfile', <<-GEMFILE)
    source 'https://rubygems.org'
    #gem 'rails', path: '../rails'
    #gem 'rails', '= 3.2.13'
    gem 'rails', '= 4.0.0'
    gem 'sqlite3'
  GEMFILE

  system 'bundle'
end

require 'bundler'
Bundler.setup(:default)

require 'active_record'
require 'minitest/autorun'
require 'logger'

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')
ActiveRecord::Base.logger = Logger.new(STDOUT)


ActiveRecord::Schema.define do
  create_table :items do |t|
    t.string 'type'
  end
end

class Item < ActiveRecord::Base
  default_scope { order 'type ASC nulls last' }
end

class BugTest < MiniTest::Unit::TestCase
  def test_association_stuff
    assert_includes Item.last.to_sql, 'ORDER BY type DESC nulls last LIMIT 1'
  end
end

Investigating in resolving the issue.

pftg added a commit to jetthoughts/rails that referenced this issue Jul 29, 2013
pftg added a commit to jetthoughts/rails that referenced this issue Aug 1, 2013
Order part of query with custom statements like `nulls first` or
`nulls last` at the end which arrange `null` values to top or bottom,
is broken for reverse order sql generation by adding `DESC` at the end,
because of skipping `asc|desc` before `nulls first|last` expression.

Refactored `reverse_sql_order` to take in account,
that there is `nulls` statement after order direction
which are supported by ANSI from 2008.

Closes rails#11571
pftg added a commit to jetthoughts/rails that referenced this issue Aug 13, 2013
Order part of query with custom statements like `nulls first` or
`nulls last` at the end which arrange `null` values to top or bottom,
is broken for reverse order sql generation by adding `DESC` at the end,
because of skipping `asc|desc` before `nulls first|last` expression.

Refactored `reverse_sql_order` to take in account,
that there is `nulls` statement after order direction
which are supported by ANSI from 2008.

Closes rails#11571
@ghiculescu
Copy link
Member

Hi, any updates on this one?

@pftg
Copy link
Contributor

pftg commented Nov 6, 2013

There is PR for this issue: #7423

@jgadbois jgadbois added the stale label Apr 23, 2014
@rafaelfranca
Copy link
Member

This issue has been automatically marked as stale because it has not been commented on for at least
three months.

The resources of the Rails team are limited, and so we are asking for your help.

If you can still reproduce this error on the 4-1-stable, 4-0-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

@laurocaetano
Copy link
Contributor

Just removed the stale tag since it still an issue and it has a PR: #7423

@sgrif
Copy link
Contributor

sgrif commented Oct 29, 2015

We'v rejected the PRs for this issue, so I'm closing this as wontfix.

@sgrif sgrif closed this as completed Oct 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants