Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

postgresql_adapter.rb:default_sequence_name is broken for non-public schemas #7516

Closed
keithgabryelski opened this Issue · 8 comments

4 participants

@keithgabryelski

The current code:

  # Returns the sequence name for a table's primary key or some other specified key.
  def default_sequence_name(table_name, pk = nil) #:nodoc:
    serial_sequence(table_name, pk || 'id').split('.').last
  rescue ActiveRecord::StatementInvalid
    "#{table_name}_#{pk || 'id'}_seq"
  end

rails 4 is slightly different, but still has the ".split('.').last" -- which is the bug.

This code seems to want to remove the schema name from the return value of "serial_sequence" -- a typical result from serial_sequence" might look like "public.foos_id_seq" and the ".split('.').last" would resolve to "foos_id_seq".

the code would then be used by something that fetched from that object -- which works for "public" (or anything that is in the schema_search path) but wil fails if you have a table/model for table "other.foos" (that is, something not in the schema_search path OR if there is a public.foos and a other.foos -- you'll aways fetch from "public.foos").

since there is no "next_sequence_value" function this code probably has never been exercised -- sequence_name doesn't seem to be used anywhere for postgres, by default -- so you'll need the following code to exercise the issue:

  • enable prefetching of the primary key
  • add a "next_sequence_value" which simply requests the nextval for the value returned by sequence_name
  • and a patch to fix "default_sequence_name"

    module ActiveRecord::ConnectionAdapters
      class PostgreSQLAdapter < AbstractAdapter
        def prefetch_primary_key?(table_name = nil)
          return true
        end
    
        def next_sequence_value(sequence_name)
          return execute("select nextval('#{sequence_name}')").field_values("nextval").first.to_i
        end
    
        def default_sequence_name(table_name, pk = nil) #:nodoc:
          serial_sequence(table_name, pk || 'id')
        rescue ActiveRecord::StatementInvalid
          "#{table_name}_#{pk || 'id'}_seq"
        end
    
      end
    end
    
@senny
Owner

I think this is related to #3524

@arunagw
Collaborator

@keithgabryelski can you please check and confirm is this issue still exists?

thanks

@arunagw
Collaborator

Closing this issue for now. Please feel free to reopen if this issue still exists.

@arunagw arunagw closed this
@keithgabryelski

sorry, i'm on vacation -- this is certain STILL a problem -- how should I reopen this?

@arunagw arunagw reopened this
@rafaelfranca
Owner

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.

@senny senny added the PostgreSQL label
@senny senny removed the stale label
@senny senny added this to the 4.2.0 milestone
@senny
Owner

@keithgabryelski removed the stale label. This bug is still present. Will work on it.

@senny senny closed this in d6c1205
@senny
Owner

@keithgabryelski I pushed a fix to master, can you verify if this solves your problem?

@keithgabryelski
@senny senny referenced this issue from a commit
@senny senny pg, `default_sequence_name` needs to return a string.
This is a reacon to d6c1205#commitcomment-7502487
This backwards incompatibility was introduced with d6c1205 to fix #7516.
However both `connection.default_sequence_name` and `model.sequence_name` are public API.
The PostgreSQL adapter should honor the interface and return strings.

/cc @matthewd @chancancode
3fe54b3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.