options :azure, :contained, and :use_utf16 need to be coerced to boolean from string/nil

[PaulCharlton]

TinyTDS v2.1.2 and current HEAD

Current State:

At the moment, the only "false" values for these configs are "nil" and the boolean value false, whereas all non-nil string values, including the empty string and the string "false" are interpreted as the boolean value true.

This presents several issues:

1. Rails developers left casting of config values to non-string types to each database adapter or their parent classes. see reference
   1.1. the config values :azure, :contained, and :use_utf16 are not known to the scaffolding currently provided in rails, and are interpreted as string type.
2. Rails discards all non-string config values parsed from ENV 'DATABASE_URL', when merging into existing config, which means that there is no means for the environment variable to remove a config set in database.yml
   2.1 setting any of the aforementioned options in database.yml to any value other than nil can not be converted to a boolean false value by anything set in 'DATABASE_URL', even if 'DATABASE_URL' contains "?azure=false", it will be interpreted by TinyTDS as boolean true
3. meeting expectation of Rails developers.
   3.1 Rails ActiveRecord/ActiveModel has type conversions which already set expectations on how strings are converted to a target of boolean, ideally TinyTDS would use the same conversion.

Rails Reference and also Discussion here

Desired State:

1. all settings in DATABASE_URL, such as "?azure=false" should return "expected" config result of boolean false, consistent with ActiveRecord conversion from database native string to Ruby boolean.
2. [this one is Rails, not TinyTDS]. a setting in DATABASE_URL such as "?azure=" should delete any existing config key named :azure [it used to work this way, need to research if it is a bug or a deliberate breaking change from past behavior]
3. unit tests for the above config settings which inject strings with falsy values instead of just booleans.

[aharpervc]

Does :use_utf16 already work like you want? https://github.com/rails-sqlserver/tiny_tds/blob/master/lib/tiny_tds/client.rb#L52

all settings in DATABASE_URL, such as "?azure=false" should return "expected" config result of boolean false, consistent with ActiveRecord conversion from database native string to Ruby boolean.

Where is the code responsible for this?

[PaulCharlton]

I already started testing a WIP PR for this ... new unit tests first.
:use_utf16 is closer in behavior, but still not consistent with rails handling of underlying database fields, and the use_utf16 defaults to true when the config is nil

I was thinking code something like the following (half baked) psuedocode:

   FALSE_VALUES = [
        false, 0,
        "", :"",
        "0", :0,
        "f", :f,
        "F", :F,
        "false", :false,
        "FALSE", :FALSE,
        "off", :off,
        "OFF", :OFF,
    ].to_set.freeze

...
    def initialize(opts = {})
  ...
      opts[:azure] = to_boolean(:azure)
      opts[:contained] = to_boolean(:contained)
      opts[:use_utf16] = opts[:use_utf16].nil? || to_boolean(:use_utf16)
  ...
    end
...
    def to_boolean(opt)
      // something for handling nil
      return !FALSE_VALUES.include?(opts[:opt].to_s)
    end

the corresponding rails typecasting code is in activemodel/lib/active_model/type/boolean.rb @ github where "truthy" is semi-defined by the behavior therein.

[aharpervc]

I'm somewhat skeptical that that code should live here in the tiny_tds project. To a reasonable degree, this project should be web framework agnostic and follow normal gem paradigms. Treating "F" as false doesn't seem like a normal gem thing, it seems like some kind of Rails configuration idea.

Do other database gems do this kind of thing? Should that code live in the activerecord sql server adapter?

[PaulCharlton]

wrote up an issue for rails/rails to address the ability to unset or remove a config key via the contents of DATABASE_URL here -> rails/rails#38748

[PaulCharlton]

@aharpervc good point

see: rails/rails#9097 (comment) and rails/rails@e54acf1

which explicitly defer type-coercion into the adapter layer -- however, the config option needs to be a config option key that the adapter layer is aware of, and TinyTDS has not explicitly published these newer settings into the sqlserver adapter layer as yet. (actually, from reading the code, and later comments, the type-coercion is deferred to the point of use ... and in our current example, TinyTDS is the first point of use unless we want to train the SQLServer adapter how to recognize those new config keys

[aharpervc]

unless we want to train the SQLServer adapter how to recognize those new config keys

Yeah, I'm thinking that is possible the preferable way to do it. That seems like the piece of the stack that could reasonably be responsible for handling "rails" <-> "database driver" connectivity, which in this case means mapping various properties and their data types

[PaulCharlton]

@aharpervc after looking through the code in the activerecord-sqlserver-adapter, I concur that it should be responsible for the type-casting as it is already reading (aka first use) those values from the config hash and passing them to the initialization of the TinyTDS client. (with the exception of :use_rtf16, which it does not interpret, and I am seeing how a rails app can ever pass a meaningful value of :use_rtf16 into TinyTDS via activerecord-sqlserver-adapter

[PaulCharlton]

another thought -- usually when I have two (or more) pieces of code that need a shared understanding of data format, I write a schema and then either a code generator from the schema to each user of that data, or write a schema interpreter in each user, and share the schema amongst them. In this particular case, we have at least 3 pieces of code that need a shared understanding of the config attributes --- (1) TinyTDS, (2) AR-SqlServer-Adapter, (3) application code (database.yml), (4) Devops config (env URL).

[PaulCharlton]

see PR here -> rails-sqlserver/activerecord-sqlserver-adapter#741