softlayer / softlayer-ruby

http://softlayer.github.io/softlayer-ruby/
MIT License
54 stars 35 forks source link

Allow more direct access to the hashes that are used to implement Model Objects #20

Closed SLsthompson closed 10 years ago

SLsthompson commented 10 years ago

In issue #19, cbreden suggested:

Redirect open-ended hash calls to a instantiated object to the associated object mask via

def @sl_hash[key] end This allows for something like:

Client.virtual_servers().first['hostname'] # -> "instance_hostname"

...Along with what you currently allow via method_missing:

Client.virtual_servers().first.hostname # -> "instance_hostname"

SLsthompson commented 10 years ago

I went on to say that the idea gave me the willies. cbreden replied, rationally:

I think it's just barely above the "because we can mark" since the SoftLayer API objects historically have been hashes. Adding this in to the base model class properly won't introduce any collisions. Users the ability to weave some hash magic throughout these objects if they so choose. It also introduces the opportunity to define unique behavior in the sub classes and tap into

def []=(key,value)
  do_something
end

But that said, I won't deny it's ooginess. I think the debate would boil down to purity vs. flexibility. Pushing undeclared calls to the @sl_hash via method_missing is already leaning towards the flexibility angle.

SLsthompson commented 10 years ago

Here is the "purity argument".

The sl_hash is intended to be an implementation detail of the model objects. Ideally the model objects would have attributes (a lá attr_reader) for each of the keys in the hash. To allow them to be treated as such, the base model implements method_missing and exposes the keys in the hash as routines just as if they had been declared as attributes.

Allowing the user to address the hash directly violates the encapsulation of the object and exposes an implementation detail. The purity argument is that this is a big no-no.

Dutifully recorded here for posterity.

SLsthompson commented 10 years ago

@cbreden, I have a question about this suggestion that I want to ask. I'm going to leave much of the thinking behind the question intentionally vague (so you are not influenced by my background processing). Please let me know what you think.

The question is this, given an object from the framework, say an instance of SoftLayer::Ticket, and given the model you have requested. Would you expect that the expressions:

my_ticket.title

and

my_ticket['title']

Would always return identical results?

cbreden commented 10 years ago

Regarding the purity argument, I do agree that attr_reader (maybe attr_accessor) definitions would essentially be the gold standard for a clean ("pure") ruby object. However, once we start talking about method_missing, it starts to get more subjective. I personally feel that while even though it closely mirrors the runtime effect of attr_readers, the current 'method_missing method' strays quite far away from the purity paradigm already.

Just to state the obvious to make the point, a main downside is the lack of documentation for the callable instance variables -- they're not directly searchable in the docs and IDEs won't help you with auto-complete. Developers will have to dig into this code to see what variables have been exposed through the default object mask. When it gets to that point, people may lose momentum.

So, given that they must currently still become familiar with that hash, it seems to make sense to expose it in case they have some hash operations they'd really like to use.

Now regarding that question......

cbreden commented 10 years ago

That's a very deep question. I know there a million blogs that argue about these semantics, but I'm going to go with what my gut is thinking and dance around the question a bit.

Breaking these down one at a time, my_ticket['title'] is actually the most clear to me. I expect it to return a value with essentially no processing -- a quick, atomic, already-in-memory process 99% of the time. It might be a dirty value that hasn't been written back to the 'master' source yet, but it should be safe to use.

my_ticket.title has a bit more uncertainty. It's implementation is less clear (not necessarily a bad thing), but I wouldn't necessarily know if that calls a live web service, hits an in-memory variable, or has to invoke some processing. We both know it's just hitting the same hash variable (assuming it hasn't been overloaded by an actual method somewhere in the sub-classes), but there's no way of knowing unless you look.

So, to me, they definitely don't imply the same behavior.

So back to that actual question about the returned values. Despite what I just said..........eeeeeeeeehh........I think I would expect them to return the same results assuming I know that to be a property of the SoftLayer object.

We're notably not doing my_ticket.get_title (methodmissing could snip the 'get' and query the hash for the remaining 'title'). Not suggesting that we do it either, but I'm just trying to think about all the ways to assess those @sl_hash variables and gauge which ones feel correct.

Hit me with your best retort! I know I'll be thinking about this one for awhile.

SLsthompson commented 10 years ago

OK. Here's the Scenario that got me thinking about this. In the SoftLayer universe you can create a "standard support ticket" (compare to a "system administration ticket"). A standard supper ticket has both a subject and a title. The subject is one selection out of a fixed collection of values ("Private Network Question", "Sales Request", etc.). The title is provided by the customer when the ticket is created.

However, when the ticket is created, the ticketing system changes the title that the user entered by prepending the subject on the front of it. So if you select the "Sales Request" subject, and enter "I want to order 10,000 servers" for the title, the actual value for title in the system becomes "Sales Request - I want to order 10,000 servers".

This is a problem, for example, in the mobile application because the subject string pushes the title to the right (and potentially off screen on a small device). You're left with a long list of the same subject string over an over, and it's hard to distinguish between multiple tickets with the same subject but different titles

(whew)

With that background, what if the object model for ticket actually declared an attribute ticket that took the raw value (["ticket"]) and stripped off the pre-pended subject if it was there. In that instance the view of title for the model is very different than the title which is a raw member of the sl_hash.

How do you feel about that scenario?

Taking that to an extreme on the purity scale... would it make more sense to implement the hash syntax for low level access to the sl_hash and remove the method_missing mechanism altogether so that there is a clearer distinction when you are accessing the low-level information vs. the higher-level object model?

cbreden commented 10 years ago

Ah, great example. Good case for custom interpretation of the SL properties. Makes for another interesting discussion about how we'd expect to be able to search on property values....

I think making that distinction more clear might be for the best. Accessing the hash can imply you're hitting the low-level SoftLayer properties directly (with the expectation that you'll know how to use it). Then, for whatever higher-level object variables/methods end up coming into play, they'll be documented/searchable in the docs and be flagged/tagged inside of IDEs.

It seems that what remains is whether or not we should statically define long lists of attr_readers for the simpler properties that we expect to get a lot of hits. Do you know if there's an easier 'ruby' way to expose a batch of variables with proper documentation?

SLsthompson commented 10 years ago

Do you know if there's an easier 'ruby' way to expose a batch of variables with proper documentation?

None that I'm aware of. Adding the actual attributes would be easy enough but adding the documentation is a bit more head-scratchy. Let me see what Rdoc might offer. Attributes that arise from metaprogramming in Ruby is common enough that I'll be there's something.

SLsthompson commented 10 years ago

Let me see what Rdoc might offer.

http://docs.seattlerb.org/rdoc/RDoc/Parser/Ruby.html

cbreden commented 10 years ago

Jeez, it barely looks like we save anything.

##
# :attr_reader: my_attr_name

is barely different from...

# blah
attr_reader my_attr_name

...except the later will get properly registered in IDEs like Eclipse. I think we should maybe just go with that.

SLsthompson commented 10 years ago

Eclipse… :-)

SLsthompson commented 10 years ago

There is a complication with using attr_reader. For the most part we want to promote attributes that already exist in the SoftLayer Hash. For example, on Account I want to make companyName a model-level attribute of the Account class... that should pretty much be a reflection of the companyName property in the underlying Hash.

attr_reader assumes that there is an instance variable for this (@companyName in this case) and it simply returns the value of that instance variable. To make attr_reader work, then, I have to create instance variables for all the properties in the hash that are promoted to the model level. That, in essence, would mean that there is two copies of the data which is probably a recipe for disaster in the future of one kind or another. (shaking the :8ball:)

What I'm working with right now is a class method called sl_attr. What it does is defines a method for each hash property that is promoted to attribute status. The method simply returns the current value out of the hash. So without documentation Account looks something like:

sl_attr :companyName
sl_attr :firstName
sl_attr :lastName
sl_attr :address1

etc...

It's lovely as it stands but (to your point) these won't be recognized by IDEs and to get them to show up in the documentation system I have to do:

##
# :attr_reader: companyName
# This is the company name attached to the contact info for the account
sl_attr :companyName

Which, as you realized, is annoying. I've tried all sorts of things to compact the comments, but RDoc really wants it broken down pretty much as you see it above

SLsthompson commented 10 years ago

I don't think this is as bad as it first seems. The documentation engine can pick up the name of the attribute, the benefit of having the documentation both "right there" in the source and in the generated Rdoc documentation is nice. The syntax seems annoying, but I think it pays for itself.

##
# :attr_reader:
# This is the company name attached to the contact info for the account
sl_attr :companyName
cbreden commented 10 years ago

I guess that seems adequate. Maybe IDEs have a way to monitor for custom elements like "sl_attr". Otherwise, at least the docs look good.

I figure for something like 'password' (implying the root or administrator password), you'll just make a custom function. What about for singleton values that are that aren't at the top level of the hash? Say for example, a virtual server's datacenter's full name?

SLsthompson commented 10 years ago

I've resigned myself with the disappointment that we can't model out the entire set of SoftLayer data types all at once.

The value of my_server.datacenter is a hash. You can the full name as:

my_server.datacenter["longName"]

Once you step beyond the "thin veneer" of the model, you're back in hash-land. As we evolve the model, the veneer will become more substantial and real, but along the way I imagine it will require scripts to change. I don't really see a good way around it.

For example, instead of having a server return it's primary_public_ip address directly, I think it should probably return its primary_public_interface and the ip address would simply be an attribute of the nic. But I don't have a class to model a NetworkInterface at the moment. When we add it, we'll either have to leave primary_public_ip on Server for backward compatibility, or ask folks to change their scripts.

cbreden commented 10 years ago

I would definitely advocate leaving something like primary_public_ip on Server. It goes back to one of the main points of the semantic model: strive to make these objects easy to use by promoting common use cases. I'd say with confidence that 9 times out of 10, people just want that primary public ip string. Keeping that shortcut makes perfect sense in this particular example.

Unfortunately, like you were getting at, I know we'll always be debating which uses are _common_ and deserve that treatment. I don't see a practical way to generate any kind of objective measurements here, so I think it'll a case-by-case discussion/analysis.

At the very least, is it safe to say that as long as we make sure these deeper hashed attributes hold a 1:1 relationship back to the associated object, we won't break anything? (--by inadvertently returning a single value when there might be many?) When we flesh out esoteric models like NetworkInterface, we can refactor these current hash shortcuts to the new object methods to support backwards compatibility.