Remove Grape::Util::InheritableValues, resolving inheritable settings by walking parent

.rubocop.yml

@@ -52,7 +52,7 @@ Metrics/BlockLength:
     - spec/**/*_spec.rb
 
 Metrics/ClassLength:
-  Max: 405
+  Max: 425
 
 Metrics/CyclomaticComplexity:
   Max: 15

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

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`).
 

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