From c8ae2d5e5a283cd1cb86536345b22b536f5ff619 Mon Sep 17 00:00:00 2001 From: Evan Owen Date: Thu, 16 May 2013 15:24:23 -0700 Subject: [PATCH] Convert resource links from procs to serializable hash https://github.com/recurly/recurly-app/pull/1440#issuecomment-18028526 Procs can't be properly marshalled, so hrefs are lost when Recurly::Resource objects are cached. By storing links as a hash and resolving the href without a proc, we can properly marshal them from cache. Acked-by: Stephen Celis Signed-off-by: Evan Owen --- Gemfile.lock | 2 +- lib/recurly/account.rb | 4 +- lib/recurly/api.rb | 6 +-- lib/recurly/coupon.rb | 4 +- lib/recurly/invoice.rb | 8 ++-- lib/recurly/resource.rb | 75 +++++++++++++++++++++++++---------- lib/recurly/subscription.rb | 16 ++++---- lib/recurly/transaction.rb | 4 +- spec/recurly/account_spec.rb | 6 --- spec/recurly/resource_spec.rb | 4 +- 10 files changed, 78 insertions(+), 51 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index dc276fde6..45123a32a 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -4,7 +4,7 @@ PATH recurly (2.1.8) GEM - remote: http://rubygems.org/ + remote: https://rubygems.org/ specs: addressable (2.2.6) bouncy-castle-java (1.5.0146.1) diff --git a/lib/recurly/account.rb b/lib/recurly/account.rb index 16beadfca..d546b7c17 100644 --- a/lib/recurly/account.rb +++ b/lib/recurly/account.rb @@ -51,8 +51,8 @@ def invoice! # (e.g., the account is already opwn), and may raise an exception if the # attempt fails. def reopen - return false unless self[:reopen] - reload self[:reopen].call + return false unless link? :reopen + reload follow_link :reopen true end diff --git a/lib/recurly/api.rb b/lib/recurly/api.rb index 21c13b547..33851a139 100644 --- a/lib/recurly/api.rb +++ b/lib/recurly/api.rb @@ -41,13 +41,13 @@ def accept_language=(language) # @return [Net::HTTPOK, Net::HTTPResponse] # @raise [ResponseError] With a non-2xx status code. def head uri, params = {}, options = {} - request :head, uri, { :params => params }.merge(options) + request :head, uri, { :params => params || {} }.merge(options) end # @return [Net::HTTPOK, Net::HTTPResponse] # @raise [ResponseError] With a non-2xx status code. def get uri, params = {}, options = {} - request :get, uri, { :params => params }.merge(options) + request :get, uri, { :params => params || {} }.merge(options) end # @return [Net::HTTPCreated, Net::HTTPResponse] @@ -64,7 +64,7 @@ def put uri, body = nil, options = {} # @return [Net::HTTPNoContent, Net::HTTPResponse] # @raise [ResponseError] With a non-2xx status code. - def delete uri, options = {} + def delete uri, body = nil, options = {} request :delete, uri, options end diff --git a/lib/recurly/coupon.rb b/lib/recurly/coupon.rb index 35657169d..be3eb05b4 100644 --- a/lib/recurly/coupon.rb +++ b/lib/recurly/coupon.rb @@ -51,7 +51,7 @@ def save # account = Account.find account_code # coupon.redeem account def redeem account_or_code, currency = nil - return false unless self[:redeem] + return false unless link? :redeem account_code = if account_or_code.is_a? Account account_or_code.account_code @@ -59,7 +59,7 @@ def redeem account_or_code, currency = nil account_or_code end - Redemption.from_response self[:redeem].call( + Redemption.from_response follow_link(:redeem, :body => (redemption = redemptions.new( :account_code => account_code, :currency => currency || Recurly.default_currency diff --git a/lib/recurly/invoice.rb b/lib/recurly/invoice.rb index be04af64e..bc13954d6 100644 --- a/lib/recurly/invoice.rb +++ b/lib/recurly/invoice.rb @@ -40,8 +40,8 @@ class Invoice < Resource # @return [true, false] +true+ when successful, +false+ when unable to # (e.g., the invoice is no longer open). def mark_successful - return false unless self[:mark_successful] - reload self[:mark_successful].call + return false unless link? :mark_successful + reload follow_link :mark_successful true end @@ -50,8 +50,8 @@ def mark_successful # @return [true, false] +true+ when successful, +false+ when unable to # (e.g., the invoice is no longer open). def mark_failed - return false unless self[:mark_failed] - reload self[:mark_failed].call + return false unless link? :mark_failed + reload follow_link :mark_failed true end diff --git a/lib/recurly/resource.rb b/lib/recurly/resource.rb index c60d404a9..37c0cdcc4 100644 --- a/lib/recurly/resource.rb +++ b/lib/recurly/resource.rb @@ -393,13 +393,10 @@ def from_xml xml xml.each_element do |el| if el.name == 'a' - name, uri = el.attribute('name').value, el.attribute('href').value - record[name] = case el.attribute('method').to_s - when 'get', '' then proc { |*opts| API.get uri, {}, *opts } - when 'post' then proc { |*opts| API.post uri, nil, *opts } - when 'put' then proc { |*opts| API.put uri, nil, *opts } - when 'delete' then proc { |*opts| API.delete uri, *opts } - end + record.links[el.attribute('name').value] = { + :method => el.attribute('method').to_s, + :href => el.attribute('href').value + } next end @@ -407,17 +404,16 @@ def from_xml xml resource_class = Recurly.const_get( Helper.classify(el.attribute('type') || el.name), false ) - record[el.name] = case el.name + case el.name when *associations[:has_many] - Pager.new resource_class, :uri => href.value, :parent => record + record[el.name] = Pager.new( + resource_class, :uri => href.value, :parent => record + ) when *(associations[:has_one] + associations[:belongs_to]) - lambda { - begin - relation = resource_class.from_response API.get(href.value) - relation.attributes[member_name] = record - relation - rescue Recurly::API::NotFound - end + record.links[el.name] = { + :resource_class => resource_class, + :method => :get, + :href => href.value } end else @@ -633,9 +629,11 @@ def persisted? # account[:last_name] # => "Beneke" # @see #write_attribute def read_attribute key - value = attributes[key = key.to_s] - if value.respond_to?(:call) && self.class.reflect_on_association(key) - value = attributes[key] = value.call + key = key.to_s + if attributes.key? key + value = attributes[key] + elsif links.key?(key) && self.class.reflect_on_association(key) + value = attributes[key] = follow_link key end value end @@ -677,6 +675,39 @@ def attributes= attributes = {} } end + # @return [Hash] The raw hash of record href links. + def links + @links ||= {} + end + + # Whether a record has a link with the given name. + # + # @param key [Symbol, String] The name of the link to check for. + # @example + # account.link? :billing_info # => true + def link? key + links.key?(key.to_s) + end + + # Fetch the value of a link by following the associated href. + # + # @param key [Symbol, String] The name of the link to be followed. + # @param options [Hash] A hash of API options. + # @example + # account.read_link :billing_info # => + def follow_link key, options = {} + if link = links[key = key.to_s] + response = API.send link[:method], link[:href], nil, options + if resource_class = link[:resource_class] + response = resource_class.from_response response + response.attributes[self.class.member_name] = self + end + response + end + rescue Recurly::API::NotFound + raise unless resource_class + end + # Serializes the record to XML. # # @return [String] An XML string. @@ -865,8 +896,9 @@ def marshal_dump @href, changed_attributes, previous_changes, + response, etag, - response + links ] end @@ -879,7 +911,8 @@ def marshal_load serialization @changed_attributes, @previous_changes, @response, - @etag = serialization + @etag, + @links = serialization end # @return [String] diff --git a/lib/recurly/subscription.rb b/lib/recurly/subscription.rb index 214bdfe25..a1b326009 100644 --- a/lib/recurly/subscription.rb +++ b/lib/recurly/subscription.rb @@ -97,8 +97,8 @@ def subscription_add_ons= subscription_add_ons # subscription = account.subscriptions.first # subscription.cancel # => true def cancel - return false unless self[:cancel] - reload self[:cancel].call + return false unless link? :cancel + reload follow_link :cancel true end @@ -119,11 +119,11 @@ def cancel # subscription = account.subscriptions.first # subscription.terminate(:partial) # => true def terminate refund_type = :none - return false unless self[:terminate] + return false unless link? :terminate unless REFUND_TYPES.include? refund_type.to_s raise ArgumentError, "refund must be one of: #{REFUND_TYPES.join ', '}" end - reload self[:terminate].call(:params => { :refund => refund_type }) + reload follow_link(:terminate, :params => { :refund => refund_type }) true end alias destroy terminate @@ -134,8 +134,8 @@ def terminate refund_type = :none # (e.g., the subscription is already active), and may raise an exception # if the reactivation fails. def reactivate - return false unless self[:reactivate] - reload self[:reactivate].call + return false unless link? :reactivate + reload follow_link :reactivate true end @@ -145,8 +145,8 @@ def reactivate # (e.g., the subscription is not active). # @param next_renewal_date [Time] when the subscription should renew. def postpone next_renewal_date - return false unless self[:postpone] - reload self[:postpone].call( + return false unless link? :postpone + reload follow_link(:postpone, :params => { :next_renewal_date => next_renewal_date } ) true diff --git a/lib/recurly/transaction.rb b/lib/recurly/transaction.rb index c476f734f..b2434aefc 100644 --- a/lib/recurly/transaction.rb +++ b/lib/recurly/transaction.rb @@ -69,9 +69,9 @@ def save # @param amount_in_cents [Integer, nil] The amount (in cents) to refund # (refunds fully if nil). def refund amount_in_cents = nil - return false unless self[:refund] + return false unless link? :refund refund = self.class.from_response( - self[:refund].call :params => { :amount_in_cents => amount_in_cents } + follow_link :refund, :params => { :amount_in_cents => amount_in_cents } ) refund.uuid == uuid ? copy_from(refund) && self : refund end diff --git a/spec/recurly/account_spec.rb b/spec/recurly/account_spec.rb index e8ab4dcd6..38080b9df 100644 --- a/spec/recurly/account_spec.rb +++ b/spec/recurly/account_spec.rb @@ -80,12 +80,6 @@ XML end - it 'handle empty values for embedded billing info' do - stub_api_request :post, 'accounts', 'accounts/create-201' - @account.billing_info = { :number => '', :verification_value => '' } - @account.save.must_equal true - end - describe "persisted accounts" do before do @account.persist! diff --git a/spec/recurly/resource_spec.rb b/spec/recurly/resource_spec.rb index 0e1a84ab0..54af8837e 100644 --- a/spec/recurly/resource_spec.rb +++ b/spec/recurly/resource_spec.rb @@ -149,9 +149,9 @@ def to_param 3.times { |n| record[:seasons][n].must_be_kind_of Integer } record[:never_gonna_happen]['season'].must_be_kind_of Integer stub_api_request(:put, 'resources/1/renew') { "HTTP/1.1 200\n" } - record[:renew].call + record.follow_link :renew stub_api_request(:delete, 'resources/1/cancel') { "HTTP/1.1 422\n" } - proc { record[:cancel].call }.must_raise API::UnprocessableEntity + proc { record.follow_link :cancel }.must_raise API::UnprocessableEntity end end end