Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix error for hash object warnings with delegated method descriptions #938

Merged
merged 4 commits into from
Jul 16, 2024
Merged

Fix error for hash object warnings with delegated method descriptions #938

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e1fd88a
Use `method_name` instead of `method` when generating hash warnings
h-lame Jun 24, 2024
accd783
Use `method_name` instead of `method` when generating required warnings
h-lame Jun 24, 2024
ba599dc
Fix rubocop-rspec by removing a redundant `let`
h-lame Jul 3, 2024
6d84540
Handle warnings for param descriptions without a controller method
h-lame Jul 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions lib/apipie/generator/swagger/param_description/builder.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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)
Copy link
Contributor

@PanosCodes PanosCodes Jul 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we end up with a blank @controller_method ?
If so maybe we could add a test case for that & update the builder's YARD block to

# @param [Apipie::MethodDescription, nil] controller_method

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I admit that I haven't really dug into the "could @controller_method be nil" question. I added the @controller_method.present? check because of a test failing. I don't really know if in the normal usage of these objects to define API docs you would ever have a nil in there. But, obviously, the code doesn't actually prevent it.

I could take the check out and try to trace the test that failed a bit deeper to understand why it's able to construct the world where it's exercising these objects without a @controller_method being set. I don't really know what else might break elsewhere if it's nil.

Copy link
Contributor Author

@h-lame h-lame Jul 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A nil @controller_method is expected if we call json_schema_for_self_describing_class (see

apipie-rails/lib/apipie/swagger_generator.rb

Lines 36 to 43 in 4be3741

def self.json_schema_for_self_describing_class(cls, allow_nulls)
Apipie::Generator::Swagger::MethodDescription::ResponseSchemaService.new(
ResponseDescriptionAdapter.from_self_describing_class(cls),
allow_null: allow_nulls,
http_method: nil,
controller_method: nil
).to_swagger
end
) - eventually this ends up building a Apipie::Generator::Swagger::ParamDescription::Builder with a nil @controller_method. I'll update the YARD block and introduce an explicit test for it being nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, added a new commit that changes the YARD for Apipie::Generator::Swagger::ParamDescription::Builder and puts in tests for both Apipie::Generator::Swagger::ParamDescription::Builder and Apipie::Generator::Swagger::ParamDescription::Type when emitting warnings with PropDesc for a param_description and nil for a controller_method.

@controller_method.method_name
else
Apipie::Generator::Swagger::MethodDescription::Decorator.new(@controller_method).ruby_name
end
Expand Down
6 changes: 5 additions & 1 deletion lib/apipie/generator/swagger/param_description/type.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
30 changes: 30 additions & 0 deletions spec/lib/apipie/generator/swagger/param_description/builder_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
31 changes: 29 additions & 2 deletions spec/lib/apipie/generator/swagger/param_description/type_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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