raise ApiVersionNotSetError if Base.api_version is nil

[jtgrenz]

Currently if you try to access a resource without setting an api_version first, you raise an exception:

NoMethodError: undefined method `construct_api_path' for nil:NilClass`
from .../shopify_api/lib/shopify_api/resources/base.rb:73:in `prefix'

Its not a super clear error. Instead, lets raise an error if the api_version hasn't been set (aka ShopifyAPI::Base.api_version would return nil) to let developers know they need to set the api version. This error is super easy to run into if you spin up the shopify_api_console and forget you need to set a version first.

[eapache]

Works for me. The other alternative would be to do this check in the caller? I'm not sure what's more appropriate for this gem.

[jtgrenz]

I initially had the check in Base.prefix since thats where the error most often occurs, but when I searched the code base to see if api_version.construct_api_path was called anywhere else, I found it in the Assets#find method and figured maybe it was best to prevent api_version from returning nil at all. If its important that api_version is able to return nil, then maybe we can just add it to all callers.

Looks like all the calls to api_version include:

• Base.prefix calling api_version.construct_api_path
• Asset.find calling api_version.construct_api_path
• GraphQL.initialize calling api_version.construct_graphql_path
• TestCase.fake calling api_version.construct_api_path (unrelated to Base but uses a default version if nil)
• Session#valid calling api_version.present? (session is unrelated to Base however so this might be appropriately nil)

[alexaitken]

Another alternative is moving over to a Null Object for no api version, so that it doesn't have an error like NoMethodError. Instead we can raise an exception that points to the gem has not been configured with an api version.

This will remove the need to do a conditional check on nil on every use of api version in the gem.

[jtgrenz]

Ahhhh, forgot the NullObject pattern.

Just updated. Base.api_version works as usual, but instead of returning if _api_version is not defined, it returns ApiVersion::NullVersion. I gave it a class level method_missing method to raise and also a nil? method.

[alexaitken]

Null object does not have to implement nil?

Instead implement a null version of all the methods on ApiVersion.

My suggestion would be

class NoVersionConfigured < StandardError; end

class NullVersion < ApiVersion
  def initialize
    @version_name = "no-version"
    @numeric_version = 0
  end

  def stable? 
     false
  end
  
  def construct_api_path(path)
        raise ApiVersionNotSetError, "You must set ShopifyAPI::Base.api_version before making a request."
  end
  
  def construct_graphql_path
      raise ApiVersionNotSetError, "You must set ShopifyAPI::Base.api_version before making a request."
  end
end

[jtgrenz]

If someone was checking to see if the version was nil, but it was actually NullVersion, I figured it fulfills the same meaning in that its not a version. Its just a fancier NilClass

I also think its probably misleading to have a null version respond to message other than raising an error. ShopifyAPI::Base.api_version.stable? # => false implies there is a version set, its just not stable (:unstable perhaps). I did consider implementing every method and having it raise, but method_missing seemed like an easier choice.

[alexaitken]

We could have it raise on all methods that matter. The main goal of the Null Object pattern is to remove nil checks and always have an implementation of the interface. Responding true to the nil? method would encourage the thing that we are trying to remove.

[jtgrenz]

The behavior we're trying to remove sending messages ApiVersion messages to NilObject where we expect an instance of ApiVersion and raising a no_method error. We don't want to provide any default behavior, we just want to replace any methods that ApiVersion has raise with a more meaningful message and avoid adding conditionals to handle the NoMethodError.

If we don't redefine nil? to true (and in hindsight, empty? and present?), then those values default to making the object truthy. Developers will still check if ShopifyAPI::ApiVersion.api_version.nil? but the code will instead now blowup at an expected location opposed to where they expected it to possibly be nil.

[eapache]

So there are two different ways of conceptualizing the NullObject pattern: as a "better null", which just has nicer errors or other properties, or as a specific non-null instance of the object which happens to get used when you might otherwise return null. Without getting too into the weeds of OO design, either will work for this particular case, though the latter (as Alex has suggested) is usually more common and probably what I'd go with for convention.

[jtgrenz]

updated so NullVersion only defines construct_api_path, construct_graphql_path and stable? and raises on each. Any other method will just inherit from Object. Any additional NoMethodErrors are probably unlikely but are already more helpful by virtue of not originating from the NilClass. Ie: NoMethodError: undefined method 'whatever' for ShopifyAPI::ApiVersion::NullVersion gives a better indication that you don't have a version.