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.

Sign up for GitHub

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

Allow symbol value via ENV #309

Closed
wants to merge 4 commits into from
Closed

Allow symbol value via ENV #309

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d6336e1
Allow symbol value via ENV
Oct 6, 2021
aa76ea9
Let strings starting with :: intact
Oct 7, 2021
01d4796
Replace .squish with .strip
Oct 7, 2021
efb69a6
Covert string to symbol
Oct 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,14 +453,16 @@ You can customize how environment variables are processed:
* `env_converter` (default: `:downcase`) - how to process variables names:
* `nil` - no change
* `:downcase` - convert to lower case
* `env_parse_values` (default: `true`) - try to parse values to a correct type (`Boolean`, `Integer`, `Float`, `String`)
* `env_parse_values` (default: `true`) - try to parse values to a correct type (`Boolean`, `Integer`, `Float`, `String` & `Symbol`)

For instance, given the following environment:

```bash
SETTINGS__SECTION__SERVER_SIZE=1
SETTINGS__SECTION__SERVER=google.com
SETTINGS__SECTION__SSL_ENABLED=false
SETTINGS__SECTION__SERVER_TYPE=:remote
SETTINGS__SECTION__SERVER_ACCESS=::ENABLED
```

And the following configuration:
Expand All @@ -481,6 +483,8 @@ The following settings will be available:
Settings.section.server_size # => 1
Settings.section.server # => 'google.com'
Settings.section.ssl_enabled # => false
Settings.section.server_type # => :remote
Settings.section.server_access # => '::ENABLED'
```

### Working with AWS Secrets Manager
Expand Down
20 changes: 15 additions & 5 deletions lib/config/sources/env_source.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,15 +59,25 @@ def load

# Try to convert string to a correct type
def __value(v)
case v
when 'false'
false
when 'true'
true
if %w(true false).include? v
eval(v)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the explicit cases for true and false. eval is a needlessly powerful and expensive tool to use here and reduces the readability of the code.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will revert this change

elsif v.strip =~ /^:[^:]/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new behavior is backwards-incompatible because it's possible that someone has an existing configuration where a value begins with : and their expectation is that they'll receive a String.

I think we have to let users opt into the new behavior explicitly. Let's add a new Config.env_parse_symbols whose default is false.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed! I will add a new configuration

convert_str_to_symbol(v)
else
Integer(v) rescue Float(v) rescue v
end
end

# Remove all special characters from a string before converting into a symbol
def convert_str_to_symbol(str)
str.
gsub(/[^a-z0-9\-_]+/i, "-").
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of the transformations are expected. The least surprising behavior from a user's perspective is that their input is interpreted exactly as they specified it. What's the rationale for modifying the input?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Transformations are needed for Symbol representation because a String may contain whitespaces, may start with or contain invalid characters, etc.
Example,

  1. "1"
  2. "$"
  3. " hello. this is a string ", etc

Invalid Symbol representation for above stings are:

  • :1
  • :$
  • :hello. this is a string

Whereas, .to_sym would prepend : to a passed string:

  • :"1"
  • :"$"
  • :" hello. this is a string "

After transformation Symbol representations would look like:

  • :"1"
  • :""
  • :hello_this_is_a_string

Can you please suggest the preferred Symbol representation for Config.env_parse_symbols?

Copy link
Member

@cjlarose cjlarose Oct 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see why these transformation were added then. The behavior, I think, is just unexpected from a user's point-of-view. Right now the behavior is roughly,

If Config.env_parse_symbols is truthy, then an ENV var value that starts with a : (but not with ::) with be returned as a Symbol. If the value starts with : (but not with ::) and contains characters that are incompatible with Ruby's definition of a Symbol literal, then the string is modified to conform to that definition.

This is unexpected for users because they likely have code that depends on the precise characters in the Symbol they specified in configuration. In other words, if they have code the depends on Settings.foo == :"hi, hello!", they would likely expect that ENV['Settings.foo'] = ":\"hi, hello!\"" would initialize Settings.foo to be exactly the Symbol specified. In other words, I'd expect this spec to pass:

        it 'should recognize quoted symbols as symbols' do
          ENV['Settings.new_var'] = ':"hi, hello!"'

          expect(config.new_var).to eq(:"hi, hello!")
        end

Instead, it fails because the returned Symbol is :hi_hello.

Instead of the current behavior, I'd expect the parsing logic to be

If Config.env_parse_symbols is truthy, and if the ENV var value is a valid Ruby Symbol literal, then return a Symbol. Otherwise, just leave it alone and return a String. In particular, if the ENV var value was literally ":hello. this is a string", then I'd expect the user to receive a String containing exactly those same characters.

In code, I'd expect this spec to pass:

        it 'should return invalid symbols as strings' do
          ENV['Settings.new_var'] = ':hello. this is a string'

          expect(config.new_var).to eq(':hello. this is a string')
        end

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

':"bam"' and ":bam" both looks valid to me, as latter was the format requested in this issue #286.

If Config.env_parse_symbols is set to true and the string starts with : then the code will parse the string as symbol.

str.tr(%q{:"'}, '').to_sym # Replace :, ", ' before converting into symbol

A simple logic that doesn't have to invalidate string based on its format.

Example:

':foo'.tr(%q{:"'}, '').to_sym             #   :foo
':"foo"'.tr(%q{:"'}, '').to_sym           #   :foo
':foo bar'.tr(%q{:"'}, '').to_sym         #   :"foo bar"  
':"foo bar"'.tr(%q{:"'}, '').to_sym       #   :"foo bar"
':foo_bar'.tr(%q{:"'}, '').to_sym         #   :foo_bar
':"foo_bar"'.tr(%q{:"'}, '').to_sym       #   :foo_bar

# String with Special characters and Numbers. Also, starts with and without `:`

":$foo".tr(%q{:"'}, '').to_sym            #   :$foo
':"$foo"'.tr(%q{:"'}, '').to_sym          #   :$foo
"4foo-bar".tr(%q{:"'}, '').to_sym         #   :"4foo-bar"
':"4foo-bar"'.tr(%q{:"'}, '').to_sym      #   :"4foo-bar"
"$4foo".tr(%q{:"'}, '').to_sym            #   :"$4foo"
':"$4foo"'.tr(%q{:"'}, '').to_sym         #   :"$4foo"
":*".tr(%q{:"'}, '').to_sym               #   :*
':"*"'.tr(%q{:"'}, '').to_sym             #   :*

From the above examples, we can notice that the string w/o : and/or "(quotes) can be represented as symbols - that should give users a flexibility to define strings as per their convience and set them to be parsed as symbols.

Please correct me if I am wrong with the above conclusion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok yes I understand the confusion now after re-reading the comments in #286

I'll re-iterate that I don't find any value in supporting the conversion of strings like :hello this is a string or :4foo-bar into symbols because they are not valid Ruby Symbol literals. Converting these things into Symbols as a "convenience," I think, would be more of a maintenance burden and source of confusion than is necessary.

For now, Config.env_parse_values only parses values that are valid in the Ruby grammar, and I think that's the least surprising convention.

gsub(/-{2,}/, "-").
gsub(/^-|-$/i, "").
tr("-", "_").
downcase.
to_sym
end
end
end
end
10 changes: 10 additions & 0 deletions spec/config_env_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,16 @@
expect(config.new_var.is_a? Float).to eq(true)
end

it 'should recognize strings starting with : as symbols' do
ENV['Settings.new_var'] = ':remote'
expect(config.new_var).to eq(:remote)
end

it 'should leave strings starting with :: intact' do
ENV['Settings.new_var'] = '::ENABLED'
expect(config.new_var).to eq('::ENABLED')
end

it 'should leave strings intact' do
ENV['Settings.new_var'] = 'foobar'

Expand Down