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

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

Open
PaulCharlton opened this issue Mar 17, 2020 · 2 comments

Comments

@PaulCharlton
Copy link

PaulCharlton commented Mar 17, 2020

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

extended discussion at: rails-sqlserver/tiny_tds#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
Copy link
Author

PR is forthcoming

PaulCharlton added a commit to PaulCharlton/activerecord-sqlserver-adapter that referenced this issue Mar 17, 2020
…ognized by TinyTDS -- Issue rails-sqlserver#740

Signed-off-by: mystic knight <techguru@byiq.com>
@PaulCharlton
Copy link
Author

see also: rails-sqlserver/tiny_tds#461 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants