fog / fog-core

fog's core, shared behaviors without API and provider specifics
MIT License
45 stars 95 forks source link

Inconsistent Hash Access #46

Open krames opened 10 years ago

krames commented 10 years ago

This issue was spurred by https://github.com/fog/fog/pull/2939.

This is certainly not the first time, I have seen downstream developers expect to use string and symbols interchangeably. Doing a quick search through issues shows 33 issues have been submitted about this and something tells me there are a lot more that aren't being reported.

Should address this and if so how?

/cc: @geemus @tokengeek

tokengeek commented 10 years ago

I'm not sure.

Part of me wonders if it is just bad Rails practice catching people out. Perhaps it's not our problem to solve.

Part of me wonders if it is more primitive obsession and is something that a real "Response", "Metadata" or "Credentials" object should be dealing with rather than souping up Hash.

I can see the benefits and the number of issues it has caused. Something is bugging me about just making it interchangeable though.

I'd rather us use a library like https://github.com/intridea/hashie than us roll our own though.

If this is such an obvious problem that Rails solved it why has it not made it into the Ruby language Hash itself?

Will someone bring out the old "Use Symbols because they are more efficient that Strings" blog post and then find a HTTP header you can not represent as a Symbol.

So I'm not sure either way. I'm pretty sure I've fallen for this problem myself (at least once today) but don't know if we want to be seeing people typecasting hashes everywhere to indifferent versions or monkey patching Hash itself.

Since the primary use case for fog is as a library for others, we shouldn't be monkey patching which will still allow people to pass in vanilla hashes and experience issues the same way.

We have a bit more control about returned Hashes though.

krames commented 10 years ago

I have seen this issue in a lot of cases where people are using hashes as a parameter to the request layer and or passing in attribute information. (I know I am guilty of it)

My gut thought is that we should introduce some decorator or utility method to help fog devs address this concern. I can also see trying to standardize on strings or symbols. Something tells me both of those are easier said than done.

nirvdrum commented 10 years ago

I've previously filed an issue. The real implementations all seem to interchangeably use Strings and Symbols, whereas all the mock implementations seem to only allow Strings. I think part of the historic oddity here is AWS uses headers that can't easily be represented as Symbols in Ruby 1.8. This might be cleared up in a 2.0 release if we standardized on Symbols everywhere shrug

geemus commented 10 years ago

Yeah, I also feel a bit dirty about instituting what seems like a Rails-ism. But I also want fog to be easy/usable. Standardizing on one or the other could also help, but as mentioned I'm fairly sure there would be exceptions still and confusion. I just feel so dirty about indifferent access...

nirvdrum commented 10 years ago

To finish my half thought there, standardizing on Symbols in fog 2.0 might be easier as we go Ruby 1.9+.

krames commented 10 years ago

I am indifferent (hah!) to which one we pick, but I am getting the feeling every one would prefer to standardize on symbols right?

@tokengeek Is this something that robocop could detect?

nirvdrum commented 10 years ago

My preference is for symbols because of GC. But if you're using a lot of ad-hoc headers, then String have a benefit because they can be GC'd.

tokengeek commented 10 years ago

@krames Not out of the box I don't think. You can write custom cops. So with that you could possibly pick up on warnings when strings are used as Hash keys. I'm not sure how much work it could be.

+1 for symbols though

plribeiro3000 commented 9 years ago

Same as fog/fog#362!

cphrmky commented 8 years ago

Was a consensus ever reached on this issue? I just ran into it with fog-softlayer/issues/86.

nirvdrum commented 8 years ago

I'll just note that in more recent Ruby releases, symbols can be GC'd. With fog planning to go Ruby 2.0+, the difference in GC behavior for Strings and Symbols should fade.

(PS: I see that my previous comment was a bit ambiguous. My preference for symbols was to prevent overall object churn. Frozen string literals, however, basically mitigate that problem. My concern about symbols not being able to be GC'd was just that you could fill the symbol table with unbounded growth if arbitrary headers could be used. The symbol table can now GC, so that concern is also mitigated.)

geemus commented 8 years ago

Being consistent internally seems good, but being usable for end-users is perhaps even better/more important. I'm open to doing what we need to do and it sounds like we are moving toward a place where some of our more concrete concerns are taken care of (and whether we feel good about it or not perhaps doesn't matter as much as how usable it is or is not).

cphrmky commented 8 years ago

👍 for usability

stale[bot] commented 6 years ago

This issue has been automatically marked stale due to inactivity. It will be closed if no further activity occurs. Thank you for your contributions.