From 04ab8f1d2d34ee15a97542bc8afa215e4e392679 Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Sun, 13 Jul 2025 15:58:21 +0200 Subject: [PATCH] Refactor AttributesDoc with ParamsDocumentation module. Add `except_values` in doc --- CHANGELOG.md | 1 + lib/grape/validations/attributes_doc.rb | 60 ------- lib/grape/validations/params_documentation.rb | 52 ++++++ lib/grape/validations/params_scope.rb | 32 ++-- spec/grape/validations/attributes_doc_spec.rb | 156 ------------------ .../validations/params_documentation_spec.rb | 144 ++++++++++++++++ 6 files changed, 209 insertions(+), 236 deletions(-) delete mode 100644 lib/grape/validations/attributes_doc.rb create mode 100644 lib/grape/validations/params_documentation.rb delete mode 100644 spec/grape/validations/attributes_doc_spec.rb create mode 100644 spec/grape/validations/params_documentation_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 25132a6c9..dfe5dd1e6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ * [#2575](https://github.com/ruby-grape/grape/pull/2575): Refactor Api description class - [@ericproulx](https://github.com/ericproulx). * [#2577](https://github.com/ruby-grape/grape/pull/2577): Deprecate `return` in endpoint execution - [@ericproulx](https://github.com/ericproulx). * [#13](https://github.com/ericproulx/grape/pull/13): Refactor endpoint helpers and error middleware integration - [@ericproulx](https://github.com/ericproulx). +* [#2583](https://github.com/ruby-grape/grape/pull/2583): Optimize api parameter documentation and memory usage - [@ericproulx](https://github.com/ericproulx). * Your contribution here. #### Fixes diff --git a/lib/grape/validations/attributes_doc.rb b/lib/grape/validations/attributes_doc.rb deleted file mode 100644 index c0d5ed954..000000000 --- a/lib/grape/validations/attributes_doc.rb +++ /dev/null @@ -1,60 +0,0 @@ -# frozen_string_literal: true - -module Grape - module Validations - # Documents parameters of an endpoint. If documentation isn't needed (for instance, it is an - # internal API), the class only cleans up attributes to avoid junk in RAM. - - class AttributesDoc - attr_accessor :type, :values - - # @param api [Grape::API::Instance] - # @param scope [Validations::ParamsScope] - def initialize(api, scope) - @api = api - @scope = scope - @type = type - end - - def extract_details(validations) - details[:required] = validations.key?(:presence) - - desc = validations.delete(:desc) || validations.delete(:description) - - details[:desc] = desc if desc - - documentation = validations.delete(:documentation) - - details[:documentation] = documentation if documentation - - details[:default] = validations[:default] if validations.key?(:default) - - details[:min_length] = validations[:length][:min] if validations.key?(:length) && validations[:length].key?(:min) - details[:max_length] = validations[:length][:max] if validations.key?(:length) && validations[:length].key?(:max) - end - - def document(attrs) - return if @api.namespace_inheritable(:do_not_document) - - details[:type] = type.to_s if type - details[:values] = values if values - - documented_attrs = attrs.each_with_object({}) do |name, memo| - memo[@scope.full_name(name)] = details - end - - @api.namespace_stackable(:params, documented_attrs) - end - - def required - details[:required] - end - - protected - - def details - @details ||= {} - end - end - end -end diff --git a/lib/grape/validations/params_documentation.rb b/lib/grape/validations/params_documentation.rb new file mode 100644 index 000000000..742ee8637 --- /dev/null +++ b/lib/grape/validations/params_documentation.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +module Grape + module Validations + # Documents parameters of an endpoint. If documentation isn't needed (for instance, it is an + # internal API), the class only cleans up attributes to avoid junk in RAM. + + module ParamsDocumentation + def document_params(attrs, validations, type = nil, values = nil, except_values = nil) + if @api.namespace_inheritable(:do_not_document) + validations.except!(:desc, :description, :documentation) + else + documented_attrs = attrs.each_with_object({}) do |name, memo| + memo[full_name(name)] = extract_details(validations, type, values, except_values) + end + @api.namespace_stackable(:params, documented_attrs) + end + end + + private + + def extract_details(validations, type, values, except_values) + {}.tap do |details| + details[:required] = validations.key?(:presence) + details[:type] = TypeCache[type] if type + details[:values] = values if values + details[:except_values] = except_values if except_values + details[:default] = validations[:default] if validations.key?(:default) + if validations.key?(:length) + details[:min_length] = validations[:length][:min] if validations[:length].key?(:min) + details[:max_length] = validations[:length][:max] if validations[:length].key?(:max) + end + + desc = validations.delete(:desc) || validations.delete(:description) + details[:desc] = desc if desc + + documentation = validations.delete(:documentation) + details[:documentation] = documentation if documentation + end + end + + class TypeCache < Grape::Util::Cache + def initialize + super + @cache = Hash.new do |h, type| + h[type] = type.to_s + end + end + end + end + end +end diff --git a/lib/grape/validations/params_scope.rb b/lib/grape/validations/params_scope.rb index cb536edba..651b2312d 100644 --- a/lib/grape/validations/params_scope.rb +++ b/lib/grape/validations/params_scope.rb @@ -7,6 +7,7 @@ class ParamsScope attr_reader :type, :params_meeting_dependency include Grape::DSL::Parameters + include Grape::Validations::ParamsDocumentation # There are a number of documentation options on entities that don't have # corresponding validators. Since there is nowhere that enumerates them all, @@ -323,23 +324,14 @@ def configure_declared_params end def validates(attrs, validations) - doc = AttributesDoc.new @api, self - doc.extract_details validations - coerce_type = infer_coercion(validations) - - doc.type = coerce_type - + required = validations.key?(:presence) default = validations[:default] values = validations[:values].is_a?(Hash) ? validations.dig(:values, :value) : validations[:values] - - doc.values = values - except_values = validations[:except_values].is_a?(Hash) ? validations.dig(:except_values, :value) : validations[:except_values] # NB. values and excepts should be nil, Proc, Array, or Range. # Specifically, values should NOT be a Hash - # use values or excepts to guess coerce type when stated type is Array coerce_type = guess_coerce_type(coerce_type, values, except_values) @@ -349,23 +341,23 @@ def validates(attrs, validations) # type should be compatible with values array, if both exist validate_value_coercion(coerce_type, values, except_values) - doc.document attrs + document_params attrs, validations, coerce_type, values, except_values opts = derive_validator_options(validations) # Validate for presence before any other validators - validates_presence(validations, attrs, doc, opts) + validates_presence(validations, attrs, opts) # Before we run the rest of the validators, let's handle # whatever coercion so that we are working with correctly # type casted values - coerce_type validations, attrs, doc, opts + coerce_type validations, attrs, required, opts validations.each do |type, options| # Don't try to look up validators for documentation params that don't have one. next if RESERVED_DOCUMENTATION_KEYWORDS.include?(type) - validate(type, options, attrs, doc, opts) + validate(type, options, attrs, required, opts) end end @@ -429,7 +421,7 @@ def check_coerce_with(validations) # composited from more than one +requires+/+optional+ # parameter, and needs to be run before most other # validations. - def coerce_type(validations, attrs, doc, opts) + def coerce_type(validations, attrs, required, opts) check_coerce_with(validations) return unless validations.key?(:coerce) @@ -439,7 +431,7 @@ def coerce_type(validations, attrs, doc, opts) method: validations[:coerce_with], message: validations[:coerce_message] } - validate('coerce', coerce_options, attrs, doc, opts) + validate('coerce', coerce_options, attrs, required, opts) validations.delete(:coerce_with) validations.delete(:coerce) validations.delete(:coerce_message) @@ -465,11 +457,11 @@ def check_incompatible_option_values(default, values, except_values) raise Grape::Exceptions::IncompatibleOptionValues.new(:default, default, :except, except_values) end - def validate(type, options, attrs, doc, opts) + def validate(type, options, attrs, required, opts) validator_options = { attributes: attrs, options: options, - required: doc.required, + required: required, params_scope: self, opts: opts, validator_class: Validations.require_validator(type) @@ -516,10 +508,10 @@ def derive_validator_options(validations) } end - def validates_presence(validations, attrs, doc, opts) + def validates_presence(validations, attrs, opts) return unless validations.key?(:presence) && validations[:presence] - validate('presence', validations.delete(:presence), attrs, doc, opts) + validate('presence', validations.delete(:presence), attrs, true, opts) validations.delete(:message) if validations.key?(:message) end end diff --git a/spec/grape/validations/attributes_doc_spec.rb b/spec/grape/validations/attributes_doc_spec.rb deleted file mode 100644 index d5ae81d2f..000000000 --- a/spec/grape/validations/attributes_doc_spec.rb +++ /dev/null @@ -1,156 +0,0 @@ -# frozen_string_literal: true - -describe Grape::Validations::AttributesDoc do - shared_examples 'an optional doc attribute' do |attr| - it 'does not mention it' do - expected_opts.delete(attr) - validations.delete(attr) - - expect(subject.first['nested[engine_age]']).not_to have_key(attr) - end - end - - let(:api) { Class.new(Grape::API::Instance) } - let(:scope) do - params = nil - api_instance = api - - # just to get nested params - Grape::Validations::ParamsScope.new(type: Hash, api: api) do - params = Grape::Validations::ParamsScope.new(element: 'nested', - type: Hash, - api: api_instance, - parent: self) - end - - params - end - - let(:validations) do - { - presence: true, - desc: 'Age of...', - documentation: 'Age is...', - default: 1, - length: { min: 1, max: 13 } - } - end - - let(:doc) { described_class.new(api, scope) } - - describe '#extract_details' do - subject { doc.extract_details(validations) } - - it 'cleans up doc attrs needed for documentation only' do - subject - - expect(validations[:desc]).to be_nil - expect(validations[:documentation]).to be_nil - end - - it 'does not clean up doc attrs mandatory for validators' do - subject - - expect(validations[:presence]).not_to be_nil - expect(validations[:default]).not_to be_nil - end - - it 'tells when attributes are required' do - subject - - expect(doc.required).to be_truthy - end - end - - describe '#document' do - subject do - doc.extract_details validations - doc.document attrs - end - - let(:attrs) { %w[engine_age car_age] } - let(:valid_values) { [1, 3, 5, 8] } - - let!(:expected_opts) do - { - required: true, - desc: validations[:desc], - documentation: validations[:documentation], - default: validations[:default], - type: 'Integer', - values: valid_values, - min_length: validations[:length][:min], - max_length: validations[:length][:max] - } - end - - before do - doc.type = Integer - doc.values = valid_values - end - - context 'documentation is enabled' do - subject do - super() - api.namespace_stackable(:params) - end - - it 'documents attributes' do - expect(subject.first).to eq('nested[engine_age]' => expected_opts, - 'nested[car_age]' => expected_opts) - end - - it_behaves_like 'an optional doc attribute', :default - it_behaves_like 'an optional doc attribute', :documentation - it_behaves_like 'an optional doc attribute', :desc - it_behaves_like 'an optional doc attribute', :type do - before { doc.type = nil } - end - it_behaves_like 'an optional doc attribute', :values do - before { doc.values = nil } - end - - context 'false as a default value' do - before { validations[:default] = false } - - it 'is still documented' do - doc = subject.first['nested[engine_age]'] - - expect(doc).to have_key(:default) - expect(doc[:default]).to be(false) - end - end - - context 'nil as a default value' do - before { validations[:default] = nil } - - it 'is still documented' do - doc = subject.first['nested[engine_age]'] - - expect(doc).to have_key(:default) - expect(doc[:default]).to be_nil - end - end - - context 'the description key instead of desc' do - let!(:desc) { validations.delete(:desc) } - - before { validations[:description] = desc } - - it 'adds the given description' do - expect(subject.first['nested[engine_age]'][:desc]).to eq(desc) - end - end - end - - context 'documentation is disabled' do - before { api.namespace_inheritable :do_not_document, true } - - it 'does not document attributes' do - subject - - expect(api.namespace_stackable(:params)).to eq([]) - end - end - end -end diff --git a/spec/grape/validations/params_documentation_spec.rb b/spec/grape/validations/params_documentation_spec.rb new file mode 100644 index 000000000..ad157eb08 --- /dev/null +++ b/spec/grape/validations/params_documentation_spec.rb @@ -0,0 +1,144 @@ +# frozen_string_literal: true + +describe Grape::Validations::ParamsDocumentation do + subject { klass.new(api_double) } + + let(:api_double) do + Class.new do + def initialize + @namespace_stackable = {} + @namespace_inheritable = {} + end + + def namespace_stackable(key, value = nil) + if value.nil? + @namespace_stackable[key] + else + @namespace_stackable[key] = value + end + end + + def namespace_inheritable(key, value = nil) + if value.nil? + @namespace_inheritable[key] + else + @namespace_inheritable[key] = value + end + end + end.new + end + + let(:klass) do + Class.new do + include Grape::Validations::ParamsDocumentation + attr_accessor :api + + def initialize(api) + @api = api + end + + def full_name(name) + "full_name_#{name}" + end + end + end + + describe '#document_params' do + it 'stores documented params with all details' do + attrs = %i[foo bar] + validations = { + presence: true, + default: 42, + length: { min: 1, max: 10 }, + desc: 'A foo', + documentation: { note: 'doc' } + } + type = Integer + values = [1, 2, 3] + except_values = [4, 5, 6] + subject.document_params(attrs, validations.dup, type, values, except_values) + expect(api_double.namespace_stackable(:params).keys).to include('full_name_foo', 'full_name_bar') + expect(api_double.namespace_stackable(:params)['full_name_foo']).to include( + required: true, + type: 'Integer', + values: [1, 2, 3], + except_values: [4, 5, 6], + default: 42, + min_length: 1, + max_length: 10, + desc: 'A foo', + documentation: { note: 'doc' } + ) + end + + context 'when do_not_document is set' do + let(:validations) do + { desc: 'desc', description: 'description', documentation: { foo: 'bar' }, another_param: 'test' } + end + + before do + allow(api_double).to receive(:namespace_inheritable).with(:do_not_document).and_return(true) + end + + it 'removes desc, description, and documentation' do + subject.document_params([:foo], validations) + expect(validations).to eq({ another_param: 'test' }) + end + end + + context 'when validation is empty' do + let(:validations) do + {} + end + + it 'does not raise an error' do + expect { subject.document_params([:foo], validations) }.not_to raise_error + expect(api_double.namespace_stackable(:params)['full_name_foo']).to eq({ required: false }) + end + end + + context 'when desc is not present' do + let(:validations) do + { description: 'desc2' } + end + + it 'uses description if desc is not present' do + subject.document_params([:foo], validations) + expect(api_double.namespace_stackable(:params)['full_name_foo'][:desc]).to eq('desc2') + end + end + + context 'when desc nor description is present' do + let(:validations) do + {} + end + + it 'uses description if desc is not present' do + subject.document_params([:foo], validations) + expect(api_double.namespace_stackable(:params)['full_name_foo']).to eq({ required: false }) + end + end + + context 'when documentation is not present' do + let(:validations) do + {} + end + + it 'does not include documentation' do + subject.document_params([:foo], validations) + expect(api_double.namespace_stackable(:params)['full_name_foo']).not_to have_key(:documentation) + end + end + + context 'when type is nil' do + let(:validations) do + { presence: true } + end + + it 'sets type as nil' do + subject.document_params([:foo], validations) + expect(api_double.namespace_stackable(:params)['full_name_foo'][:type]).to be_nil + end + end + end +end