Make reverse_sql_order work correctly if "nulls first/last" is in the middle of "order by".

[Euphoreka7]

Having order("CASE column1 WHEN 'data1' THEN 0 ELSE column2 END, column3.id NULLS FIRST, RANDOM()") and adding .last i get exception from postgres because of invalid query syntax:

ORDER BY CASE column1 WHEN 'data1' THEN 0 ELSE column2 END DESC, column3.id NULLS FIRST DESC, RANDOM()

after making this patch it works correctly:

ORDER BY CASE column1 WHEN 'data1' THEN 0 ELSE column2 END DESC, column3.id DESC NULLS FIRST, RANDOM() DESC

[kamipo]

Thanks for the PR. Can you add a test case about what this fixes?

[Euphoreka7]

I added some tests. now 'foo nulls first' can be reversed
I'm not really sure why current rule for nulls first/last implemented in this way:

      def does_not_support_reverse?(order)
        # Account for String subclasses like Arel::Nodes::SqlLiteral that
        # override methods like #count.
        order = String.new(order) unless order.instance_of?(String)

        # Uses SQL function with multiple arguments.
        (order.include?(",") && order.split(",").find { |section| section.count("(") != section.count(")") }) ||
          # Uses "nulls first" like construction.
          /nulls (first|last)\Z/i.match?(order)
      end

should 'nulls first' construction be irreversible only in case if order ends with this construction?
or is it a bug and should be:

      def does_not_support_reverse?(order)
        # Account for String subclasses like Arel::Nodes::SqlLiteral that
        # override methods like #count.
        order = String.new(order) unless order.instance_of?(String)

        # Uses SQL function with multiple arguments.
        (order.include?(",") && order.split(",").find { |section| section.count("(") != section.count(")") }) ||
          # Uses "nulls first" like construction.
          /nulls (first|last)\Z/i.match?(section)
      end

? (order replaced with section)

if it's a bug and rails should see the whole order irreversible no matter where 'null first' is - I happy to create a new pull request to fix this bug (and close this one).

if it's ok to have 'nulls first' as a non-last option - this PR should fix broken AR behavior

previously AR having foo NULLS FIRST, bar made foo NULLS FIRST DESC, bar DESC from it and raised pg exception because of wrong query syntax. this pr should fix it

[Euphoreka7]

I can also drop || /nulls (first|last)\Z/i.match?(order) as with those changes it will work keeping original first/last order

[kamipo]

does_not_support_reverse? wad added at #18928.

I suppose the reason that does not support reverse NULLS FIRST/LAST is that we could not decide the behavior user expected whether or not NULLS FIRST/LAST should be reversed.

(IMO at least unless NULLS FIRST/LAST is reversed, relation.order("id NULLS FIRST").reverse_order.to_a is not to be the same result with relation.order("id NULLS FIRST").to_a.reverse)

[Euphoreka7]

fair. I created a new PR to fix already existing regex bug.