From 1214218aac37ca371a247683ec0e9bc16c994444 Mon Sep 17 00:00:00 2001 From: Konstantin Shabanov Date: Wed, 3 Jun 2015 13:04:25 +0700 Subject: [PATCH 1/3] Use accessors instead of methods with memoization Get rid of confusing behaviour when `unexpose` could leak to subclasses which have not locked down their attributes with `.exposures` call yet. --- CHANGELOG.md | 1 + lib/grape_entity/entity.rb | 88 ++++++++------------------------ spec/grape_entity/entity_spec.rb | 15 +----- 3 files changed, 25 insertions(+), 79 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c55f6c4..c6e06808 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ * [#114](https://github.com/intridea/grape-entity/pull/114): Added 'only' option that selects which attributes should be returned - [@estevaoam](https://github.com/estevaoam). * [#115](https://github.com/intridea/grape-entity/pull/115): Allowing 'root' to be inherited from parent to child entities - [@guidoprincess](https://github.com/guidoprincess). * [#121](https://github.com/intridea/grape-entity/pull/122): Sublcassed Entity#documentation properly handles unexposed params - [@dan-corneanu](https://github.com/dan-corneanu). +* [#134](https://github.com/intridea/grape-entity/pull/134): Subclasses no longer affected in all cases by `unexpose` in parent - [@etehtsea](https://github.com/etehtsea). * Your contribution here. 0.4.5 (2015-03-10) diff --git a/lib/grape_entity/entity.rb b/lib/grape_entity/entity.rb index d5980319..745dac04 100644 --- a/lib/grape_entity/entity.rb +++ b/lib/grape_entity/entity.rb @@ -98,6 +98,26 @@ def entity(options = {}) end end + class << self + # Returns exposures that have been declared for this Entity or + # ancestors. The keys are symbolized references to methods on the + # containing object, the values are the options that were passed into expose. + # @return [Hash] of exposures + attr_accessor :exposures + # Returns all formatters that are registered for this and it's ancestors + # @return [Hash] of formatters + attr_accessor :formatters + attr_accessor :nested_attribute_names + attr_accessor :nested_exposures + end + + def self.inherited(subclass) + subclass.exposures = exposures.try(:dup) || {} + subclass.nested_exposures = nested_exposures.try(:dup) || {} + subclass.nested_attribute_names = nested_attribute_names.try(:dup) || {} + subclass.formatters = formatters.try(:dup) || {} + end + # This method is the primary means by which you will declare what attributes # should be exposed by the entity. # @@ -142,10 +162,9 @@ def self.expose(*args, &block) unless @nested_attributes.empty? orig_attribute = attribute.to_sym attribute = "#{@nested_attributes.last}__#{attribute}" - nested_attribute_names_hash[attribute.to_sym] = orig_attribute + nested_attribute_names[attribute.to_sym] = orig_attribute options[:nested] = true - nested_exposures_hash[@nested_attributes.last.to_sym] ||= {} - nested_exposures_hash[@nested_attributes.last.to_sym][attribute.to_sym] = options + nested_exposures.deep_merge!(@nested_attributes.last.to_sym => { attribute.to_sym => options }) end exposures[attribute.to_sym] = options @@ -178,58 +197,6 @@ def self.with_options(options) @block_options.pop end - # Returns a hash of exposures that have been declared for this Entity or ancestors. The keys - # are symbolized references to methods on the containing object, the values are - # the options that were passed into expose. - def self.exposures - return @exposures unless @exposures.nil? - - @exposures = {} - - if superclass.respond_to? :exposures - @exposures = superclass.exposures.merge(@exposures) - end - - @exposures - end - - class << self - attr_accessor :_nested_attribute_names_hash - attr_accessor :_nested_exposures_hash - - def nested_attribute_names_hash - self._nested_attribute_names_hash ||= {} - end - - def nested_exposures_hash - self._nested_exposures_hash ||= {} - end - - def nested_attribute_names - return @nested_attribute_names unless @nested_attribute_names.nil? - - @nested_attribute_names = {}.merge(nested_attribute_names_hash) - - if superclass.respond_to? :nested_attribute_names - @nested_attribute_names = superclass.nested_attribute_names.deep_merge(@nested_attribute_names) - end - - @nested_attribute_names - end - - def nested_exposures - return @nested_exposures unless @nested_exposures.nil? - - @nested_exposures = {}.merge(nested_exposures_hash) - - if superclass.respond_to? :nested_exposures - @nested_exposures = superclass.nested_exposures.deep_merge(@nested_exposures) - end - - @nested_exposures - end - end - # Returns a hash, the keys are symbolized references to fields in the entity, # the values are document keys in the entity's documentation key. When calling # #docmentation, any exposure without a documentation key will be ignored. @@ -272,17 +239,6 @@ def self.format_with(name, &block) formatters[name.to_sym] = block end - # Returns a hash of all formatters that are registered for this and it's ancestors. - def self.formatters - @formatters ||= {} - - if superclass.respond_to? :formatters - @formatters = superclass.formatters.merge(@formatters) - end - - @formatters - end - # This allows you to set a root element name for your representation. # # @param plural [String] the root key to use when representing diff --git a/spec/grape_entity/entity_spec.rb b/spec/grape_entity/entity_spec.rb index 40f2f64d..85758ca0 100644 --- a/spec/grape_entity/entity_spec.rb +++ b/spec/grape_entity/entity_spec.rb @@ -296,23 +296,12 @@ class Parent < Person expect(subject.exposures).to eq(name: {}, email: {}) end - # the following 2 behaviors are testing because it is not most intuitive and could be confusing - context 'when called from the parent class' do - it 'remove from parent and all child classes that have not locked down their attributes with an .exposures call' do + context 'when called from the parent class' do + it 'remove from parent and do not remove from child classes' do subject.expose :name, :email child_class = Class.new(subject) subject.unexpose :email - expect(subject.exposures).to eq(name: {}) - expect(child_class.exposures).to eq(name: {}) - end - - it 'remove from parent and do not remove from child classes that have locked down their attributes with an .exposures call' do - subject.expose :name, :email - child_class = Class.new(subject) - child_class.exposures - subject.unexpose :email - expect(subject.exposures).to eq(name: {}) expect(child_class.exposures).to eq(name: {}, email: {}) end From ee316f444ec6741919322442e157cbd3b4ba8a6c Mon Sep 17 00:00:00 2001 From: Konstantin Shabanov Date: Wed, 3 Jun 2015 19:55:14 +0700 Subject: [PATCH 2/3] Remove `to_sym` calls duplication --- lib/grape_entity/entity.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/grape_entity/entity.rb b/lib/grape_entity/entity.rb index 745dac04..88fe8dae 100644 --- a/lib/grape_entity/entity.rb +++ b/lib/grape_entity/entity.rb @@ -161,13 +161,13 @@ def self.expose(*args, &block) args.each do |attribute| unless @nested_attributes.empty? orig_attribute = attribute.to_sym - attribute = "#{@nested_attributes.last}__#{attribute}" - nested_attribute_names[attribute.to_sym] = orig_attribute + attribute = "#{@nested_attributes.last}__#{attribute}".to_sym + nested_attribute_names[attribute] = orig_attribute options[:nested] = true - nested_exposures.deep_merge!(@nested_attributes.last.to_sym => { attribute.to_sym => options }) + nested_exposures.deep_merge!(@nested_attributes.last.to_sym => { attribute => options }) end - exposures[attribute.to_sym] = options + exposures[attribute] = options # Nested exposures are given in a block with no parameters. if block_given? && block.parameters.empty? From a55cf24d6d3008074eedbc528ceeebc5c72dccf5 Mon Sep 17 00:00:00 2001 From: Konstantin Shabanov Date: Wed, 3 Jun 2015 21:03:24 +0700 Subject: [PATCH 3/3] Replace `!(.empty? || .nil?)` with `.present?` --- lib/grape_entity/entity.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/grape_entity/entity.rb b/lib/grape_entity/entity.rb index 88fe8dae..fcca503f 100644 --- a/lib/grape_entity/entity.rb +++ b/lib/grape_entity/entity.rb @@ -202,7 +202,7 @@ def self.with_options(options) # #docmentation, any exposure without a documentation key will be ignored. def self.documentation @documentation ||= exposures.each_with_object({}) do |(attribute, exposure_options), memo| - unless exposure_options[:documentation].nil? || exposure_options[:documentation].empty? + if exposure_options[:documentation].present? memo[key_for(attribute)] = exposure_options[:documentation] end end