jruby / activerecord-jdbc-adapter

JRuby's ActiveRecord adapter using JDBC.
BSD 2-Clause "Simplified" License
462 stars 386 forks source link

postgres adapter select_all returns nil column type for Rails 4.x #877

Open EdmundLeex opened 6 years ago

EdmundLeex commented 6 years ago

select_all method seems to always return empty column types.

rails 4.2.4 and activerecord-jdbcpostgresql-adapter 1.3.24

Consider this:

ActiveRecord::Base.connection.select_all("SELECT pg_is_in_recovery()")

# => #<ActiveRecord::Result:0x2f0166d7 @column_types={}, @columns=["pg_is_in_recovery"], @hash_rows=nil, @rows=[["f"]]>

And compare with c ruby pg gem.

ActiveRecord::Base.connection.select_all("SELECT pg_is_in_recovery()")
# => #<ActiveRecord::Result:0x00007fe65a63efc8 @column_types={"pg_is_in_recovery"=>#<ActiveModel::Type::Boolean:0x00007fe658efe110 @limit=nil, @precision=nil, @scale=nil>}, @columns=["pg_is_in_recovery"], @hash_rows=nil, @rows=[[false]]>
EdmundLeex commented 6 years ago

I am no expert of java, but this seems to be the culprit? https://github.com/jruby/activerecord-jdbc-adapter/blob/4cf36806d12d4b82330494b4f67a8909eabd0d0f/src/java/arjdbc/jdbc/JdbcResult.java#L86

rdubya commented 6 years ago

Hi @EdmundLeex, I actually just put in the change you mentioned to fix this issue. Are you running on the latest master branch?

rdubya commented 6 years ago

Realized I should have been clearer with my wording, I just put in the change that you linked to today. It is part of a commit that is supposed to fix the issue you reported. So the code has only been up on GitHub for a few hours. What version of the gem are you using and what version of Rails?

EdmundLeex commented 6 years ago

@rdubya sweet. can you point me to the change please? we are on rails 4.2.4 and activerecord-jdbcpostgresql-adapter 1.3.24

rdubya commented 6 years ago

Unfortunately, these fixes only apply to versions 50 and 51 which are only Rails 5.0 and Rails 5.1 compatible (respectively). I'm not sure how much work it would be to make it work for the 1.3.x version.

EdmundLeex commented 6 years ago

oh no.. Mind pointing me to the PR/git sha? I'd like to see if I can help with that.

rdubya commented 6 years ago

Sure, here's the PR: https://github.com/jruby/activerecord-jdbc-adapter/pull/875

I was also attempting to standardize the result object that is returned so other adapters can reuse it, so there's more in there than just the fix for the types. It should give you an idea of what needs to happen though.

EdmundLeex commented 6 years ago

Thank you. I will look into this. Please keep me posted too 😆

kares commented 6 years ago

to sum up here, this would need some clever backporting (considering current master does work right). it might be possible to do so and resolve the issue for 1.3 but we would need someone to look into it ...

875 would not pick easily but the overall work (on top of the PR) might not be that 'bad' to port over.