diff --git a/.rubocop.yml b/.rubocop.yml index d2a6f8b73..9dd8b6a8b 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -52,7 +52,7 @@ Metrics/BlockLength: - spec/**/*_spec.rb Metrics/ClassLength: - Max: 405 + Max: 425 Metrics/CyclomaticComplexity: Max: 15 diff --git a/CHANGELOG.md b/CHANGELOG.md index ff76b8d70..8eb3e1473 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,7 @@ * [#2823](https://github.com/ruby-grape/grape/pull/2823): Move stackable settings storage onto `Grape::Util::InheritableSetting`, resolving inheritance by walking `parent` instead of a second chain; `Grape::Util::StackableValues` becomes a read-only view kept for grape-swagger, and `Grape::Util::BaseInheritable` is removed - [@ericproulx](https://github.com/ericproulx). * [#2828](https://github.com/ruby-grape/grape/pull/2828): Compare `Grape::Util::InheritableSetting` by own state instead of serializing both chains through `to_hash`, and give `Grape::Util::InheritableValues` an `==`, so the duplicate-route check in `route` stops costing O(n²) `to_hash` calls on APIs whose namespaces share relative paths - [@ericproulx](https://github.com/ericproulx). * [#2831](https://github.com/ruby-grape/grape/pull/2831): Document and test open (beginless/endless) `Range`s as a `values:`/`except_values:` alternative to a dedicated numericality validator - [@dblock](https://github.com/dblock). +* [#2830](https://github.com/ruby-grape/grape/pull/2830): Remove `Grape::Util::InheritableValues`, resolving inheritable settings by walking `Grape::Util::InheritableSetting#parent` instead of layering a second chain of stores, and keep namespace settings in a plain Hash - [@ericproulx](https://github.com/ericproulx). * Your contribution here. #### Fixes diff --git a/UPGRADING.md b/UPGRADING.md index 837ecfdb6..dacb7e55b 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -14,6 +14,11 @@ Two consequences: * **A middle version is now served.** `mount v1; mount v2; mount v3` alongside a catch-all `route :any, '*path'` answered `406 API version not found` for v2, while v1 and v3 worked; v2 is now served. * **An unmatched version reaches a catch-all.** When a catch-all ANY route is present, a request whose version matches nothing now falls through to it instead of surfacing the versioner's `406`, which is what `cascade: true` (the default) asks for. Declare `version ..., cascade: false` to keep the hard 406 — that path is unchanged and never consults the catch-all. +#### `Grape::Util::InheritableValues` has been removed + +Inheritable settings no longer layer a scope's values over a store handed down from the enclosing scope. Each `Grape::Util::InheritableSetting` now keeps only what its own scope assigned, and resolves a lookup by walking `#parent` — the same move `Grape::Util::StackableValues` got in [#2823](https://github.com/ruby-grape/grape/pull/2823), and the reason `Grape::Util::InheritableValues` no longer has anything to do. + +Resolution is unchanged in every observable way: the nearest scope wins, a scope may override an inherited value with an explicit `nil`, and a value an enclosing scope gains later is still visible through scopes already nested inside it. Only the class is gone. Nothing in Grape referenced it outside `InheritableSetting`; code that constructed one directly should keep a plain Hash per scope and resolve against the parent chain. #### `forward_match` is no longer exposed on routes @@ -132,7 +137,7 @@ One micro-change: the `middleware` DSL reader dropped its `|| []` fallback — ` The `Grape::Namespace` objects registered by the `namespace` DSL (and its `group` / `resource` / `resources` / `segment` aliases) and the mount path recorded by `mount` are now written and read through dedicated accessors on `Grape::Util::InheritableSetting` instead of raw `namespace_stackable` keys, following the same move made for rescue handlers: -* `namespaces` / `add_namespace(namespace)` replace `namespace_stackable[:namespace]` (not to be confused with the pre-existing `namespace` reader, which returns the `InheritableValues` store). +* `namespaces` / `add_namespace(namespace)` replace `namespace_stackable[:namespace]` (not to be confused with the pre-existing `namespace` reader, which returns the namespace-settings store). * `namespace_path` returns the normalized joined path prefix, absorbing the `Grape::Namespace.joined_space_path(...)` call previously spelled out at the read sites, and `namespace_requirements` returns the requirements declared by registered namespaces, absorbing the `filter_map(&:requirements)`. * `mount_path` / `add_mount_path(path)` replace `namespace_stackable[:mount_path]`; the reader absorbs the outermost-wins convention (previously the `.first` at the read site in `Endpoint#build_stack`). diff --git a/lib/grape/util/inheritable_setting.rb b/lib/grape/util/inheritable_setting.rb index d0bf0a8ef..b611c9c74 100644 --- a/lib/grape/util/inheritable_setting.rb +++ b/lib/grape/util/inheritable_setting.rb @@ -8,12 +8,15 @@ module Util # first), plain +=+ writers are nearest-wins scalar overrides, and # +!+/+?+ pairs are scope flags. Deep-merged readers return nil when # nothing is registered; plain stack readers return a frozen empty - # Array. The backing stores (InheritableValues and the per-scope Hashes) - # and their keys are internal. + # Array. The backing stores — a per-scope Hash per kind of state, each + # holding only what that scope itself set — and their keys are internal. # # Settings instances form a chain: a scope inherits its parent's values # (see #inherit_from), and endpoints snapshot the chain with - # #point_in_time_copy_for_endpoint. + # #point_in_time_copy_for_endpoint. Nothing is copied down the chain when + # a scope is created; every reader resolves against #parent on demand, so + # a value an enclosing scope gains later is visible through scopes already + # nested inside it. class InheritableSetting # Maps the callbacks DSL method names to their pluralized # namespace-stackable storage keys (see #callbacks / #add_callback). @@ -57,9 +60,16 @@ def self.reset_global! # #inherit_from). def initialize @route = {} - @namespace = InheritableValues.new # only inheritable from a parent when - # used with a mount, or should every API::Class be a separate namespace by default? - @namespace_inheritable = InheritableValues.new + # Namespace settings are scope-local by design: nothing ever layers + # them over a parent's (see #inherit_from, and the nesting behaviour + # DSL::Settings#namespace_setting is specified to have), so a plain + # Hash is the whole store. + @namespace = {} + # This scope's own inheritable overrides. Like @stackable_values it + # stays nil until the first write, and inheritance is resolved by + # walking #parent (see #inheritable) rather than by keeping a second + # chain of stores alongside it. + @namespace_inheritable = nil # This scope's own stackable registrations, one Array per key. Stays # nil until the first registration so scopes that only inherit don't # each carry an empty Hash. @@ -73,16 +83,15 @@ def global self.class.global end - # Set our inherited values to the given parent's current values. Also, - # update the inherited values on any settings instances which were forked - # from us. + # Inherit from the given parent: its values resolve behind ours from now + # on, including any it gains later. Also re-parents any settings + # instances which were forked from us. # @param parent [InheritableSetting] def inherit_from(parent) return if parent.nil? @parent = parent - @namespace_inheritable.inherited_values = parent.namespace_inheritable @route = parent.route.merge(route) @point_in_time_copies&.each { |cloned_one| cloned_one.inherit_from parent } @@ -195,8 +204,8 @@ def to_hash { global: global.clone, route: route.clone, - namespace: namespace.to_hash, - namespace_inheritable: @namespace_inheritable.to_hash, + namespace: namespace.dup, + namespace_inheritable: inheritable_values, namespace_stackable: stacked_keys.to_h { |key| [key, stacked(key)] }, rescue_handlers:, base_only_rescue_handlers: @@ -315,38 +324,38 @@ def add_rescue_options(options) # through #rescue_all? / #rescue_grape_exceptions?; the backing store # is an internal detail. def add_all_rescue_handler(handler) - @namespace_inheritable[:rescue_all] = true - @namespace_inheritable[:all_rescue_handler] = handler + set_inheritable(:rescue_all, true) + set_inheritable(:all_rescue_handler, handler) end def add_grape_exceptions_rescue_handler(handler) - @namespace_inheritable[:rescue_all] = true - @namespace_inheritable[:rescue_grape_exceptions] = true - @namespace_inheritable[:grape_exceptions_rescue_handler] = handler + set_inheritable(:rescue_all, true) + set_inheritable(:rescue_grape_exceptions, true) + set_inheritable(:grape_exceptions_rescue_handler, handler) end def add_internal_grape_exceptions_rescue_handler(handler) - @namespace_inheritable[:internal_grape_exceptions_rescue_handler] = handler + set_inheritable(:internal_grape_exceptions_rescue_handler, handler) end def rescue_all? - @namespace_inheritable[:rescue_all] == true + inheritable(:rescue_all) == true end def rescue_grape_exceptions? - @namespace_inheritable[:rescue_grape_exceptions] == true + inheritable(:rescue_grape_exceptions) == true end def all_rescue_handler - @namespace_inheritable[:all_rescue_handler] + inheritable(:all_rescue_handler) end def grape_exceptions_rescue_handler - @namespace_inheritable[:grape_exceptions_rescue_handler] + inheritable(:grape_exceptions_rescue_handler) end def internal_grape_exceptions_rescue_handler - @namespace_inheritable[:internal_grape_exceptions_rescue_handler] + inheritable(:internal_grape_exceptions_rescue_handler) end # Rescue-handler maps registered by +rescue_from+, keyed by exception @@ -512,35 +521,35 @@ def add_contract_key_map(key_map) # when never set (Endpoint applies the request-serving fallbacks); the # backing store is an internal detail. def format - @namespace_inheritable[:format] + inheritable(:format) end def format=(format) - @namespace_inheritable[:format] = format + set_inheritable(:format, format) end def default_format - @namespace_inheritable[:default_format] + inheritable(:default_format) end def default_format=(default_format) - @namespace_inheritable[:default_format] = default_format + set_inheritable(:default_format, default_format) end def default_error_formatter - @namespace_inheritable[:default_error_formatter] + inheritable(:default_error_formatter) end def default_error_formatter=(formatter) - @namespace_inheritable[:default_error_formatter] = formatter + set_inheritable(:default_error_formatter, formatter) end def default_error_status - @namespace_inheritable[:default_error_status] + inheritable(:default_error_status) end def default_error_status=(status) - @namespace_inheritable[:default_error_status] = status + set_inheritable(:default_error_status, status) end # Versioning state recorded by the routing DSL (see DSL::Routing): @@ -550,27 +559,27 @@ def default_error_status=(status) # Nearest-wins scalars with plain += writers; readers return nil when # never set; the backing store is an internal detail. def version - @namespace_inheritable[:version] + inheritable(:version) end def version=(versions) - @namespace_inheritable[:version] = versions + set_inheritable(:version, versions) end def version_options - @namespace_inheritable[:version_options] + inheritable(:version_options) end def version_options=(options) - @namespace_inheritable[:version_options] = options + set_inheritable(:version_options, options) end def root_prefix - @namespace_inheritable[:root_prefix] + inheritable(:root_prefix) end def root_prefix=(prefix) - @namespace_inheritable[:root_prefix] = prefix + set_inheritable(:root_prefix, prefix) end # Cascade flag assigned by the +cascade+ DSL. An explicit nil is @@ -579,15 +588,15 @@ def root_prefix=(prefix) # assigned it — Grape::API::Instance#cascade? falls back to the # version options' cascade, then to true, when it was never assigned. def cascade - @namespace_inheritable[:cascade] + inheritable(:cascade) end def cascade=(value) - @namespace_inheritable[:cascade] = value + set_inheritable(:cascade, value) end def cascade_defined? - @namespace_inheritable.key?(:cascade) + inheritable?(:cascade) end # Scope flags flipped by the routing DSL's bang methods (see @@ -596,35 +605,35 @@ def cascade_defined? # and everything nested under it. Readers return false when never set; # the backing store is an internal detail. def do_not_route_head! - @namespace_inheritable[:do_not_route_head] = true + set_inheritable(:do_not_route_head, true) end def do_not_route_head? - @namespace_inheritable[:do_not_route_head] == true + inheritable(:do_not_route_head) == true end def do_not_route_options! - @namespace_inheritable[:do_not_route_options] = true + set_inheritable(:do_not_route_options, true) end def do_not_route_options? - @namespace_inheritable[:do_not_route_options] == true + inheritable(:do_not_route_options) == true end def do_not_document! - @namespace_inheritable[:do_not_document] = true + set_inheritable(:do_not_document, true) end def do_not_document? - @namespace_inheritable[:do_not_document] == true + inheritable(:do_not_document) == true end def lint! - @namespace_inheritable[:lint] = true + set_inheritable(:lint, true) end def lint? - @namespace_inheritable[:lint] == true + inheritable(:lint) == true end # The params-builder strategy set by +build_with+ (both the @@ -633,11 +642,11 @@ def lint? # builds its Grape::Request. Nearest-wins scalar; nil when never set; # the backing store is an internal detail. def build_params_with - @namespace_inheritable[:build_params_with] + inheritable(:build_params_with) end def build_params_with=(strategy) - @namespace_inheritable[:build_params_with] = strategy + set_inheritable(:build_params_with, strategy) end # The authentication configuration Hash recorded by the +auth+ DSL @@ -646,11 +655,11 @@ def build_params_with=(strategy) # to warn about unauthenticated bare Rack mounts; the backing store is # an internal detail. def auth - @namespace_inheritable[:auth] + inheritable(:auth) end def auth=(auth_options) - @namespace_inheritable[:auth] = auth_options + set_inheritable(:auth, auth_options) end # Immutable snapshot of the settings Router::Pattern::Path reads to @@ -680,10 +689,40 @@ def path_settings protected - # Peer access to the inheritable store for #inherit_from and - # #copy_state_from; internal code reads the ivar directly. + # This scope's own inheritable overrides, before inheritance; nil when + # the scope overrode nothing. Peer access for #copy_state_from. attr_reader :namespace_inheritable + # The nearest scope's value for +key+: this scope's own override when it + # has one, otherwise the enclosing scope's. Keyed on presence rather than + # truthiness, so a scope can deliberately override an inherited value + # with nil (see #cascade). + def inheritable(key) + return @namespace_inheritable[key] if @namespace_inheritable&.key?(key) + + parent&.inheritable(key) + end + + # Whether any scope along the chain assigned +key+ — including one that + # assigned nil, which #inheritable cannot distinguish from never-set. + def inheritable?(key) + return true if @namespace_inheritable&.key?(key) + + parent&.inheritable?(key) || false + end + + # Every inheritable value along the chain resolved into one Hash, nearest + # scope winning. Like #stacked it hands back the backing Hash when only + # one scope in the chain has values, so callers must treat the result as + # read-only. + def inheritable_values + inherited = parent&.inheritable_values + own = @namespace_inheritable + return own || {} unless inherited + + own ? inherited.merge(own) : inherited + end + # This scope's own +rescue_from+ registrations, before inheritance: # {rescue_handlers: {klass => handler}, base_only_rescue_handlers: {...}}. attr_reader :rescue_handler_maps @@ -727,8 +766,8 @@ def merged_rescue_handlers(key) # Used by +point_in_time_copy+ to populate a freshly-built instance # with cloned state from another instance of the same class. def copy_state_from(source) - @namespace = source.namespace.clone - @namespace_inheritable = source.namespace_inheritable.clone + @namespace = source.namespace.dup + @namespace_inheritable = source.namespace_inheritable&.dup # Shallow, matching the store this replaced: the per-key Arrays stay # shared with the source, so a registration made on the source after # the copy was taken is still visible through it. @@ -739,6 +778,13 @@ def copy_state_from(source) private + # Overrides +key+ for this scope, leaving the enclosing scopes' value + # untouched. The store is allocated on first use. Returns the assigned + # value, since the writers built on it are +=+ methods. + def set_inheritable(key, value) + (@namespace_inheritable ||= {})[key] = value + end + # Appends one registration for +key+ to this scope, leaving inherited # ones untouched. The store is allocated on first use. Returns the # registered value, since this replaced an assignment expression and the diff --git a/lib/grape/util/inheritable_values.rb b/lib/grape/util/inheritable_values.rb deleted file mode 100644 index 0cf3c0b5d..000000000 --- a/lib/grape/util/inheritable_values.rb +++ /dev/null @@ -1,90 +0,0 @@ -# frozen_string_literal: true - -module Grape - module Util - # A settings scope's own values layered over the enclosing scope's, the - # nearest scope winning on read. Used for the nearest-wins scalar settings - # behind Grape::Util::InheritableSetting; stacking registrations are kept - # by the setting itself. - # - # +@new_values+ is lazily allocated on first write so settings layers - # that only inherit (never override) don't carry an empty Hash each. - class InheritableValues - attr_accessor :inherited_values, :new_values - - # @param inherited_values [Object] An object implementing an interface - # of the Hash class. - def initialize(inherited_values = nil) - @inherited_values = inherited_values || {} - # @new_values stays nil until the first write. - end - - def [](name) - return @inherited_values[name] unless @new_values - - @new_values.fetch(name) { @inherited_values[name] } - end - - def []=(name, value) - (@new_values ||= {})[name] = value - end - - def delete(*keys) - return [] unless @new_values - - keys.map { |key| @new_values.delete(key) } - end - - def key?(name) - @inherited_values.key?(name) || @new_values&.key?(name) || false - end - - def merge(new_hash) - values.merge!(new_hash) - end - - def to_hash - values - end - - # Two layers over the same inherited store resolve identically when their - # own overrides match, which equality checks first so the common case - # never merges either side down. Since +@new_values+ is allocated lazily, - # nil and an empty Hash both mean "this layer overrides nothing". Layers - # that fail that check can still resolve identically — an override may - # merely restate an inherited value — so they fall back to #to_hash. - def ==(other) - return true if equal?(other) - return false unless other.is_a?(self.class) - return true if inherited_values == other.inherited_values && same_overrides?(other) - - to_hash == other.to_hash - end - alias eql? == - - def initialize_copy(other) - super - @inherited_values = other.inherited_values - @new_values = other.new_values&.dup - end - - protected - - def values - return @inherited_values.merge(@new_values) if @new_values && !@new_values.empty? - - @inherited_values.is_a?(Hash) ? @inherited_values.dup : @inherited_values.to_hash - end - - private - - # Whether both layers override the same keys with the same values, with - # nil and an empty Hash treated alike (see #==). - def same_overrides?(other) - return other.new_values.blank? if new_values.blank? - - new_values == other.new_values - end - end - end -end diff --git a/spec/grape/util/inheritable_setting_spec.rb b/spec/grape/util/inheritable_setting_spec.rb index 877e634f1..d94c4804d 100644 --- a/spec/grape/util/inheritable_setting_spec.rb +++ b/spec/grape/util/inheritable_setting_spec.rb @@ -134,6 +134,48 @@ end end + describe 'inheritable value resolution' do + it 'shadows an inherited value with an explicitly assigned nil' do + parent.cascade = true + subject.cascade = nil + + expect(subject.cascade).to be_nil + expect(parent.cascade).to be(true) + end + + it 'reports through the predicate that an enclosing scope assigned a value' do + expect(subject.cascade_defined?).to be(false) + + parent.cascade = false + + expect(subject.cascade_defined?).to be(true) + expect(subject.cascade).to be(false) + end + + it 'resolves a value the parent gains after the child was created' do + expect(subject.auth).to be_nil + + parent.auth = { type: :http_basic } + + expect(subject.auth).to eq(type: :http_basic) + end + + it 'leaves the parent untouched when a nested scope overrides' do + subject.root_prefix = :child_prefix + + expect(parent.root_prefix).to eq :namespace_inheritable_foo_bar + end + + # See bug #891: entity classes and the like are shared with a copy, never + # duplicated. + it 'shares complex values with a point-in-time copy rather than duplicating them' do + options = { entity: Class.new } + subject.version_options = options + + expect(subject.point_in_time_copy.version_options).to be(options) + end + end + describe '#namespace_stackable' do it 'works with stackable values' do expect(subject.helpers).to eq [:namespace_stackable_foo_bar] diff --git a/spec/grape/util/inheritable_values_spec.rb b/spec/grape/util/inheritable_values_spec.rb deleted file mode 100644 index 51ed46e17..000000000 --- a/spec/grape/util/inheritable_values_spec.rb +++ /dev/null @@ -1,108 +0,0 @@ -# frozen_string_literal: true - -describe Grape::Util::InheritableValues do - subject { described_class.new(parent) } - - let(:parent) { described_class.new } - - describe '#delete' do - it 'deletes a key' do - subject[:some_thing] = :new_foo_bar - subject.delete :some_thing - expect(subject[:some_thing]).to be_nil - end - - it 'does not delete parent values' do - parent[:some_thing] = :foo - subject[:some_thing] = :new_foo_bar - subject.delete :some_thing - expect(subject[:some_thing]).to eq :foo - end - end - - describe '#[]' do - it 'returns a value' do - subject[:some_thing] = :foo - expect(subject[:some_thing]).to eq :foo - end - - it 'returns parent value when no value is set' do - parent[:some_thing] = :foo - expect(subject[:some_thing]).to eq :foo - end - - it 'overwrites parent value with the current one' do - parent[:some_thing] = :foo - subject[:some_thing] = :foo_bar - expect(subject[:some_thing]).to eq :foo_bar - end - - it 'parent values are not changed' do - parent[:some_thing] = :foo - subject[:some_thing] = :foo_bar - expect(parent[:some_thing]).to eq :foo - end - end - - describe '#[]=' do - it 'sets a value' do - subject[:some_thing] = :foo - expect(subject[:some_thing]).to eq :foo - end - end - - describe '#to_hash' do - it 'returns a Hash representation' do - parent[:some_thing] = :foo - subject[:some_thing_more] = :foo_bar - expect(subject.to_hash).to eq(some_thing: :foo, some_thing_more: :foo_bar) - end - end - - describe '#==' do - it 'is true for two layers over the same parent with the same overrides' do - subject[:some_thing] = :foo - expect(subject).to eq(described_class.new(parent).tap { |v| v[:some_thing] = :foo }) - end - - it 'is false when the overrides differ' do - subject[:some_thing] = :foo - expect(subject).not_to eq(described_class.new(parent).tap { |v| v[:some_thing] = :bar }) - end - - it 'is false when the inherited values differ' do - parent[:some_thing] = :foo - expect(subject).not_to eq(described_class.new(described_class.new)) - end - - it 'does not distinguish a never-written layer from one emptied by #delete' do - subject[:some_thing] = :foo - subject.delete :some_thing - expect(subject).to eq(described_class.new(parent)) - end - - it 'is true when an override only restates an inherited value' do - parent[:some_thing] = :foo - subject[:some_thing] = :foo - expect(subject).to eq(described_class.new(parent)) - end - - it 'is false for a non-InheritableValues' do - expect(subject).not_to eq(subject.to_hash) - end - end - - describe '#clone' do - let(:obj_cloned) { subject.clone } - - context 'complex (i.e. not primitive) data types (ex. entity classes, please see bug #891)' do - let(:description) { { entity: double } } - - before { subject[:description] = description } - - it 'copies values; does not duplicate them' do - expect(obj_cloned[:description]).to eq description - end - end - end -end