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

Allow for stringified options keys #7

Closed
wants to merge 4 commits into from
Closed

Allow for stringified options keys #7

wants to merge 4 commits into from

Conversation

julik
Copy link

@julik julik commented Jan 9, 2012

I've reworked the patch now to not convert the hashes every time (since this could introduce memory leaks into the system when too many mailcious string keys would be sent).

When config caches come from YAML files and the like it's sometimes difficult to trace whether a specific config key is a symbol. This leads to obscure error messages. Therefore it's sane to recursively symbolize the option hash keys and do a little allowance for the delivery option flag to be a string. This makes Pony friends with YAML configs without extraneous to_sym calls.
For example, hashes from YAML configs are usually string keyed, however we do not want to convert everything we receive into symbols since this can pollute the symbol table and lead to memory leaks. We therefore use a proxy class which acts sort of like the HashWithIndifferentAccess proxy, and will fetch is values from the hash with string keys when requested by symbol. The downside is that we need to wrap calls to options with this decorator every time, which we do using a delegate.
@julik
Copy link
Author

julik commented Jan 9, 2012

Please note that due to some bug with Github you actually need to pull in this commit
https://github.com/julik/pony/commit/4c462028f0988ca66002e164898052ea18c34f65
I'm really sorry for all the mess.

@benprew
Copy link
Owner

benprew commented Jan 9, 2012

Julik,

Thanks for working on this and I appreciate the spelling fixes too :).

I'd prefer one of two solutions:

  1. Use the Hashie gem instead of writing SymbolProxy
    (https://github.com/intridea/hashie)
  2. If you're using yaml or similar to write config files, you can
    prepend keys with ':' in the yaml file and ruby will interpret them as
    symbols (http://stackoverflow.com/questions/800122/best-way-to-convert-strings-to-symbols-in-hash)

Thanks

--Ben

On Mon, Jan 9, 2012 at 3:16 AM, Julik Tarkhanov
reply@reply.github.com
wrote:

I've reworked the patch now to not convert the hashes every time (since this could introduce memory leaks into the system when too many mailcious string keys would be sent).

You can merge this Pull Request by running:

 git pull https://github.com/julik/pony master

Or you can view, comment on it, or merge it online at:

 #7

-- Commit Summary --

  • Allow for stringified option keys and stringified "via" option
  • Revert "Allow for stringified option keys and stringified "via" option"
  • depricated -> deprecated typo
  • Allow for stringified keys in configs

-- File Changes --

M lib/pony.rb (59)
M spec/pony_spec.rb (25)

-- Patch Links --

 https://github.com/benprew/pony/pull/7.patch
 https://github.com/benprew/pony/pull/7.diff


Reply to this email directly or view it on GitHub:
#7

@julik
Copy link
Author

julik commented Jan 9, 2012

Thanks for the quick reaction! I'll take a stab at using Hashie, if you don't object to it becoming a dependency.

Specifying symbols in YAML is an option but the first Google result for it will likely be the Ruby yaml class notation which is ugly.

@julik julik closed this Jan 9, 2012
@julik
Copy link
Author

julik commented Jan 9, 2012

Hm. Unfortunately IndifferentAccess is only in the 2.0 branch of Hashie which is not stable yet. Is it good for Pony to depend on a prerelease gem? Maybe you could include my stub as a temporary solution and replace it with Hashie once they push out 2.0?

@julik julik reopened this Jan 11, 2012
@konklone
Copy link

I'd like to draw attention back to this pull request. Applications using safe_yaml to ensure that their YAML processing is naive and to-spec (doesn't do any Ruby-specific processing) won't get that symbol conversion.

This is an example of the manual conversion I need to do in my own code now:
https://github.com/sunlightlabs/scout/blob/master/config/email.rb#L49

So one way or the other, pony should stop depending on Ruby-specific YAML processing.

@konklone
Copy link

This is still an issue -- libraries in Ruby-land, where configuration is frequently read in from YAML files, should accept either string or symbol keys. Ruby-specific processing of YAML files, which symbol keys depends on, is a security antipattern.

@benprew
Copy link
Owner

benprew commented Dec 13, 2017

Closing this PR, but opening issue #20 to put together a fix.

@benprew benprew closed this Dec 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants