Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

#configure loses nested hashes somehow #14

Open
jrochkind opened this Issue Sep 12, 2012 · 14 comments

Comments

Projects
None yet
3 participants
Collaborator

jrochkind commented Sep 12, 2012

This one is weird edge case of some kind, but i'm not sure what kind. Here's the reproduction:

conf = Confstruct::Configuration.new
conf.configure(    :top=>{:middle=>{"one"=>"two"}} )

conf.top.middle    # =>  {"one"=>"two"}
conf.top.middle.class # => Confstruct::HashWithStructAccess
conf.top.middle.one # => nil    WHAT?
conf.top.middle["one"]  # => nil  # uh?
conf.top.middle[:one]   #=> nil

What's going on? Is "one" somehow a reserved word too? Not sure. If we change the literal to a symbol, weirdly all is well. huh?

 conf = Confstruct::Configuration.new
 conf.configure(    :top=>{:middle=>{:one=>"two"}} )

 conf.top.middle.one  # => "two" # great!

Hmm, a string other than 'one'?

 conf = Confstruct::Configuration.new
 conf.configure(    :top=>{:middle=>{"foo"=>"two"}} )
 conf.top.middle # {"foo"=>"two"}
 conf.top.middle.foo  # => nil

Nope it's not the word 'one' I don't think.

But it doens't require three levels of nesting, one will do it with strings:

 conf = Confstruct::Configuration.new
 conf.configure("top" => { "bottom" => "bottom" })
 conf.top # => {"bottom" => "bottom" }
 conf.top.class #  => Confstruct::HashWithStructAccess
 conf.top.bottom # => nil
 conf.top["bottom"] #=> nil
 conf.top.keys # => ["bottom"]   WHAT???

Ugh. Any ideas?

Collaborator

jrochkind commented Sep 12, 2012

I see a spec for "it should initialize properly from a nested hash with string keys", which makes me suspect you ran into this problem before and fixed it for #initailize (it is indeed working there) but not #configure.

If you see this and can recall what was up and give me hints or commit references, please do. But i'm working on it.

Collaborator

jrochkind commented Sep 12, 2012

Hmm, definitely out of my league here.

I tracked down the previous commit fixing for "it should initialize properly from a nested hash with string keys":

50b9d84

Debugging and following that lead, I wanted to change this line:

https://github.com/mbklein/confstruct/blob/master/lib/confstruct/hash_with_struct_access.rb#L60

From HashWithStructAccess.new to HashWithStructAccess.from_hash

If the value is a hash (but not already a HashWithStructAccess), convert it to a HashWithStructAccess -- using 'from_hash' not using 'new'.

If I make that change, then the spec I added for the reproduction case above passes -- but a bunch of other specs start failing. Bah!

@mbklein, if you have any insight GREATLY appreciated.

Collaborator

jrochkind commented Sep 12, 2012

Hmm, I think some of those other failing specs may have been WRONG, they were actually spec'ing BUGGY behavior before. i'll get a fork and let you see it.

Collaborator

jrochkind commented Sep 12, 2012

nope, that's not it, they were right, and break by my change. Ooh geez. I need an assist.

(Man, was Confstruct Too Much Magic? But it's so useful!)

Owner

mbklein commented Sep 13, 2012

Hah. Yeah. Too Much Magic indeed. I'll take a look at this one as soon as I can, but I have to get my pentagram and chicken bones set up first. :-)

Collaborator

jrochkind commented Sep 17, 2012

any chance to take a look?

Owner

mbklein commented Sep 18, 2012

I think I got it. Try the new HEAD and see if it solves the problem. I'll release v0.2.5 if it works like you expect.

Collaborator

jrochkind commented Sep 18, 2012

Huh, not good, if I take out my local workaround for the confstruct bug, and point at git head confstruct, and run all my tests --- a bunch of my tests start failing that did not fail with released confstruct, including ones not obviously specifically related to to this bug.

Will investigate and try to figure out more. Man, this logic is tangled up internally, eh?

Collaborator

jrochkind commented Sep 18, 2012

ah, I see, the side effect of this is that ALL hash keys turn to symbols.

Before I could do this:

a= Confstruct::Configuration.new(:definition => {"a" =>  "b"})
a.definition.each_pair {|k, v| "#{k.titlelize}:#{v.titleize}"}

=> "A:B". 

Now k turns into a SYMBOL (not sure about v), so I get "undefined method 'titleize' on Symbol"

I think I understand why this is, why it's neccesary, and why it maybe even makes sense. What do you think? It is a backwards-incompat, but fortunately one that my tests have already caught (in at least some places) so I can fix.

At least in THIS app. I wonder if it would mess up some of my other apps using confstruct and somehow relying on the old behavior. Hmm, a mess.

(I wonder if your HashWithStructAccess's should be normalizing to strings rather than symbols anyway? You know symbols are 'interned' and never get GC'd, so it's dangerous to create a lot (like hundreds of thousands) of em? Might not matter here.)

Collaborator

jrochkind commented Sep 18, 2012

Ah, and my own local workaround in this partiuclar project (bento_search) resulted in the same failing tests with things that had been strings before turned to symbols, I just hadn't run my whole test suite yet. So, yeah, seems to be an inherent part of the fix. Is backwards incompat in some cases though. Hmm, what do you think?

Owner

mbklein commented Sep 18, 2012

Maybe. I can see using indifferent access for retrieval, and always using one or the other (strings, probably, due to the GC issue) for properties created on the fly.

Owner

mbklein commented Sep 18, 2012

I have a bunch of ideas for this but unfortunately no time to plow into it right now. Grrrrrr.

Collaborator

jrochkind commented Sep 18, 2012

Hmm, so what should we do?

I have a local workaround in my project even if you revert the commit in
HEAD (basically just making sure to turn Hash's to Confstructs before I
pass them to .configure, which is all hidden in my own gems internals, so
it's no problem.

But without the commit in HEAD, it is odd behavior -- you have a hash in
there, whose contents are not accessible via ANY method, not struct-like
access, not hash-like access with string key, not hash-like access with
symbol key. So that seems bad. but with commit in HEAD, some possible
backwards-breaking.

What do you want to do?

I'll try to find time to look at the diff for the commit in HEAD to see if
I can understand what you did, that I couldn't figure out myself, so I can
possibly help in the future if I have more time than you.

On Tue, Sep 18, 2012 at 5:53 PM, Michael B. Klein
notifications@github.comwrote:

I have a bunch of ideas for this but unfortunately no time to plow into it
right now. Grrrrrr.


Reply to this email directly or view it on GitHubhttps://github.com/mbklein/confstruct/issues/14#issuecomment-8672102.

I am having the same problem and just stumbled across this GH issue by accidence - could you make this behaviour prominent in the README so that it is crystal clear that we need to pass in symbols for now until this is fixed?

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