PostgreSQL interval type bugs #7518

Closed
xb opened this Issue Sep 3, 2012 · 7 comments

Comments

Projects
None yet
4 participants

xb commented Sep 3, 2012

The PostgreSQL type "interval" can actually happen to have a precision. (See http://www.postgresql.org/docs/9.1/static/datatype-datetime.html )Thus "interval(6)" is a type as legal as "interval".

However, the current ActiveRecord implementation maps "interval" to :string (which is correct, as there is no Ruby data type which better matches the postgresql interval data type without data loss), but it maps "interval(6)" to :integer.

There are actually 3 bugs.

  1. The first bug is that ActiveRecord::ConnectionAdapters::PostgreSQLColumn#simplified_type does not understand "interval(6)", while it should.
  2. The second bug is that ActiveRecord::ConnectionAdapters::Column#simplified_type simply searches for the word "int" anywhere in the field type string (instead of checking whether field_type=="int").
  3. The third bug is that ActiveRecord::ConnectionAdapters::Column#simplified_type does not raise an Exception if an unknown field_type string is encountered, while it should, because developers and users would then clearly see where the limits of what the current implementation supports are crossed.

Update: changed title to not mess up with Github, old title was:

ActiveRecord::ConnectionAdapters::PostgreSQLColumn.new(nil,nil).send(:simplified_type,"interval(6)").should == ActiveRecord::ConnectionAdapters::PostgreSQLColumn.new(nil,nil).send(:simplified_type,"interval")

Would you mind changing the issue title to be a bit more descriptive please? Thanks.

Owner

senny commented Sep 5, 2012

I'll have a look at this one.

@senny senny added a commit to senny/rails that referenced this issue Sep 6, 2012

@senny senny postgres, map scaled intervals to string datatype (#7518) 319482d
Owner

senny commented Sep 6, 2012

I fixed the bug. Regarding point 2 and 3 you mentioned. Currently all columns have the same superclass, which has the described behavior. This is not really postgres related. In my opinion it could be beneficiary to have the whole #simplified_type method in the postgres adapter. This would allow us to map the datatyps more cleanly and we could raise errors when something is not supported.

@rafaelfranca Is there a specific reason why #simplified_type delegates to the superclass? I think share code in the mapping of datatypes over specific vendors is more a drawback than an advantage.

Owner

tenderlove commented Sep 6, 2012

@senny I don't think there is a particular reason. If it makes sense to push all the code down to the subclass, then we should do it. (I've had the same question myself, so I'm glad you brought it up)

Owner

senny commented Sep 6, 2012

@tenderlove I'll work out a PR with all the code pushed into the subclass and proper error handling. We can then discuss the advantages and disadvantages in that PR.

Closed by #7545, thanks!

Owner

tenderlove commented Sep 7, 2012

@senny sounds good. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment