Simplified NativeSender class ( ability to do caching )

[DmytroVasin]

Idea: Smarty Street is remote (3rd party ) service. From my point of view - we need to provide an interface for caching similar queries, to not call Smarty Street all the time.

Solution: If I understood correctly: native_sender.rb is the only one file which interacts with Smarty Street API. It responsible for: build request, send a request, convert/prepare a response.

I did small refactoring ( without changing the behavior of the gem ). We just separate logic that makes a request and converts the response to SmartyStreet objects.

In such case we will have an ability to do next:

module SmartyStreetsCaching
  def make_request(smarty_request)
    request_uri = "#{smarty_request.url_prefix}?#{create_query(smarty_request)}"
    cache_key = "smarty_streets:[#{Digest::MD5.hexdigest(request_uri)}]"

    Rails.cache.fetch(cache_key, expires_in: 1.day) do
      super
    end
  end
end

SmartyStreets::NativeSender.prepend(SmartyStreetsCaching)

# Note: That is a simplified version of caching, it does not care about the HTTP method, headers, etc.

Could you please check the logic. And give some feedback if any. Thank you.

[mdwhatcott]

@DmytroVasin - Interesting idea!

Could you go into more detail about your use case and why caching would benefit you?

Caching introduces a very stateful element to an otherwise stateless library. We release regular data and code updates to the API and if a long-lived app is caching indefinitely, that client wouldn't receive the updates for cached addresses. Would the cache grow indefinitely (memory leak) or be capped? I'm not exactly sure the complexity needed to get this right is worth the development horsepower required. This is only the first time this feature has been requested. Also, we generally try to keep feature parity between all 8 of our SDKs. Generally, for a feature to be included, it needs to be compelling enough to warrant the effort needed to provide that feature in 8 languages.

I wonder if the wireup/builder could simply provide a hook/method for you to supply your own caching layer. I'm not convinced that a caching layer is generally useful enough to include for everyone.

BTW, because of the high cost of lookups to the International Street API we already implement server-side caching to prevent runaway lookups to that API.

[DmytroVasin]

@mdwhatcott Thank you for your feedback.

Sorry, but maybe you got me wrong.

My PR does not implement caching feature. Moreover, it does not implement any feature at all.

The main purpose of this PR is to change a little bit the internal structure of the library to have an ability to patch it simpler. Currently, the library does not allow to intercept the request before sending it to the SmartStreet server.

My patch just reorganizes the internal structure of your code. ( decouple request sender from request builder )

What is the aim?

In our app we create a batch of similar records with the same addresses, in before creation hook we validate address through your app.

But we know that records are similar - so we don't want to send additional HTTP requests.

That's is why we want to implement caching ( on our side ). With that idea, we started reviewing the implementation of smarty-street-ruby gem and realize that there is no way to intercept the request. The logic of "request creation" tightly coupled to "request sending" logic.

Now, our app working through my patch, but I assume that is a really nice patch that provides alot of flexibility to end user.

TLDR.

PR - is just a small refactoring of internal structure. It decouples "request creation" and "request sending" logic.

I hope I clarify a little bit the reason for the creation of PR

Thanks.

[mdwhatcott]

@DmytroVasin - Thanks for clarifying.

Couldn't you introduce your own caching layer that is completely independent of the internals of this SDK? Just hash the address details that you would assign to a lookup, check your own cache and then only submit that record/lookup to this SDK if it's not found in the cache.

[DmytroVasin]

Yep, that was the first thoughts that I had. But I picked another way ( with interceptor ) - it just seems more natural in usage.

But nevertheless, I've understood your doubts. I've also don't like refactoring only as refactoring :) You can close PR if you don't see any profit for your SDK.

Thanks for your time.