MSSql adapter could be refactored to allow easier type overrides #509

Closed
garysweaver opened this Issue Dec 2, 2013 · 4 comments

Projects

None yet

3 participants

@garysweaver
Contributor

Unlike the other adapters, the MS SQL adapter is complicated to override default SQL datatypes. It appears that part of the reason is that it might benefit from being split into a few different adapters to work with various versions of MS SQL Server. To @kares and others, I'm curious whether you think the adapter could be refactored to define a native_database_types method and the NATIVE_DATABASE_TYPES constant.

To see what I mean, look at how this method and constant are defined just about everywhere else:

$ grep -r "def native_database_types" *
lib/arjdbc/db2/adapter.rb:    def native_database_types
lib/arjdbc/derby/adapter.rb:    def native_database_types
lib/arjdbc/firebird/adapter.rb:    def native_database_types
lib/arjdbc/h2/adapter.rb:    def native_database_types
lib/arjdbc/hsqldb/adapter.rb:    def native_database_types
lib/arjdbc/jdbc/adapter.rb:      def native_database_types
lib/arjdbc/jdbc/connection.rb:      def native_database_types
lib/arjdbc/mysql/adapter.rb:    def native_database_types
lib/arjdbc/oracle/adapter.rb:    def native_database_types
lib/arjdbc/postgresql/adapter.rb:    def native_database_types
lib/arjdbc/sqlite3/adapter.rb:    def native_database_types
$ grep -r NATIVE_DATABASE_TYPES *
lib/arjdbc/db2/adapter.rb:    NATIVE_DATABASE_TYPES = {
lib/arjdbc/db2/adapter.rb:      types = super.merge(NATIVE_DATABASE_TYPES)
lib/arjdbc/derby/adapter.rb:    NATIVE_DATABASE_TYPES = {
lib/arjdbc/derby/adapter.rb:      NATIVE_DATABASE_TYPES
lib/arjdbc/derby/adapter.rb:      native_type = NATIVE_DATABASE_TYPES[t]
lib/arjdbc/firebird/adapter.rb:    NATIVE_DATABASE_TYPES = {
lib/arjdbc/firebird/adapter.rb:      NATIVE_DATABASE_TYPES
lib/arjdbc/h2/adapter.rb:    NATIVE_DATABASE_TYPES = {
lib/arjdbc/h2/adapter.rb:      NATIVE_DATABASE_TYPES
lib/arjdbc/hsqldb/adapter.rb:    NATIVE_DATABASE_TYPES = {
lib/arjdbc/hsqldb/adapter.rb:      NATIVE_DATABASE_TYPES
lib/arjdbc/mysql/adapter.rb:    NATIVE_DATABASE_TYPES = {
lib/arjdbc/mysql/adapter.rb:      NATIVE_DATABASE_TYPES
lib/arjdbc/oracle/adapter.rb:    NATIVE_DATABASE_TYPES = {
lib/arjdbc/oracle/adapter.rb:      super.merge(NATIVE_DATABASE_TYPES)
lib/arjdbc/oracle/adapter.rb:      NATIVE_DATABASE_TYPES.each do |key, value|
lib/arjdbc/postgresql/adapter.rb:    NATIVE_DATABASE_TYPES = {
lib/arjdbc/postgresql/adapter.rb:    NATIVE_DATABASE_TYPES.update({
lib/arjdbc/postgresql/adapter.rb:      NATIVE_DATABASE_TYPES
lib/arjdbc/sqlite3/adapter.rb:    NATIVE_DATABASE_TYPES = {
lib/arjdbc/sqlite3/adapter.rb:      types = NATIVE_DATABASE_TYPES.dup

In the activerecord-native_db_types_override gem, we rely on that standard method and constant to allow very slightly more straightforward/clean overrides- if nothing else, it is an example of how those that need to and are willing to take the risks involved could override the defaults. Exposing the ability to override in this fashion might help others.

Thanks in advance for any feedback!

@kares
Member
kares commented Dec 2, 2013

Yes could be - just include NATIVE_TYPES constant for the latest SQLServer - please do that if you care that much ...
esp. now that you reported this while having another one #508

@garysweaver
Contributor

@kares Hahaha. That was not me and sorry I missed that. I told them someone should do that so they created a ticket like me.

It is more than the constant. The native_database_types method should be used and the constant just used for defaults. I think it might make sense for either the adapter to be split into multiple adapters for each version that requires a different datatype for certain things, or define version specific datatypes in the constants, per my really brief look at the code. Which do you recommend? I assume the latter. I've not yet thoroughly read the code and don't have SQL Server to test with, so I'm guessing it would affect you more than me, which is mostly the reason I mentioned it.

@stonehz
stonehz commented Dec 3, 2013

👍

@kares
Member
kares commented Dec 3, 2013

Honestly, I do not have that much time to care about MS-SQL esp. for "minor" stuff like this ... (there are more serious SQLServer issues open here). There's plenty of gems that make APIs out of AR internals. That might be this case as well. That being said if you guys care so much, I'd be glad to accept a PR - no need for spliting the adapter ... it's probably fine if it works with newer DB versions and stays as is for 2k. Also I'm not sure about native_database_types if those happen as in AR for us ... we get "default" types from the driver that is why modify_types is used - there's already something "auto-magically" setup (this might be an issue since we support 2 drivers - thus all types would need to be listed and tested if they're recognized the same with both JDBC drivers).

I'm closing this as a duplicate for #508

@kares kares closed this Dec 3, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment