search

rails-sqlserver / activerecord-sqlserver-adapter

SQL Server Adapter For Rails
MIT License
975 stars 563 forks source link

reconcile config options with TinyTDS and documentation; coerce config values to expected types #740

Open PaulCharlton opened 4 years ago

PaulCharlton commented 4 years ago

reconcile config options with TinyTDS and documentation; coerce config values to expected types

extended discussion at: https://github.com/rails-sqlserver/tiny_tds/issues/461 copied here =>

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 well 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 this Adapter]. 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.
PaulCharlton commented 4 years ago

PR is forthcoming

PaulCharlton commented 4 years ago

see also: https://github.com/rails-sqlserver/tiny_tds/issues/461#issuecomment-599680834