brendon / acts_as_list

An ActiveRecord plugin for managing lists.
http://brendon.github.io/acts_as_list/
MIT License
2.04k stars 355 forks source link

Getting invalid input syntax for uuid #253

Closed musaffa closed 7 years ago

musaffa commented 7 years ago

After upgrading from 0.8.2 to 0.9.1, I'm getting this error:

ActiveRecord::StatementInvalid (PG::InvalidTextRepresentation: ERROR:  invalid input syntax for uuid: "'2cf8a976-8fea-4157-8233-206bdcf20b28'"

0.8.2 is generating this SQL which is correct:

UPDATE "footnotes" SET "updated_at" = '2017-02-01 01:45:14.438527', "position" = ("footnotes"."position" + 1) WHERE "footnotes"."primary_legislation_id" = $1 AND ("footnotes"."position" >= 1) AND ("footnotes"."position" < 2 AND "footnotes".id != '551d4359-76c6-41b7-b779-6490a7d189fa')  [["primary_legislation_id", "256a693f-4b3f-4b81-90fc-6f17379f3ca7"]]

But 0.9.1 is generating the following SQL statement:

UPDATE "footnotes" SET "position" = ("footnotes"."position" + 1), "updated_at" = '2017-02-01 01:33:33.649759' WHERE "footnotes"."primary_legislation_id" = $1 AND ("footnotes".id != '''2cf8a976-8fea-4157-8233-206bdcf20b28''') AND ("footnotes"."position" >= 1) AND ("footnotes"."position" < 2)  [["primary_legislation_id", "256a693f-4b3f-4b81-90fc-6f17379f3ca7"]]

The problem is at "footnotes".id != uuid.

brendon commented 7 years ago

A stack trace will be helpful here. We need to figure out what particular method is generating this query. Either we're not quoting things properly on our end, or your uuid gem or method is returning a troublesome string.

We changed the methods used to generate these queries in the latest release to be more hash/array based instead of string based.

eg:

where("#{quoted_table_name}.#{self.class.primary_key} != ?", self.send(self.class.primary_key))

and

conditions = except ? "#{quoted_table_name}.#{self.class.primary_key} != #{self.class.connection.quote(except.id)}" : {}

It's likely in the first example where we're relying on the database layer to quote the id or not.

musaffa commented 7 years ago

@brendon I don't use any uuid gem. Here's the migration file of footnote:

# migrations/create_footnotes.rb
class CreateFootnotes < ActiveRecord::Migration
  def change
    create_table :footnotes, id: :uuid, default: 'gen_random_uuid()' do |t|
      t.string :body, null: false
      t.integer :position, null: false
      t.references :primary_legislation, type: :uuid, null: false
      t.references :footnotable, polymorphic: true, type: :uuid, null: false
      t.timestamps null: false
    end

    add_foreign_key :footnotes, :primary_legislations, on_delete: :cascade

    add_index :footnotes, [:primary_legislation_id, :position]
    add_index :footnotes, [:footnotable_type, :footnotable_id]
  end
end

Here's the model file:

# models/footnote.rb
class Footnote < ApplicationRecord
  belongs_to :primary_legislation
  belongs_to :footnotable, polymorphic: true

  acts_as_list scope: :primary_legislation
end

This is the stacktrace of footnote.move_higher call in a pry session:

[6] pry(main)> footnote = Footnote.find_by(id: "bbb9c6d3-d271-4d0d-bc36-7daf8053a7c0")                                                                                                                                Footnote Load (0.9ms)  SELECT  "footnotes".* FROM "footnotes" WHERE "footnotes"."id" = $1 LIMIT $2  [["id", "bbb9c6d3-d271-4d0d-bc36-7daf8053a7c0"], ["LIMIT", 1]]
=> #<Footnote:0x0055ebec318ed0
 id: "bbb9c6d3-d271-4d0d-bc36-7daf8053a7c0",
 body: "Substituted for the former second paragraph by the Constitution (Fifteenth Amendment) Act, 2011 (Act XIV of 2011), section 3.",
 position: 3,
 primary_legislation_id: "0a21b249-08cf-4b39-99a0-927db58880e2",
 footnotable_type: "PrimaryLegislation",
 footnotable_id: "0a21b249-08cf-4b39-99a0-927db58880e2",
 created_at: Fri, 03 Feb 2017 00:14:05 BDT +06:00,
 updated_at: Fri, 03 Feb 2017 04:03:06 BDT +06:00>
[7] pry(main)> footnote.move_higher
  Footnote Load (41.0ms)  SELECT  "footnotes".* FROM "footnotes" WHERE "footnotes"."primary_legislation_id" = $1 AND ("footnotes"."position" <= 3) AND ("footnotes".id != 'bbb9c6d3-d271-4d0d-bc36-7daf8053a7c0') ORDER BY "footnotes"."position" DESC LIMIT $2  [["primary_legislation_id", "0a21b249-08cf-4b39-99a0-927db58880e2"], ["LIMIT", 1]]
   (0.5ms)  BEGIN
  Footnote Load (0.9ms)  SELECT  "footnotes".* FROM "footnotes" WHERE "footnotes"."primary_legislation_id" = $1 AND ("footnotes"."position" <= 3) AND ("footnotes".id != 'bbb9c6d3-d271-4d0d-bc36-7daf8053a7c0') ORDER BY "footnotes"."position" DESC LIMIT $2  [["primary_legislation_id", "0a21b249-08cf-4b39-99a0-927db58880e2"], ["LIMIT", 1]]
  Footnote Load (0.9ms)  SELECT  "footnotes".* FROM "footnotes" WHERE "footnotes"."primary_legislation_id" = $1 AND ("footnotes"."position" <= 3) AND ("footnotes".id != 'bbb9c6d3-d271-4d0d-bc36-7daf8053a7c0') ORDER BY "footnotes"."position" DESC LIMIT $2  [["primary_legislation_id", "0a21b249-08cf-4b39-99a0-927db58880e2"], ["LIMIT", 1]]
  SQL (41.6ms)  UPDATE "footnotes" SET "position" = $1, "updated_at" = $2 WHERE "footnotes"."id" = $3  [["position", 3], ["updated_at", 2017-02-03 18:19:27 UTC], ["id", "e734c987-0061-4129-9bbc-c0ac695fdc83"]]
   (0.9ms)  SELECT COUNT(*) FROM "footnotes" WHERE "footnotes"."primary_legislation_id" = $1 AND ("footnotes"."position" = 3)  [["primary_legislation_id", "0a21b249-08cf-4b39-99a0-927db58880e2"]]
  SQL (1.4ms)  UPDATE "footnotes" SET "position" = ("footnotes"."position" - 1), "updated_at" = '2017-02-03 18:19:27.088884' WHERE "footnotes"."primary_legislation_id" = $1 AND ("footnotes".id != '''e734c987-0061-4129-9bbc-c0ac695fdc83''') AND ("footnotes"."position" > 2) AND ("footnotes"."position" <= 3)  [["primary_legislation_id", "0a21b249-08cf-4b39-99a0-927db58880e2"]]
   (0.4ms)  ROLLBACK
ActiveRecord::StatementInvalid: PG::InvalidTextRepresentation: ERROR:  invalid input syntax for uuid: "'e734c987-0061-4129-9bbc-c0ac695fdc83'"
LINE 1: ...imary_legislation_id" = $1 AND ("footnotes".id != '''e734c98...
                                                             ^
: UPDATE "footnotes" SET "position" = ("footnotes"."position" - 1), "updated_at" = '2017-02-03 18:19:27.088884' WHERE "footnotes"."primary_legislation_id" = $1 AND ("footnotes".id != '''e734c987-0061-4129-9bbc-c0ac695fdc83''') AND ("footnotes"."position" > 2) AND ("footnotes"."position" <= 3)
from /home/musaffa/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/activerecord-5.0.1/lib/active_record/connection_adapters/postgresql_adapter.rb:598:in `async_exec'
brendon commented 7 years ago

Ideally I need the full stack trace not the SQL log as I want to know which particular arel method is causing the quoting.

Also check this out: http://ruby-journal.com/how-to-override-default-primary-key-id-in-rails/ it hints at rails not thinking that the id column is a string column. That should be where you dig next I think :)

musaffa commented 7 years ago

Unfortunately the full stack trace didn't yield very useful result. With some debugging, I've figured out where the problem is. It is in this line like you suggested: :

if avoid_id
  scope = scope.where("#{quoted_table_name}.#{self.class.primary_key} != ?", self.class.connection.quote(avoid_id))
end

avoid_id returns a string to represent uuid which is obvious because Rails cannot interpret bdce4d08-fe0f-427b-bb2a-b7665a0dc4e8 as an integer. The article in the link, in fact, hints at Rails not thinking a string column an integer column, not the other way around.

I also tried this migration and it fails in the same way:

class CreateFootnotes < ActiveRecord::Migration
  def change
    create_table :footnotes, id: false do |t|
      t.uuid :id, primary_key: true, null: false, default: 'gen_random_uuid()'
      ....
    end
    ....
  end
end

A UUID cannot be represented as an integer. So the above active record query will always fail for UUID.

musaffa commented 7 years ago

In fact, any string based primary column will fail. A simple solution will be to manage string types (uuid, string) differently to the integer types.

brendon commented 7 years ago

Thanks @musaffa for the debugging :) I've committed a change to master that should work for you. Can you test your code against master and let me know how you get on?

We were quoting the value because we used to string interpolate the query. Now no longer :)

musaffa commented 7 years ago

Now it's working. Thanks.

Will it work for integer types? I thought quote has been introduced as a security feature against SQL injection attacks.

brendon commented 7 years ago

Glad to hear that.

We're using the array method to supply arguments ['some sql = ?', the_value] so Rails will quote / escape that automatically. I'll release a new version.