Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MSSQL - NATIVE_DATABASE_TYPES does not exist #508

Closed
stonehz opened this issue Dec 2, 2013 · 5 comments
Closed

MSSQL - NATIVE_DATABASE_TYPES does not exist #508

stonehz opened this issue Dec 2, 2013 · 5 comments

Comments

@stonehz
Copy link

stonehz commented Dec 2, 2013

In my attempt to override certain default data types
I discovered that NATIVE_DATABASE_TYPES does not exist for MSSQL.
Checking here:
https://github.com/jruby/activerecord-jdbc-adapter/blob/master/lib/arjdbc/mssql/adapter.rb

All other DBs seem to have one (mysql, postgres,oracle,sqllite3,etc)

Is there another way to override data_types?

Thank you in advance!

JDBC version 1.3.3
JRuby 1.7.5
Rails 4.00
@kares
Copy link
Member

kares commented Dec 2, 2013

Hey! please do not ask questions here - this is not a support forum ... have you tried looking through code :) ?!

ActiveRecord::ConnectionAdapters::MSSQLAdapter.class_eval do
  def modify_types(types)
    super
    # do your own sh*t
  end
end

might work with adapter: mssql

@kares kares closed this as completed Dec 2, 2013
@stonehz
Copy link
Author

stonehz commented Dec 2, 2013

Thanks 👍

I looked through the code and wondering whether NATIVE_DATABASE_TYPES was left out intentionally.
Gems like https://github.com/FineLinePrototyping/activerecord-native_db_types_override
can't be used.

thanks for your help.. I added an initializer file with the content below:

  ActiveRecord::ConnectionAdapters::MSSQLAdapter.class_eval do
    def modify_types(types)
      super
      types[:primary_key] = "bigint NOT NULL IDENTITY(1, 1) PRIMARY KEY"
      types
    end
  end

@kares
Copy link
Member

kares commented Dec 2, 2013

I see, thanks ... so gems actually assume NATIVE_DATABASE_TYPES is public API ;( in that case it's a bug.

@garysweaver
Copy link
Contributor

@kares Ok, so I see what you mean now about modify_types(types).

This is a bit of a problem I guess, because some adapters like the db2 adapter define NATIVE_DATABASE_TYPES but then override those types in modify_types, which would seem to make its native_database_types lying, because it really is not giving a full list of the abstract types as keys. Same for sybase, informix, mimer, mssql. And then oracle ignores the native_database_types convention so if the method were overriden, that wouldn't help. Etc.

Each adapter's modify_types(types) could be moved to either NATIVE_DATABASE_TYPES (for common types) or native_database_types (for types that conditionally need to be merged into NATIVE_DATABASE_TYPES). Then, all modify_types(types) would do is merge native_database_types into its types arg. But, even if all that were to be done, the method name native_database_types would be wrong in all of these, because it would not be a full list, since some of the types are from the drivers.

Another idea would be to do the opposite. NATIVE_DATABASE_TYPES and native_database_types could be removed from the aforementioned adapters, because NATIVE_DATABASE_TYPES and native_database_types aren't providing what is expected/don't seem to follow POLA. I would go in and make these changes as a PR, but ripping out things that people mistakenly think is helping the adapters confirm to AR adapter conventions might piss people off.

So, I think I'll stay out of it. Like you and @stonehz said, the right way is for anything that wants to override a default type to have a modify_types(types) that calls super and then merges changes that the user wants to override. Agree?

@kares kares removed the mssql label Feb 10, 2014
@kares kares added this to the 1.4.0 milestone Aug 24, 2014
@kares kares removed this from the 1.4.0 milestone Sep 9, 2015
@kares
Copy link
Member

kares commented Sep 11, 2015

closing due 230f9c5

@kares kares closed this as completed Sep 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants