From fe7afaaca7a25fe0490905fe18cedc0d867203de Mon Sep 17 00:00:00 2001 From: Murray Steele Date: Tue, 16 Jul 2024 14:13:20 +0100 Subject: [PATCH] Fix error for hash object warnings with delegated method descriptions (#938) * Use `method_name` instead of `method` when generating hash warnings In https://github.com/Apipie/apipie-rails/pull/865 we introduced a `method_name` method on `MethodDescription` to avoid this bug, but the commit didn't actually use that method. Sometimes the `@controller_method` object used is a `Apipie::Generator::Swagger::MethodDescription::Decorator` which is a `SimpleDelegate` onto a `Apipie::MethodDescription` and we'd expect to be able to call `method` on it, but unfortunately `method` is one of the things _not_ delegated by `SimpleDelegate` because it's a standard ruby method and so you get ArgumentError: wrong number of arguments (given 0, expected 1) when you try to call it. Using `method_name` instead avoids that so that's what we do - and now we can happily generate the swagger warnings when we have hash type objects without defined params. * Use `method_name` instead of `method` when generating required warnings Unlike the previous commit, this isn't strictly required as we're not calling the un-delegated `method`, but instead the warning will contain a standard `to_s` of the `@controller_method` that isn't very easy to read, like this: WARNING (105): [#] -- The parameter :param is optional but default value is not specified (use :default_value => ...) By using `method_name` instead we get symmetry with the hash warning from the previous commit. * Fix rubocop-rspec by removing a redundant `let` Only allowed 15 `let` or `subject`s for a given spec, and we now have 16. Removing the `with_null` memoization lets us proceed. In theory it would have allowed us to run the specs with_null set to both true and false, but in practice, we weren't, and the other memoized values _were_ useful for customising the specs. * Handle warnings for param descriptions without a controller method If we're generating swagger via `SwaggerGenerator.json_schema_for_self_describing_class` we explicitly don't have a `controller_method` being passed around so we can't use it for the warnings. Introduce a test for type and builder to cover this scenario (first spotted by a failing spec for `SwaggerGenerator`) and then change the implementation to cope with it. Luckily, `Apipie::Generator::Swagger::MethodDescription::Decorator` already had a `ruby_name` implementation that handles being given `nil`, so we use that. --- .../swagger/param_description/builder.rb | 6 ++-- .../swagger/param_description/type.rb | 6 +++- .../swagger/param_description/builder_spec.rb | 30 ++++++++++++++++++ .../swagger/param_description/type_spec.rb | 31 +++++++++++++++++-- 4 files changed, 67 insertions(+), 6 deletions(-) diff --git a/lib/apipie/generator/swagger/param_description/builder.rb b/lib/apipie/generator/swagger/param_description/builder.rb index eb695b1f..5fca774a 100644 --- a/lib/apipie/generator/swagger/param_description/builder.rb +++ b/lib/apipie/generator/swagger/param_description/builder.rb @@ -1,7 +1,7 @@ class Apipie::Generator::Swagger::ParamDescription::Builder # @param [Apipie::ParamDescription] param_description # @param [TrueClass, FalseClass] in_schema - # @param [Apipie::MethodDescription] controller_method + # @param [Apipie::MethodDescription, nil] controller_method def initialize(param_description, in_schema:, controller_method:) @param_description = param_description @in_schema = in_schema @@ -98,8 +98,8 @@ def required_from_path? def warn_optional_without_default_value(definition) if !required? && !definition.key?(:default) method_id = - if @param_description.is_a?(Apipie::ResponseDescriptionAdapter::PropDesc) - @controller_method + if @controller_method.present? && @param_description.is_a?(Apipie::ResponseDescriptionAdapter::PropDesc) + @controller_method.method_name else Apipie::Generator::Swagger::MethodDescription::Decorator.new(@controller_method).ruby_name end diff --git a/lib/apipie/generator/swagger/param_description/type.rb b/lib/apipie/generator/swagger/param_description/type.rb index f35c49b7..37c799fa 100644 --- a/lib/apipie/generator/swagger/param_description/type.rb +++ b/lib/apipie/generator/swagger/param_description/type.rb @@ -114,7 +114,11 @@ def validator def warn_hash_without_internal_typespec method_id = if @param_description.is_a?(Apipie::ResponseDescriptionAdapter::PropDesc) - @controller_method.method + if @controller_method.present? + @controller_method.method_name + else + Apipie::Generator::Swagger::MethodDescription::Decorator.new(nil).ruby_name + end else Apipie::Generator::Swagger::MethodDescription::Decorator.new(@param_description.method_description).ruby_name end diff --git a/spec/lib/apipie/generator/swagger/param_description/builder_spec.rb b/spec/lib/apipie/generator/swagger/param_description/builder_spec.rb index db7479df..c2fb5de0 100644 --- a/spec/lib/apipie/generator/swagger/param_description/builder_spec.rb +++ b/spec/lib/apipie/generator/swagger/param_description/builder_spec.rb @@ -145,6 +145,36 @@ /is optional but default value is not specified/ ).to_stderr end + + context 'and param is a prop desc' do + let(:param_description) do + Apipie.prop(:param, 'object', param_description_options, []) + end + + context 'with a delegated controller method' do + let(:method_desc) do + Apipie::Generator::Swagger::MethodDescription::Decorator.new( + Apipie::MethodDescription.new(:show, resource_desc, dsl_data) + ) + end + + it 'warns' do + expect { subject }.to output( + /is optional but default value is not specified/ + ).to_stderr + end + end + + context 'with a nil controller method' do + let(:method_desc) { nil } + + it 'warns' do + expect { subject }.to output( + /is optional but default value is not specified/ + ).to_stderr + end + end + end end end end diff --git a/spec/lib/apipie/generator/swagger/param_description/type_spec.rb b/spec/lib/apipie/generator/swagger/param_description/type_spec.rb index dbbe34f0..f08396ec 100644 --- a/spec/lib/apipie/generator/swagger/param_description/type_spec.rb +++ b/spec/lib/apipie/generator/swagger/param_description/type_spec.rb @@ -3,7 +3,6 @@ describe Apipie::Generator::Swagger::ParamDescription::Type do let(:validator_options) { {} } let(:param_description_options) { {}.merge(validator_options) } - let(:with_null) { false } let(:http_method) { :GET } let(:path) { '/api' } let(:validator) { String } @@ -63,9 +62,11 @@ ) end + let(:controller_method) { 'index' } + let(:type_definition) do described_class. - new(param_description, with_null: with_null, controller_method: 'index'). + new(param_description, with_null: false, controller_method: controller_method). to_hash end @@ -178,6 +179,32 @@ it 'outputs a hash without internal typespec warning' do expect { subject }.to output(/is a generic Hash without an internal type specification/).to_stderr end + + context 'and param is a prop desc' do + let(:param_description) do + Apipie.prop(param_description_name, 'object', {}, []) + end + + context 'with a delegated controller method' do + let(:controller_method) do + Apipie::Generator::Swagger::MethodDescription::Decorator.new( + method_desc + ) + end + + it 'outputs a hash without internal typespec warning' do + expect { subject }.to output(/is a generic Hash without an internal type specification/).to_stderr + end + end + + context 'and controller method is nil' do + let(:controller_method) { nil } + + it 'outputs a hash without internal typespec warning' do + expect { subject }.to output(/is a generic Hash without an internal type specification/).to_stderr + end + end + end end end end