Response#method_missing returns nil by default
Closed this issue · 6 comments
The implementation of method_missing in ResponseState::Response has a default return value of nil due to the fact that valid_states is empty by default. This is a risky situation when calling methods on a response. For example, if you do
puts response.mesage
You will be printing an empty string rather than the intended message, and there will be no exception raised.
You are correct that the default action for method_missing when there are no valid_states is nil and that is intentional.
We recommend you actually do set valid_states and now with the response_states macro available on your service classes this is even easier than before.
However, if you don't set the valid_states then yes the default will be nil. There is no other reasonable default.
The only other option would be to force you to set valid_states and I'm not sure if that's a direction we want to go.
I'll discuss with @brianvh.
I can honestly say that I don't like the black hole nature of the Response class, when valid_states isn't used in the Service sub-class. However, fixing this problem would result in a fairly significant, backwards-incompatible change to the library.
I think we should start with a caveat-emptor style addition to the README, so that developers are made aware of Response's black hole potential.
Perhaps it appropriate to just go ahead and make the breaking changes right now before you get your blog post out. We have fairly minimal exposure right now and while there are a few projects using it, It wouldn't be too bad to update those to make sure the class macro is used on the services.
Actually, that's a good plan...
You could allow people to explicitly set valid_states=nil for temporary backwards compatibility. Also, did you consider making the valid states a property of ResponseState::Service rather than of ResponseState::Response? It doesn't make sense to let your set of valid states be different for different responses. For example, send_state could set it to self.class.valid_states or something like that.
@pdg137 I think if we are going to have this we should go all the way and not allow empty valid_states.
Also, we already have the ability to set the valid states on the service. That's the class macro I've mentioned in several of the comments on this issue. It's also the preferred way to use the system and the readme will reflect that as part of implementing this breaking change.