diff --git a/CHANGELOG.md b/CHANGELOG.md index 97e9076..49f527c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,41 @@ since Active Remote depends on specific Rails versions. ### Changed +- Update to ActiveModel 8.1 +- `#freeze` freezes the record, not just its attributes +- A rejected save keeps the caller's edits and change tracking instead of adopting the response +- Responses are merged into the record, so omitted attributes keep their value + +### Fixed + +- Transport errors raised `NameError` instead of `RpcFailedError` and friends +- `#hash` now agrees with `#==`/`#eql?` +- `#reload` marks the record as persisted, so the next `#save` no longer duplicates it +- `#delete`, `#destroy` and `#remote` clear stale errors before checking the result +- `#delete!` and `#destroy!` report the service's messages, not `#` +- `.primary_key` is inherited by subclasses +- `.attribute_names`, `#to_key` and `#scope_keys` no longer memoize stale values +- `#previous_changes` is populated after a successful save +- `.find` and the `.first_or_*` methods accept the documented protobuf and Active Remote arguments +- The `.first_or_*` methods unwrap a search request's repeated fields, so `name: ["foo"]` no longer + becomes `"[\"foo\"]"`; matching on several values now raises `ArgumentError` +- `belongs_to`/`has_one` memoize a `nil` association and honor an explicitly assigned `nil` +- Errors from repeated RPC calls no longer accumulate duplicates +- `#attribute_for_inspect` accepts a symbol name, matching `#[]` and `#[]=`; it previously + reported `"nil"` for every attribute +- Corrected documentation throughout that described ActiveRecord behavior this gem does not have, + named the wrong method, or had gone stale — including `#cache_key`'s `cache_versioning` claims, + `#to_key`'s example, and the return values documented for `#delete!` and `#destroy!` + +### Removed + +- `.attr_publishable` and `.publishable_attributes`. Their only consumers, the publication and JSON + serializer modules, were removed in 3.0.0; the macro has had no effect since + +## [8.0.0] - 2026-07-24 + +### Changed + - Update to ActiveModel 8.0 - Require Ruby 3.2 (to match Rails 8.0) - Fix Standard violations diff --git a/active_remote.gemspec b/active_remote.gemspec index 861966e..5459559 100644 --- a/active_remote.gemspec +++ b/active_remote.gemspec @@ -34,7 +34,7 @@ Gem::Specification.new do |spec| ## # Dependencies # - spec.add_dependency "activemodel", "~> 8.0.0" - spec.add_dependency "activesupport", "~> 8.0.0" + spec.add_dependency "activemodel", "~> 8.1.0" + spec.add_dependency "activesupport", "~> 8.1.0" spec.add_dependency "protobuf", ">= 3.0" end diff --git a/lib/active_remote/association.rb b/lib/active_remote/association.rb index 2e83553..93deb37 100644 --- a/lib/active_remote/association.rb +++ b/lib/active_remote/association.rb @@ -133,26 +133,26 @@ def validate_scoped_attributes(associated_class, object_class, options) private def perform_association(associated_klass, options = {}) + ivar = :"@#{associated_klass}" + define_method(associated_klass) do + # Keyed on presence, not truthiness, so a nil association isn't re-queried. + # Checked before resolving the class, since #classify runs the inflector + # uncached and would otherwise dominate every cached read. + return instance_variable_get(ivar) if instance_variable_defined?(ivar) + klass_name = options.fetch(:class_name) { associated_klass } klass = klass_name.to_s.classify.constantize self.class.validate_scoped_attributes(klass, self.class, options) if options.key?(:scope) - value = instance_variable_get(:"@#{associated_klass}") - - unless value - value = yield(klass, self) - instance_variable_set(:"@#{associated_klass}", value) - end - - value + instance_variable_set(ivar, yield(klass, self)) end define_method(:"#{associated_klass}=") do |new_value| raise "New value must be an array" if options[:has_many] == true && new_value.class != Array - instance_variable_set(:"@#{associated_klass}", new_value) + instance_variable_set(ivar, new_value) new_value end end diff --git a/lib/active_remote/attribute_methods.rb b/lib/active_remote/attribute_methods.rb index b5eb341..2d11cb8 100644 --- a/lib/active_remote/attribute_methods.rb +++ b/lib/active_remote/attribute_methods.rb @@ -2,12 +2,6 @@ module ActiveRemote module AttributeMethods extend ::ActiveSupport::Concern - module ClassMethods - def attribute_names - @attribute_names ||= attribute_types.keys - end - end - def [](attr_name) name = attr_name.to_s name = self.class.attribute_aliases[name] || name @@ -34,10 +28,10 @@ def []=(attr_name, value) # person.attribute_for_inspect(:created_at) # # => "\"2012-10-22 00:15:07\"" # - # person.attribute_for_inspect(:tag_ids) - # # => "[1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11]" + # person.attribute_for_inspect(:age) + # # => "42" def attribute_for_inspect(attr_name) - value = attribute(attr_name) + value = self[attr_name] if value.is_a?(String) && value.length > 50 "#{value[0, 50]}...".inspect @@ -47,9 +41,5 @@ def attribute_for_inspect(attr_name) value.inspect end end - - def attribute_names - @attributes.keys - end end end diff --git a/lib/active_remote/base.rb b/lib/active_remote/base.rb index a947df4..8d52825 100644 --- a/lib/active_remote/base.rb +++ b/lib/active_remote/base.rb @@ -35,11 +35,11 @@ class Base include ::ActiveRemote::Search include ::ActiveRemote::Serialization - # Overrides some methods, providing support for dirty tracking, - # so it needs to be included last. + # Overrides persistence methods to add dirty tracking, so it has to come + # after Persistence and Search. include ::ActiveRemote::Dirty - # Overrides persistence methods, so it must included after + # Overrides #save/#save! to validate first, so it has to come after Dirty. include ::ActiveRemote::Validations include ::ActiveModel::Validations::Callbacks @@ -54,15 +54,15 @@ def initialize(attributes = {}) end end - # Returns true if +comparison_object+ is the same exact object, or +comparison_object+ - # is of the same type and +self+ has an ID and it is equal to +comparison_object.id+. + # Returns true if +comparison_object+ is the same exact object, or is of the + # same type and its primary key is set and equal to this record's. # - # Note that new records are different from any other record by definition, unless the - # other record is the receiver itself. Besides, if you fetch existing records with - # +select+ and leave the ID out, you're on your own, this predicate will return false. + # Note that this does not consider whether either record is persisted: two + # unsaved records built with the same primary key compare equal. Records + # whose primary key is nil are only equal to themselves. # - # Note also that destroying a record preserves its ID in the model instance, so deleted - # models are still comparable. + # Note also that destroying a record preserves its attributes in the model + # instance, so deleted models are still comparable. def ==(other) super || other.instance_of?(self.class) && @@ -71,6 +71,16 @@ def ==(other) end alias_method :eql?, :== + # Records that are +eql?+ must hash alike, or Set, Array#uniq and Hash keys + # treat them as distinct. + def hash + if (key = send(primary_key)) + [self.class, key].hash + else + super + end + end + # Allows sort on objects def <=>(other) if other.is_a?(self.class) @@ -80,17 +90,8 @@ def <=>(other) end end - def freeze - @attributes.freeze - self - end - - def frozen? - @attributes.frozen? - end - - # Initialize an object with the attributes hash directly - # When used with allocate, bypasses initialize + # Initialize an object from an ActiveModel::AttributeSet, as built by + # .build_from_rpc. When used with allocate, bypasses initialize. def init_with(attributes) @attributes = attributes @new_record = false @@ -118,11 +119,6 @@ def inspect "#<#{self.class} #{inspection}>" end - - # Returns a hash of the given methods with their names as keys and returned values as values. - def slice(*methods) - methods.flatten.map! { |method| [method, public_send(method)] }.to_h.with_indifferent_access - end end ::ActiveModel::Type.register(:value, ::ActiveModel::Type::Value) diff --git a/lib/active_remote/dirty.rb b/lib/active_remote/dirty.rb index d3362ee..5a07383 100644 --- a/lib/active_remote/dirty.rb +++ b/lib/active_remote/dirty.rb @@ -18,35 +18,39 @@ def reload(*) end end - # Override #remote to provide dirty tracking. + # Override #remote to provide dirty tracking. A rejected write keeps its + # pending changes so the caller can fix and retry. # def remote(*) - super.tap do - clear_changes_information + super.tap do |success| + clear_changes_information if success end end - # Override #save to store changes as previous changes then clear them. + # Override #save to expose the changes it persisted as #previous_changes. + # Clearing them is #remote's job, which super calls. # def save(*) - if (status = super) - changes_applied - end + # Snapshot first: #remote clears the tracker before super returns. + mutations = mutations_from_database - status + super.tap do |status| + @mutations_before_last_save = mutations if status + end end - # Override #save to store changes as previous changes then clear them. + # Override #instantiate to provide dirty tracking. It swaps @attributes, so + # the tracker has to be reset or a freshly loaded record reports changes. # - def save!(*) + def instantiate(*) super.tap do - changes_applied + clear_changes_information end end private - # Override #update to only send changed attributes. + # Override #remote_update to only send changed attributes. # def remote_update(*) super(changed) diff --git a/lib/active_remote/dsl.rb b/lib/active_remote/dsl.rb index 8d7487e..13d893c 100644 --- a/lib/active_remote/dsl.rb +++ b/lib/active_remote/dsl.rb @@ -5,20 +5,6 @@ module DSL extend ActiveSupport::Concern module ClassMethods - # Whitelist enable attributes for serialization purposes. - # - # ====Examples - # - # # To only publish the :guid and :status attributes: - # class User < ActiveRemote::Base - # attr_publishable :guid, :status - # end - # - def attr_publishable(*attributes) - @publishable_attributes ||= [] - @publishable_attributes += attributes - end - def endpoint_for_create(endpoint) endpoints create: endpoint end @@ -66,12 +52,6 @@ def namespace(name = false) @namespace end - # Retrieve the attributes that have been whitelisted for serialization. - # - def publishable_attributes - @publishable_attributes - end - # Set the RPC service class directly. By default, ActiveRemote determines # the RPC service by constantizing the namespace and service name. # @@ -140,10 +120,6 @@ def _endpoints self.class.endpoints end - def _publishable_attributes - self.class.publishable_attributes - end - def _service_name self.class.service_name end diff --git a/lib/active_remote/errors.rb b/lib/active_remote/errors.rb index dd3849e..531c805 100644 --- a/lib/active_remote/errors.rb +++ b/lib/active_remote/errors.rb @@ -8,11 +8,13 @@ class ActiveRemoteError < StandardError class DangerousAttributeError < ActiveRemoteError end - # Raised by ActiveRemove::Base.save when the remote record is readonly. + # Raised by ActiveRemote::Base#save, #delete, #destroy and #update_attribute + # when the remote record or its class is readonly. class ReadOnlyRemoteRecord < ActiveRemoteError end - # Raised by ActiveRemote::Validations when save is called on an invalid record. + # Raised by ActiveRemote::Validations when save! is called on a record that + # fails local validation. #save returns false instead. class RemoteRecordInvalid < ActiveRemoteError attr_reader :record @@ -40,8 +42,9 @@ def initialize(class_or_message = "") end end - # Raised by ActiveRemove::Base.save! and ActiveRemote::Base.create! methods - # when remote record cannot be saved because it is invalid. + # Raised by ActiveRemote::Base#save!, .create! and #update_attributes! when + # the service rejects the write. Local validation failures raise + # RemoteRecordInvalid instead, since Validations runs first. class RemoteRecordNotSaved < ActiveRemoteError attr_reader :record diff --git a/lib/active_remote/integration.rb b/lib/active_remote/integration.rb index d2130d1..5273909 100644 --- a/lib/active_remote/integration.rb +++ b/lib/active_remote/integration.rb @@ -5,8 +5,10 @@ module Integration included do ## # :singleton-method: - # Indicates the format used to generate the timestamp in the cache key, if - # versioning is off. Accepts any of the symbols in Time::DATE_FORMATS. + # Indicates the format used to generate the timestamp #cache_key appends + # when ActiveRemote.config.default_cache_key_updated_at? is set. Accepts + # any of the symbols in Time::DATE_FORMATS. #cache_version does + # not consult this and always uses +:usec+. # # This is +:usec+, by default. class_attribute :cache_timestamp_format, instance_writer: false, default: :usec @@ -16,25 +18,25 @@ module Integration # Indicates whether to use a stable #cache_key method that is accompanied # by a changing version in the #cache_version method. # - # This is +false+, by default until Rails 6.0. + # This is +false+, by default. class_attribute :cache_versioning, instance_writer: false, default: false end # Returns a +String+, which Action Pack uses for constructing a URL to this - # object. The default implementation returns this record's id as a +String+, - # or +nil+ if this record's unsaved. + # object. The default implementation returns this record's primary key as a + # +String+, or +nil+ if that key has no value. # # For example, suppose that you have a User model, and that you have a # resources :users route. Normally, +user_path+ will - # construct a path with the user object's 'id' in it: + # construct a path with the user object's primary key in it: # # user = User.find_by(name: 'Phusion') - # user_path(user) # => "/users/1" + # user_path(user) # => "/users/ABC-123" # # You can override +to_param+ in your model to make +user_path+ construct - # a path using the user's name instead of the user's id: + # a path using the user's name instead: # - # class User < ActiveRecord::Base + # class User < ActiveRemote::Base # def to_param # overridden # name # end @@ -48,16 +50,18 @@ def to_param key&.to_s end - # Returns a stable cache key that can be used to identify this record. + # Returns a stable cache key that can be used to identify this record. The + # key is built from the record's primary key, not an id. # - # Product.new.cache_key # => "products/new" - # Product.find(5).cache_key # => "products/5" + # Product.new.cache_key # => "products/new" + # product.cache_key # => "products/ABC-123" # - # If ActiveRecord::Base.cache_versioning is turned off, as it was in Rails 5.1 and earlier, - # the cache key will also include a version. + # When ActiveRemote.config.default_cache_key_updated_at is set and the + # record carries an +updated_at+, the key also includes that timestamp, + # formatted with .cache_timestamp_format. # - # Product.cache_versioning = false - # Person.find(5).cache_key # => "people/5-20071224150000" (updated_at available) + # ActiveRemote.config.default_cache_key_updated_at = true + # product.cache_key # => "products/ABC-123-20071224150000000000" # def cache_key if new_record? @@ -84,8 +88,8 @@ def cache_key_with_version # a recyclable caching scheme. By default, the #updated_at column is used for the # cache_version, but this method can be overwritten to return something else. # - # Note, this method will return nil if ActiveRecord::Base.cache_versioning is set to - # +false+ (which it is by default until Rails 6.0). + # Note, this method will return nil unless .cache_versioning is set to + # +true+ (it defaults to +false+). def cache_version if cache_versioning && (timestamp = try(:updated_at)) timestamp.utc.to_fs(:usec) @@ -97,26 +101,26 @@ module ClassMethods # using +method_name+, which can be any attribute or method that # responds to +to_s+. # - # class User < ActiveRecord::Base + # class User < ActiveRemote::Base # to_param :name # end # # user = User.find_by(name: 'Fancy Pants') - # user.id # => 123 + # user.guid # => "123" # user_path(user) # => "/users/123-fancy-pants" # # Values longer than 20 characters will be truncated. The value # is truncated word by word. # # user = User.find_by(name: 'David Heinemeier Hansson') - # user.id # => 125 + # user.guid # => "125" # user_path(user) # => "/users/125-david-heinemeier" # - # Because the generated param begins with the record's +id+, it is - # suitable for passing to +find+. In a controller, for example: + # The generated param begins with the primary key but is not itself a + # valid argument to +find+, which takes a hash of search args. # - # params[:id] # => "123-fancy-pants" - # User.find(params[:id]).id # => 123 + # params[:id] # => "123-fancy-pants" + # User.find(guid: params[:id].split("-").first) def to_param(method_name = nil) if method_name.nil? super() diff --git a/lib/active_remote/persistence.rb b/lib/active_remote/persistence.rb index b249ba4..fb8136b 100644 --- a/lib/active_remote/persistence.rb +++ b/lib/active_remote/persistence.rb @@ -70,54 +70,40 @@ def readonly? # Deletes the record from the service (the service determines if the # record is hard or soft deleted) and freezes this instance to indicate - # that no changes should be made (since they can't be persisted). If the - # record was not deleted, it will have error messages indicating what went - # wrong. Returns the frozen instance. + # that no changes should be made (since they can't be persisted). Returns + # the frozen instance, or false if the service reported errors. # def delete - raise ReadOnlyRemoteRecord if readonly? - - response = remote_call(:delete, scope_key_hash) - - add_errors(response.errors) if response.respond_to?(:errors) - - success? ? freeze : false + remote_delete(:delete) end # Deletes the record from the service (the service determines if the # record is hard or soft deleted) and freezes this instance to indicate # that no changes should be made (since they can't be persisted). If the - # record was not deleted, an exception will be raised. Returns the frozen - # instance. + # record was not deleted, an ActiveRemoteError is raised. Returns nil. # def delete! delete - raise ActiveRemoteError, errors.to_s if has_errors? + raise ActiveRemoteError, errors.full_messages.to_sentence if has_errors? end # Destroys (hard deletes) the record from the service and freezes this # instance to indicate that no changes should be made (since they can't - # be persisted). If the record was not deleted, it will have error - # messages indicating what went wrong. Returns the frozen instance. + # be persisted). Returns the frozen instance, or false if the service + # reported errors. # def destroy - raise ReadOnlyRemoteRecord if readonly? - - response = remote_call(:destroy, scope_key_hash) - - add_errors(response.errors) if response.respond_to?(:errors) - - success? ? freeze : false + remote_delete(:destroy) end # Destroys (hard deletes) the record from the service and freezes this # instance to indicate that no changes should be made (since they can't - # be persisted). If the record was not deleted, an exception will be - # raised. Returns the frozen instance. + # be persisted). If the record was not destroyed, an ActiveRemoteError is + # raised. Returns nil. # def destroy! destroy - raise ActiveRemoteError, errors.to_s if has_errors? + raise ActiveRemoteError, errors.full_messages.to_sentence if has_errors? end # Returns true if the record has errors; otherwise, returns false. @@ -158,10 +144,10 @@ def readonly? self.class.readonly? || @readonly end - # Executes a remote call on the current object and serializes it's attributes and - # errors from the response. + # Executes a remote call on the current object, adopting the response's + # errors and, when the call succeeded, its attributes. # - # Defaults request args to the scope key hash (e.g., { guid: 'ABC-123' }) when none are given. + # Defaults request args to the scope key hash (e.g., { "guid" => 'ABC-123' }) when none are given. # Returns false if the response contained errors; otherwise, returns true. # def remote(endpoint, request_args = scope_key_hash) @@ -214,8 +200,8 @@ def success? # * Callbacks are invoked. # * Updates all the attributes that are dirty in this object. # - # This method raises an ActiveRemote::ReadOnlyRemoteRecord if the - # attribute is marked as readonly. + # This method raises an ActiveRemote::ReadOnlyRemoteRecord if the record or + # its class is marked as readonly. def update_attribute(name, value) raise ReadOnlyRemoteRecord if readonly? @@ -235,8 +221,9 @@ def update_attributes(attributes) alias_method :update, :update_attributes # Updates the attributes of the remote record from the passed-in hash and - # saves the remote record. If the object is invalid, an - # ActiveRemote::RemoteRecordNotSaved is raised. + # saves the remote record. If the object fails local validation, an + # ActiveRemote::RemoteRecordInvalid is raised; if the service rejects the + # write, an ActiveRemote::RemoteRecordNotSaved is raised. # def update_attributes!(attributes) assign_attributes(attributes) @@ -246,6 +233,21 @@ def update_attributes!(attributes) private + # Shared by #delete and #destroy, which differ only in the endpoint they + # call. Returns the frozen instance, or false if the service reported + # errors. + # + def remote_delete(endpoint) + raise ReadOnlyRemoteRecord if readonly? + + errors.clear + response = remote_call(endpoint, scope_key_hash) + + add_errors(response.errors) if response.respond_to?(:errors) + + success? ? freeze : false + end + # Handles creating a remote object and serializing it's attributes and # errors from the response. # @@ -270,7 +272,8 @@ def create_or_update(*args) # Handles updating a remote object and serializing it's attributes and # errors from the response. Only attributes with the given attribute names - # (plus :guid) will be updated. Defaults to all attributes. + # (plus the scope keys) are sent. The default is every attribute, but + # Dirty#remote_update narrows it to the changed ones. # def remote_update(attribute_names = @attributes.keys) run_callbacks :update do diff --git a/lib/active_remote/primary_key.rb b/lib/active_remote/primary_key.rb index 264a2e3..39e96ef 100644 --- a/lib/active_remote/primary_key.rb +++ b/lib/active_remote/primary_key.rb @@ -2,6 +2,11 @@ module ActiveRemote module PrimaryKey extend ActiveSupport::Concern + included do + # A class_attribute so subclasses inherit a configured primary key. + class_attribute :_primary_key, instance_accessor: false + end + module ClassMethods ## # The default_primary_key is used to define what attribute is used @@ -20,8 +25,8 @@ def default_primary_key # calls to persist or refresh data. # def primary_key(value = nil) - @primary_key = value if value - @primary_key || default_primary_key + self._primary_key = value if value + _primary_key || default_primary_key end end @@ -33,23 +38,18 @@ def primary_key self.class.primary_key end - # Returns an Array of all key attributes if any of the attributes is set, whether or not - # the object is persisted. Returns +nil+ if there are no key attributes. - # - # class Person - # include ActiveModel::Conversion - # attr_accessor :id + # Returns the primary key value wrapped in an Array, whether or not the + # object is persisted. Returns +nil+ when that value is unset. # - # def initialize(id) - # @id = id - # end + # class Person < ActiveRemote::Base + # attribute :guid, :string # end # - # person = Person.new(1) - # person.to_key # => [1] + # Person.new(guid: "ABC-123").to_key # => ["ABC-123"] + # Person.new.to_key # => nil def to_key - @__to_key_key = respond_to?(primary_key) && send(primary_key) if @__to_key_key.nil? - @__to_key_key ? [@__to_key_key] : nil + key = respond_to?(primary_key) && send(primary_key) + key ? [key] : nil end end end diff --git a/lib/active_remote/rpc.rb b/lib/active_remote/rpc.rb index c1dd173..64c423b 100644 --- a/lib/active_remote/rpc.rb +++ b/lib/active_remote/rpc.rb @@ -10,12 +10,14 @@ module RPC end module ClassMethods - # Builds an attribute hash that be assigned directly - # to an object from an RPC response + # Builds an ActiveModel::AttributeSet from an RPC response, ready to be + # handed to #init_with. def build_from_rpc(values) values = values.stringify_keys - attribute_names.each_with_object(_default_attributes.deep_dup) do |name, attributes| + # A shallow dup is enough: every slot is overwritten below, so there is + # nothing left shared with the defaults. + attribute_names.each_with_object(_default_attributes.dup) do |name, attributes| attributes.write_from_database(name, values[name]) end end @@ -52,8 +54,14 @@ def rpc_adapter end def assign_attributes_from_rpc(response) - @attributes = self.class.build_from_rpc(response.to_hash) + errors.clear add_errors(response.errors) if response.respond_to?(:errors) + + # A rejected write echoes back the unchanged record; adopting it would + # revert the caller's edits and leave nothing to retry with. + merge_attributes_from_rpc(response.to_hash) if success? + + success? end def remote_call(rpc_method, request_args) @@ -63,5 +71,18 @@ def remote_call(rpc_method, request_args) def rpc self.class.rpc end + + private + + # Merged rather than rebuilt, since partial-update endpoints echo back only + # the fields they touched. The set is replaced rather than mutated so dirty + # snapshots taken before the call still see the old values. + def merge_attributes_from_rpc(values) + values = values.stringify_keys.slice(*self.class.attribute_names) + + @attributes = @attributes.deep_dup.tap do |attributes| + values.each { |name, value| attributes.write_from_database(name, value) } + end + end end end diff --git a/lib/active_remote/rpc_adapters/protobuf_adapter.rb b/lib/active_remote/rpc_adapters/protobuf_adapter.rb index a0a4b44..4f65100 100644 --- a/lib/active_remote/rpc_adapters/protobuf_adapter.rb +++ b/lib/active_remote/rpc_adapters/protobuf_adapter.rb @@ -67,7 +67,7 @@ def protobuf_error_class(error) ::ActiveRemote::MethodNotFoundError when ::Protobuf::Socketrpc::ErrorReason::RPC_ERROR ::ActiveRemote::RpcError - when ::Protobuf::Socketrpc::ErrorReason::RPC_FAILED_ERROR + when ::Protobuf::Socketrpc::ErrorReason::RPC_FAILED ::ActiveRemote::RpcFailedError when ::Protobuf::Socketrpc::ErrorReason::INVALID_REQUEST_PROTO ::ActiveRemote::InvalidRequestProtoError diff --git a/lib/active_remote/scope_keys.rb b/lib/active_remote/scope_keys.rb index a10665e..1c694b2 100644 --- a/lib/active_remote/scope_keys.rb +++ b/lib/active_remote/scope_keys.rb @@ -38,7 +38,7 @@ def scope_keys # Instance level access to the scope key of the current class # def scope_keys - @scope_keys ||= self.class.scope_keys + self.class.scope_keys end ## @@ -52,8 +52,8 @@ def scope_keys # would return this hash: # # { - # :guid => tag[:guid], - # :user_guid => tag[:user_guid] + # "guid" => tag[:guid], + # "user_guid" => tag[:user_guid] # } # # This hash is used when accessing or modifying a remote object diff --git a/lib/active_remote/search.rb b/lib/active_remote/search.rb index 8df8540..2c8582e 100644 --- a/lib/active_remote/search.rb +++ b/lib/active_remote/search.rb @@ -22,7 +22,7 @@ module ClassMethods # Tag.find(Tag.new(:guid => 'foo')) # # # Protobuf object - # Tag.find(Generic::Remote::TagRequest.new(:guid => 'foo')) + # Tag.find(Generic::Remote::TagRequest.new(:guid => ['foo'])) # def find(args) remote = search(args).first @@ -42,14 +42,15 @@ def find(args) # Tag.find_by(Tag.new(:guid => 'foo')) # # # Protobuf object - # Tag.find_by(Generic::Remote::TagRequest.new(:guid => 'foo')) + # Tag.find_by(Generic::Remote::TagRequest.new(:guid => ['foo'])) # def find_by(args) search(args).first end - # Tries to load the first record; if it fails, then create is called - # with the same arguments. + # Tries to load the first record; if it fails, then create is called with + # the same arguments, with any repeated search field unwrapped to a single + # value. Raises ArgumentError if a field carries more than one value. # # ====Examples # @@ -57,25 +58,25 @@ def find_by(args) # Tag.first_or_create(:name => 'foo') # # # Protobuf object - # Tag.first_or_create(Generic::Remote::TagRequest.new(:name => 'foo')) + # Tag.first_or_create(Generic::Remote::TagRequest.new(:name => ['foo'])) # def first_or_create(attributes) - remote = search(attributes).first - remote ||= create(attributes) - remote + attributes = validate_search_args!(attributes) + search(attributes).first || create(attributes_for_record(attributes)) end # Tries to load the first record; if it fails, then create! is called - # with the same arguments. + # with the same arguments. Unwraps repeated search fields as + # .first_or_create does. # def first_or_create!(attributes) - remote = search(attributes).first - remote ||= create!(attributes) - remote + attributes = validate_search_args!(attributes) + search(attributes).first || create!(attributes_for_record(attributes)) end # Tries to load the first record; if it fails, then a new record is - # initialized with the same arguments. + # initialized with the same arguments. Unwraps repeated search fields as + # .first_or_create does. # # ====Examples # @@ -83,12 +84,11 @@ def first_or_create!(attributes) # Tag.first_or_initialize(:name => 'foo') # # # Protobuf object - # Tag.first_or_initialize(Generic::Remote::TagRequest.new(:name => 'foo')) + # Tag.first_or_initialize(Generic::Remote::TagRequest.new(:name => ['foo'])) # def first_or_initialize(attributes) - remote = search(attributes).first - remote ||= new(attributes) - remote + attributes = validate_search_args!(attributes) + search(attributes).first || new(attributes_for_record(attributes)) end # Searches for records with the given arguments. Returns a collection of @@ -100,7 +100,7 @@ def first_or_initialize(attributes) # Tag.search(:name => 'foo') # # # Protobuf object - # Tag.search(Generic::Remote::TagRequest.new(:name => 'foo')) + # Tag.search(Generic::Remote::TagRequest.new(:name => ['foo'])) # def search(args) args = validate_search_args!(args) @@ -114,19 +114,36 @@ def search(args) end end - # Validates the given args to ensure they are compatible - # Search args must be a hash or respond to to_hash + # Validates the given args to ensure they are compatible. Search args must + # be a Hash, an ActiveRemote::Base, or respond to :to_hash. # def validate_search_args!(args) - unless args.is_a?(Hash) - if args.respond_to?(:to_hash) - args = args.to_hash - else - raise "Invalid parameter: #{args}. Search args must respond to :to_hash." + return args if args.is_a?(Hash) + return args.attributes if args.is_a?(::ActiveRemote::Base) + return args.to_hash if args.respond_to?(:to_hash) + + raise "Invalid parameter: #{args}. Search args must respond to :to_hash." + end + + private + + # Search fields are repeated so callers can match many records at once, + # but the record's attribute is scalar: ["foo"] would cast to "[\"foo\"]". + # + def attributes_for_record(args) + args.to_h do |name, value| + next [name, value] unless value.is_a?(::Array) + + # A type that takes the array unchanged is meant to hold it. + next [name, value] if attribute_types[name.to_s].cast(value) == value + + if value.size > 1 + raise ArgumentError, "Cannot build #{self} from #{name.inspect} => #{value.inspect}. " \ + "#{name} holds a single value, but #{value.size} were given." end - end - args + [name, value.first] + end end end @@ -135,6 +152,7 @@ def validate_search_args!(args) def reload fresh_object = self.class.find(scope_key_hash) @attributes = fresh_object.instance_variable_get(:@attributes) + @new_record = false self end end diff --git a/lib/active_remote/version.rb b/lib/active_remote/version.rb index 079f430..df352d1 100644 --- a/lib/active_remote/version.rb +++ b/lib/active_remote/version.rb @@ -1,3 +1,3 @@ module ActiveRemote - VERSION = "8.0.0" + VERSION = "8.1.0" end diff --git a/spec/lib/active_remote/association_spec.rb b/spec/lib/active_remote/association_spec.rb index 6d7b3a2..d9a3804 100644 --- a/spec/lib/active_remote/association_spec.rb +++ b/spec/lib/active_remote/association_spec.rb @@ -8,7 +8,6 @@ context "simple association" do let(:author_guid) { "AUT-123" } let(:user_guid) { "USR-123" } - let(:default_category_guid) { "CAT-123" } subject { Post.new(author_guid: author_guid, user_guid: user_guid) } it { is_expected.to respond_to(:author) } @@ -37,6 +36,19 @@ expect(Author).to receive(:search).with({guid: subject.author_guid}).and_return([]) expect(subject.author).to be_nil end + + # Truthiness-keyed memoization would treat a nil result as a cache miss. + it "memoizes the miss instead of searching again" do + expect(Author).to receive(:search).once.with({guid: subject.author_guid}).and_return([]) + 3.times { expect(subject.author).to be_nil } + end + end + + it "honors an explicitly assigned nil without searching" do + expect(Author).not_to receive(:search) + subject.author = nil + + expect(subject.author).to be_nil end context "scoped field" do diff --git a/spec/lib/active_remote/attribute_casting_spec.rb b/spec/lib/active_remote/attribute_casting_spec.rb index 3d7343a..8b8484e 100644 --- a/spec/lib/active_remote/attribute_casting_spec.rb +++ b/spec/lib/active_remote/attribute_casting_spec.rb @@ -92,6 +92,13 @@ it "does not list undeclared attributes" do expect(Author.attribute_names).not_to include("bogus") end + + it "reflects attributes declared after the first call (not stale)" do + klass = Class.new(ActiveRemote::Base) { attribute :a, :string } + klass.attribute_names # force a first read to prove the result isn't cached + klass.attribute :b, :string + expect(klass.attribute_names).to include("a", "b") + end end describe "unknown attributes" do diff --git a/spec/lib/active_remote/attribute_defaults_spec.rb b/spec/lib/active_remote/attribute_defaults_spec.rb index 26a515b..3cdfb2d 100644 --- a/spec/lib/active_remote/attribute_defaults_spec.rb +++ b/spec/lib/active_remote/attribute_defaults_spec.rb @@ -27,12 +27,18 @@ # `default: []`) is a single shared object, so mutating it in place leaks # across instances. Use `default: -> { [] }` to get a fresh value per record. # If a future ActiveModel version starts dup-ing literal defaults, this spec - # will flag the behavior change. + # will flag the behavior change. Uses a throwaway class so the mutation can't + # pollute the shared DefaultAuthor default other specs rely on. it "shares a literal mutable default across instances (in-place mutation leaks)" do - DefaultAuthor.new.books << :leaked - expect(DefaultAuthor.new.books).to eq([:leaked]) - ensure - DefaultAuthor.new.books.clear + klass = Class.new(ActiveRemote::Base) { attribute :books, default: [] } + klass.new.books << :leaked + expect(klass.new.books).to eq([:leaked]) + end + + it "gives each instance a fresh value from a proc default (no leak)" do + klass = Class.new(ActiveRemote::Base) { attribute :books, default: -> { [] } } + klass.new.books << :isolated + expect(klass.new.books).to eq([]) end it "does not apply defaults to models that declare none" do diff --git a/spec/lib/active_remote/attribute_methods_spec.rb b/spec/lib/active_remote/attribute_methods_spec.rb index ff00083..4ff0321 100644 --- a/spec/lib/active_remote/attribute_methods_spec.rb +++ b/spec/lib/active_remote/attribute_methods_spec.rb @@ -7,6 +7,13 @@ expect(tag.attribute_for_inspect("guid")).to eq("derp".inspect) end + # Every other example here passes a string, which is what hid this: a symbol + # used to miss the attribute entirely and report "nil". + it "accepts a symbol name, as #[] and #[]= do" do + tag = Tag.new(guid: "derp") + expect(tag.attribute_for_inspect(:guid)).to eq("derp".inspect) + end + context "when the attribute is a string longer than 50 characters" do it "returns the inspect-like string for the attribute" do tag = Tag.new(name: "The lazy yellow dog was caught by the slow red fox as he lay sleeping in the sun") @@ -57,6 +64,10 @@ expect(tag["bogus"]).to be_nil end + it "raises when writing an unknown attribute through []=" do + expect { tag["bogus"] = 1 }.to raise_error(ActiveModel::MissingAttributeError) + end + context "with an aliased attribute" do let(:model) do Class.new(ActiveRemote::Base) do diff --git a/spec/lib/active_remote/base_spec.rb b/spec/lib/active_remote/base_spec.rb index 45901b8..e1de984 100644 --- a/spec/lib/active_remote/base_spec.rb +++ b/spec/lib/active_remote/base_spec.rb @@ -8,6 +8,29 @@ end end + # #eql? is aliased to #==, so #hash has to agree. + describe "#hash" do + it "matches for two records with the same class and primary key" do + expect(Tag.new(guid: "1").hash).to eq(Tag.new(guid: "1").hash) + end + + it "lets equal records collapse in a Set, #uniq and as Hash keys" do + a = Tag.new(guid: "1") + b = Tag.new(guid: "1") + + expect([a, b].uniq.size).to eq(1) + expect({a => :found}[b]).to eq(:found) + end + + it "differs for the same key on another class" do + expect(Tag.new(guid: "1").hash).not_to eq(Author.new(guid: "1").hash) + end + + it "falls back to identity for new records, matching #==" do + expect([Tag.new, Tag.new].uniq.size).to eq(2) + end + end + describe "#== and #eql?" do let(:tag) { Tag.new(guid: "1") } @@ -66,6 +89,17 @@ tag = Tag.new(guid: "1").freeze expect { tag.name = "nope" }.to raise_error(FrozenError) end + + # An RPC response swaps out @attributes, which used to silently thaw the record. + it "freezes the object itself, not just the attribute set" do + tag = Tag.new(guid: "1").freeze + + expect(Object.instance_method(:frozen?).bind_call(tag)).to be(true) + end + + it "does not report an allocated-but-uninitialized record as frozen" do + expect(Tag.allocate).not_to be_frozen + end end describe "#init_with" do diff --git a/spec/lib/active_remote/dirty_spec.rb b/spec/lib/active_remote/dirty_spec.rb index 945c151..be0bfad 100644 --- a/spec/lib/active_remote/dirty_spec.rb +++ b/spec/lib/active_remote/dirty_spec.rb @@ -59,29 +59,32 @@ end end - describe "#save" do + describe "#instantiate" do + let(:post) { Post.new(name: "foo") } + + # #instantiate swaps @attributes, orphaning the mutation tracker unless reset. + it "clears changes information" do + expect { post.instantiate("name" => "bar") }.to change { post.changed? }.to(false) + end + end + + # Stub at the RPC boundary: stubbing `create_or_update` or `save` skips + # `#remote`, which is what replaces @attributes with the service response. + describe "#save and #save!" do subject(:post) { Post.new(name: "foo") } before do - allow(post).to receive(:create_or_update).and_return(true) + allow(post).to receive(:remote_call).and_return(::Generic::Remote::Post.new(name: "foo")) end - it "applies changes" do + it "applies changes via #save" do changes = post.changes post.save expect(post.previous_changes).to eq(changes) expect(post.changes).to be_empty end - end - - describe "#save!" do - subject(:post) { Post.new(name: "foo") } - - before do - allow(post).to receive(:save).and_return(true) - end - it "applies changes" do + it "applies changes via #save!" do changes = post.changes post.save! expect(post.previous_changes).to eq(changes) @@ -89,8 +92,7 @@ end end - # Dirty tracking compares CAST values, not the raw assigned value. This is only - # observable on typed attributes, so the string-only specs above don't cover it. + # Dirty tracking compares cast values, which only typed attributes reveal. describe "cast-aware change tracking" do subject(:author) { Author.new(age: 42, writes_fiction: true) } diff --git a/spec/lib/active_remote/dsl_spec.rb b/spec/lib/active_remote/dsl_spec.rb index 8d71458..72c876e 100644 --- a/spec/lib/active_remote/dsl_spec.rb +++ b/spec/lib/active_remote/dsl_spec.rb @@ -4,17 +4,6 @@ before { reset_dsl_variables(Tag) } after { Tag.service_class Generic::Remote::TagService } - describe ".attr_publishable" do - after { reset_publishable_attributes(Tag) } - - it "appends given attributes to @publishable_attributes" do - Tag.attr_publishable :guid - Tag.attr_publishable :name - - expect(Tag.publishable_attributes).to match_array([:guid, :name]) - end - end - describe ".endpoints" do it "has default values" do expect(Tag.endpoints).to eq( diff --git a/spec/lib/active_remote/integration_spec.rb b/spec/lib/active_remote/integration_spec.rb index d8d5829..c8c1006 100644 --- a/spec/lib/active_remote/integration_spec.rb +++ b/spec/lib/active_remote/integration_spec.rb @@ -36,7 +36,8 @@ twenty_o_one_one = tag.updated_at = DateTime.new(2001, 0o1, 0o1) expect(tag).to receive(:new_record?).and_return(false) expect(tag.cache_key).to eq("tags/#{guid}-#{twenty_o_one_one.to_fs(:usec)}") - tag.updated_at = nil + ensure + # Without this the flag leaks globally whenever the example above fails. ::ActiveRemote.config.default_cache_key_updated_at = false end diff --git a/spec/lib/active_remote/persistence_spec.rb b/spec/lib/active_remote/persistence_spec.rb index 33a4eae..8cad4a9 100644 --- a/spec/lib/active_remote/persistence_spec.rb +++ b/spec/lib/active_remote/persistence_spec.rb @@ -72,6 +72,30 @@ expect(subject.delete).to be_falsey end end + + # Only #save cleared errors, so a stale one made a good delete report failure. + context "when the record already carries errors from an earlier call" do + before { subject.errors.add(:name, "left over") } + + it "still succeeds and freezes the record" do + expect(subject.delete).to be_truthy + expect(subject).to be_frozen + end + + it "clears the stale errors" do + subject.delete + expect(subject.has_errors?).to be(false) + end + end + + context "when the record is readonly" do + subject { Tag.instantiate({guid: "123"}, readonly: true) } + + it "raises before issuing the RPC" do + expect(rpc).not_to receive(:execute) + expect { subject.delete }.to raise_error(ActiveRemote::ReadOnlyRemoteRecord) + end + end end describe "#delete!" do @@ -89,6 +113,11 @@ it "raises an exception" do expect { subject.delete! }.to raise_error(ActiveRemote::ActiveRemoteError) end + + # errors.to_s yields "#", which says nothing. + it "reports the service error messages" do + expect { subject.delete! }.to raise_error(ActiveRemote::ActiveRemoteError, "Name Boom!") + end end end @@ -120,6 +149,24 @@ expect(subject.destroy).to be_falsey end end + + context "when the record already carries errors from an earlier call" do + before { subject.errors.add(:name, "left over") } + + it "still succeeds and freezes the record" do + expect(subject.destroy).to be_truthy + expect(subject).to be_frozen + end + end + + context "when the record is readonly" do + subject { Tag.instantiate({guid: "123"}, readonly: true) } + + it "raises before issuing the RPC" do + expect(rpc).not_to receive(:execute) + expect { subject.destroy }.to raise_error(ActiveRemote::ReadOnlyRemoteRecord) + end + end end describe "#destroy!" do @@ -137,6 +184,10 @@ it "raises an exception" do expect { subject.destroy! }.to raise_error(ActiveRemote::ActiveRemoteError) end + + it "reports the service error messages" do + expect { subject.destroy! }.to raise_error(ActiveRemote::ActiveRemoteError, "Name Boom!") + end end end @@ -295,6 +346,62 @@ end end + # Partial-update endpoints echo back only the fields they touched. + context "when the response omits attributes the service didn't touch" do + subject { Tag.instantiate({"guid" => "123", "name" => "old", "user_guid" => "u-1"}) } + + before do + allow(rpc).to receive(:execute).and_return(Generic::Remote::Tag.new(guid: "123", name: "new")) + subject.name = "new" + end + + it "keeps the attributes that were not returned" do + expect { subject.save }.not_to change { subject.user_guid }.from("u-1") + end + + it "still adopts the values the service did return" do + subject.save + + expect(subject.name).to eq("new") + end + end + + # A rejected write echoes the unchanged record; adopting it reverts the edit. + context "when the service rejects the save" do + let(:error) { Generic::Error.new(field: "name", message: "is taken") } + + subject { Tag.instantiate({"guid" => "123", "name" => "old"}) } + + before do + allow(rpc).to receive(:execute).and_return(Generic::Remote::Tag.new(guid: "123", name: "old", errors: [error])) + subject.name = "new" + end + + it "returns false and records the error" do + expect(subject.save).to be(false) + expect(subject.errors.full_messages).to eq(["Name is taken"]) + end + + it "preserves the rejected edit" do + subject.save + + expect(subject.name).to eq("new") + end + + it "keeps the change tracked so a retry still sends it" do + subject.save + + expect(subject.changes).to eq("name" => ["old", "new"]) + end + + it "sends the pending change on the retry" do + subject.save + + expect(rpc).to receive(:execute).with(:update, {"name" => "new", "guid" => "123"}) + subject.save + end + end + context "when only some attributes change after a cast-equal assignment" do let(:author_rpc) { ::ActiveRemote::RPCAdapters::ProtobufAdapter.new(::Author.service_class, ::Author.endpoints) } @@ -395,7 +502,6 @@ end before { allow(subject).to receive(:save) } - after { allow(subject).to receive(:save).and_call_original } it "assigns new attributes" do expect(subject).to receive(:name=).with("foo") @@ -406,6 +512,18 @@ expect(subject).to receive(:save) subject.update_attribute(:name, "foo") end + + context "when the record is readonly" do + subject { Tag.instantiate({guid: "123"}, readonly: true) } + + # Must fire before assignment, or the guard in #create_or_update passes it. + it "raises without assigning or saving" do + expect(subject).not_to receive(:save) + + expect { subject.update_attribute(:name, "foo") }.to raise_error(ActiveRemote::ReadOnlyRemoteRecord) + expect(subject.name).to be_nil + end + end end describe "#update_attributes" do diff --git a/spec/lib/active_remote/primary_key_spec.rb b/spec/lib/active_remote/primary_key_spec.rb index 92d9b60..118a257 100644 --- a/spec/lib/active_remote/primary_key_spec.rb +++ b/spec/lib/active_remote/primary_key_spec.rb @@ -1,9 +1,7 @@ require "spec_helper" RSpec.describe ActiveRemote::PrimaryKey do - let(:tag) { Tag.new(id: "1234", guid: "TAG-123", user_guid: "USR-123") } - - after { Tag.instance_variable_set :@primary_key, nil } + after { Tag._primary_key = nil } describe ".default_primary_key" do it "returns array of :guid" do @@ -25,6 +23,26 @@ expect(Tag.primary_key(specified_primary_key)).to eq(specified_primary_key) end end + + context "when a superclass declares one" do + let(:parent) do + Class.new(ActiveRemote::Base) do + primary_key :uuid + attribute :uuid, :string + end + end + + it "is inherited by subclasses" do + expect(Class.new(parent).primary_key).to eq(:uuid) + end + + it "can still be overridden by the subclass" do + child = Class.new(parent) { primary_key :other } + + expect(child.primary_key).to eq(:other) + expect(parent.primary_key).to eq(:uuid) + end + end end describe "#primary_key" do @@ -45,5 +63,13 @@ expect(Tag.new(guid: "TAG-123").to_key).to eq ["TAG-123"] end end + + it "reflects a reassigned primary key rather than memoizing the first read" do + tag = Tag.new(guid: "TAG-123") + tag.to_key + tag.guid = "TAG-456" + + expect(tag.to_key).to eq ["TAG-456"] + end end end diff --git a/spec/lib/active_remote/query_attribute_spec.rb b/spec/lib/active_remote/query_attribute_spec.rb index 37c1d93..ec6af18 100644 --- a/spec/lib/active_remote/query_attribute_spec.rb +++ b/spec/lib/active_remote/query_attribute_spec.rb @@ -24,6 +24,12 @@ expect(subject.name?).to eq false end + it "is false when the attribute is an empty collection" do + author = DefaultAuthor.new + author.books = [] + expect(author.books?).to eq false + end + it "is true when the attribute is a non-empty string" do subject.name = "Chris" expect(subject.name?).to eq true diff --git a/spec/lib/active_remote/rpc_adapters/protobuf_adapter_spec.rb b/spec/lib/active_remote/rpc_adapters/protobuf_adapter_spec.rb index ef75f85..1d6fa14 100644 --- a/spec/lib/active_remote/rpc_adapters/protobuf_adapter_spec.rb +++ b/spec/lib/active_remote/rpc_adapters/protobuf_adapter_spec.rb @@ -21,4 +21,48 @@ end end end + + # A typo in any of these constants raises NameError from the failure callback + # instead, so callers rescuing the ActiveRemote error never see it. + describe "service failures" do + error_class_for_reason = { + BAD_REQUEST_DATA: ActiveRemote::BadRequestDataError, + BAD_REQUEST_PROTO: ActiveRemote::BadRequestProtoError, + SERVICE_NOT_FOUND: ActiveRemote::ServiceNotFoundError, + METHOD_NOT_FOUND: ActiveRemote::MethodNotFoundError, + RPC_ERROR: ActiveRemote::RpcError, + RPC_FAILED: ActiveRemote::RpcFailedError, + INVALID_REQUEST_PROTO: ActiveRemote::InvalidRequestProtoError, + BAD_RESPONSE_PROTO: ActiveRemote::BadResponseProtoError, + UNKNOWN_HOST: ActiveRemote::UnknownHostError, + IO_ERROR: ActiveRemote::IOError + } + + # Release the adapter-level double so mock_rpc's stub on the service class + # is reached through the client delegation. + before { allow(adapter).to receive(:client).and_call_original } + + error_class_for_reason.each do |reason, error_class| + it "raises #{error_class} for #{reason}" do + error = Struct.new(:error_type, :message).new( + ::Protobuf::Socketrpc::ErrorReason.const_get(reason), "boom" + ) + mock_rpc(Tag.service_class, :search, error: error) + + expect { adapter.execute(:search, guid: "1") }.to raise_error(error_class, "boom") + end + end + + it "falls back to ActiveRemoteError for an unrecognized reason" do + mock_rpc(Tag.service_class, :search, error: Struct.new(:error_type, :message).new(9999, "boom")) + + expect { adapter.execute(:search, guid: "1") }.to raise_error(ActiveRemote::ActiveRemoteError, "boom") + end + + it "falls back to ActiveRemoteError when the error has no type" do + mock_rpc(Tag.service_class, :search, error: Struct.new(:message).new("boom")) + + expect { adapter.execute(:search, guid: "1") }.to raise_error(ActiveRemote::ActiveRemoteError, "boom") + end + end end diff --git a/spec/lib/active_remote/scope_keys_spec.rb b/spec/lib/active_remote/scope_keys_spec.rb index 8cc2fa0..011e52f 100644 --- a/spec/lib/active_remote/scope_keys_spec.rb +++ b/spec/lib/active_remote/scope_keys_spec.rb @@ -28,6 +28,18 @@ it "returns the scope keys for the class" do expect(Tag.new.scope_keys).to eq Tag.scope_keys end + + it "picks up a scope key declared after the first read" do + model = Class.new(ActiveRemote::Base) do + attribute :guid, :string + attribute :user_guid, :string + end + record = model.new + record.scope_keys + model.scope_key :user_guid + + expect(record.scope_keys).to eq(["guid", "user_guid"]) + end end describe "#scope_key_hash" do diff --git a/spec/lib/active_remote/search_spec.rb b/spec/lib/active_remote/search_spec.rb index 98193ac..02af611 100644 --- a/spec/lib/active_remote/search_spec.rb +++ b/spec/lib/active_remote/search_spec.rb @@ -3,7 +3,6 @@ RSpec.describe ActiveRemote::Search do let(:records) { [Generic::Remote::Tag.new(guid: "123")] } let(:response) { Generic::Remote::Tags.new(records: records) } - let(:rpc) { double(:rpc) } describe ".find" do let(:args) { {} } @@ -65,13 +64,15 @@ describe ".search" do let(:serialized_records) { [Tag.instantiate(guid: "123")] } + let(:rpc) { ::ActiveRemote::RPCAdapters::ProtobufAdapter.new(::Tag.service_class, ::Tag.endpoints) } + + before do + allow(rpc).to receive(:execute).and_return(response) + allow(::Tag).to receive(:rpc).and_return(rpc) + end context "given args that respond to :to_hash" do let(:args) { {} } - let(:rpc) { ::ActiveRemote::RPCAdapters::ProtobufAdapter.new(::Tag.service_class, ::Tag.endpoints) } - - before { allow(rpc).to receive(:execute).and_return(response) } - before { allow(::Tag).to receive(:rpc).and_return(rpc) } it "searches with the given args" do expect(Tag.rpc).to receive(:execute).with(:search, args) @@ -85,12 +86,102 @@ end context "given args that don't respond to :to_hash" do - let(:request) { double(:request) } + let(:request) { Object.new } it "raises an exception" do expect { Tag.search(request) }.to raise_error(::RuntimeError, /Invalid parameter/) end end + + # The docs advertise a protobuf request or Active Remote object, not just a hash. + context "given a protobuf request object" do + it "normalizes it to a hash" do + expect(Tag.rpc).to receive(:execute).with(:search, {guid: ["123"]}) + Tag.search(::Generic::Remote::TagRequest.new(guid: ["123"])) + end + end + + context "given an Active Remote object" do + it "searches with its attributes" do + expect(Tag.rpc).to receive(:execute).with(:search, hash_including("guid" => "123")) + Tag.search(Tag.new(guid: "123")) + end + end + end + + # These take .search's argument forms but hand them to create/new. + describe "first_or_* argument handling" do + let(:empty_response) { Generic::Remote::Tags.new(records: []) } + let(:created) { Generic::Remote::Tag.new(guid: "NEW", name: "foo") } + let(:rpc) { ::ActiveRemote::RPCAdapters::ProtobufAdapter.new(::Tag.service_class, ::Tag.endpoints) } + let(:requests) { [] } + + before do + allow(::Tag).to receive(:rpc).and_return(rpc) + allow(rpc).to receive(:execute) do |endpoint, args| + requests << [endpoint, args] + (endpoint == :search) ? empty_response : created + end + end + + it "creates from a protobuf request when nothing is found" do + tag = Tag.first_or_create(::Generic::Remote::TagRequest.new(name: ["foo"])) + + expect(tag.guid).to eq("NEW") + end + + it "creates! from a protobuf request when nothing is found" do + tag = Tag.first_or_create!(::Generic::Remote::TagRequest.new(name: ["foo"])) + + expect(tag.guid).to eq("NEW") + end + + it "initializes from a protobuf request when nothing is found" do + tag = Tag.first_or_initialize(::Generic::Remote::TagRequest.new(guid: ["G"])) + + expect(tag).to be_new_record + end + + it "returns the found record without creating" do + allow(rpc).to receive(:execute).and_return(Generic::Remote::Tags.new(records: [Generic::Remote::Tag.new(guid: "OLD")])) + + expect(Tag.first_or_create(name: "foo").guid).to eq("OLD") + end + + # Search fields are repeated, but the matching attribute is scalar. + context "when a search field is repeated but the attribute is not" do + it "unwraps the value before initializing" do + tag = Tag.first_or_initialize(::Generic::Remote::TagRequest.new(name: ["foo"])) + + expect(tag.name).to eq("foo") + end + + it "still searches with the repeated value" do + Tag.first_or_initialize(::Generic::Remote::TagRequest.new(name: ["foo"])) + + expect(requests).to include([:search, {name: ["foo"]}]) + end + + it "unwraps the value before creating" do + Tag.first_or_create(::Generic::Remote::TagRequest.new(name: ["foo"])) + + expect(requests).to include([:create, hash_including("name" => "foo")]) + end + + it "refuses to build a record when the search matched on several values" do + expect { + Tag.first_or_initialize(::Generic::Remote::TagRequest.new(name: %w[foo bar])) + }.to raise_error(ArgumentError, /holds a single value, but 2 were given/) + end + + it "leaves an attribute that accepts the array alone" do + allow(::DefaultAuthor).to receive(:rpc).and_return(rpc) + + author = DefaultAuthor.first_or_initialize(books: %w[foo bar]) + + expect(author.books).to eq(%w[foo bar]) + end + end end describe "#reload" do @@ -110,5 +201,10 @@ subject.reload expect(subject.attributes).to eq attributes end + + # A reloaded record isn't new; leaving the flag set duplicates it on save. + it "marks the record as persisted" do + expect { subject.reload }.to change { subject.new_record? }.from(true).to(false) + end end end diff --git a/spec/lib/active_remote/serializers/protobuf_spec.rb b/spec/lib/active_remote/serializers/protobuf_spec.rb index e5fa227..b6a81a6 100644 --- a/spec/lib/active_remote/serializers/protobuf_spec.rb +++ b/spec/lib/active_remote/serializers/protobuf_spec.rb @@ -11,6 +11,33 @@ end end +RSpec.describe ActiveRemote::Serializers::Protobuf do + # The serializer falls back to the :value type for any field it can't map + # (e.g. enums), which relies on ActiveRemote registering :value with + # ActiveModel::Type. See active_remote/base.rb. + describe "the :value type fallback" do + let(:enum_field) { Serializer.get_field(:enum_field, true) } + + # be_a would also pass for any subclass of Type::Value. + it "registers the :value type with ActiveModel::Type" do + expect(ActiveModel::Type.lookup(:value)).to be_an_instance_of(ActiveModel::Type::Value) + end + + it "returns nil for an unmapped field type" do + expect(described_class.type_name_for_field(enum_field)).to be_nil + end + + it "returns the mapped type name for a known field type" do + expect(described_class.type_name_for_field(Serializer.get_field(:int32_field, true))).to eq(:integer) + end + + it "casts an unmapped field through the :value type (a no-op passthrough)" do + value = {custom: "object"} + expect(ActiveRemote::Serializers::Protobuf::Field.from_attribute(enum_field, value)).to eq(value) + end + end +end + RSpec.describe ActiveRemote::Serializers::Protobuf::Field do describe ".from_attribute" do context "when field is a repeated message" do diff --git a/spec/support/helpers.rb b/spec/support/helpers.rb index cbedcf7..2f3c8db 100644 --- a/spec/support/helpers.rb +++ b/spec/support/helpers.rb @@ -13,7 +13,6 @@ def reset_dsl_variables(klass) reset_app_name(klass) reset_auto_paging_size(klass) reset_namespace(klass) - reset_publishable_attributes(klass) reset_service_class(klass) reset_service_name(klass) end @@ -30,10 +29,6 @@ def reset_namespace(klass) klass.send(:instance_variable_set, :@namespace, nil) end -def reset_publishable_attributes(klass) - klass.send(:instance_variable_set, :@publishable_attributes, nil) -end - def reset_service_class(klass) klass.send(:instance_variable_set, :@service_class, nil) end