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

h-lame · 2024-06-25T13:28:07Z

Fix how we generate warnings about params or properties defined as Hashes without defining what keys they can have inside them.

Our swagger docs stopped working with an ArgumentError:

ArgumentError: wrong number of arguments (given 0, expected 1)

And it turns out it was in the Apipie::Generator::Swagger::ParamDescription::Type#warn_hash_without_internal_typespec method where we call @controller_method.method and @controller_method was a Apipie::Generator::Swagger::MethodDescription::Decorator instance instead of a Apipie::MethodDescription instance. The Decorator is a SimpleDelegator onto a MethodDescription so it should be able to call method with no arguments, but it turns out that SimpleDelegator won't delegate method because it keeps it to refer to Object#method. In #865 we introduced the method_name method on MethodDescription to address this very problem, but we didn't actually use it in all the places we needed to. This PR fixes that in warn_hash_without_internal_typespec where my code was breaking and in Apipie::Generator::Swagger::ParamDescription::Builder#warn_optional_without_default_value where we weren't trying to call method but we probably should have been. The error here was getting #<Apipie::MethodDescription:0x0000000126316f60> in log messages when we'd rather get the method name.

See #939 for some commits in this PR that should be merged separately - they're about a set of fixes that get us to a green build on modern ruby, rack, rubocop infrastructure. For now, focus on the "Use method_name..." commits in this PR.

mathieujobin · 2024-07-03T06:44:35Z

@PanosCodes opinion ?

mathieujobin · 2024-07-03T06:46:17Z

lib/apipie/error_description.rb

+            begin
+              Rack::Utils.status_code(code_or_options)
+            rescue ArgumentError
+              nil


why nil here? instead of code_or_options ?

Preserves the old behaviour - if code_or_options was a symbol and that symbol wasn't present in Rack::Utils::SYMBOL_TO_STATUS_CODE then the return would be nil. Happy to change to code_or_options though if you think that's better - arguably it'd probably be more useful to have code_or_options and it's closer to the "not a symbol" code path.

lib/apipie/response_description.rb

mathieujobin

if all tests and rubocop passes, I don't have any objections.
but I would like @PanosCodes or someone else to chime in for final approval.

h-lame · 2024-07-03T09:11:03Z

if all tests and rubocop passes, I don't have any objections. but I would like @PanosCodes or someone else to chime in for final approval.

I've addressed the rubocop problems - needed to introduce rubocop-rspec_rails and fix an existing offence to get to green - then tidy up my own introduced offence. Both of those in new commits.

Fixed the specs I broke, but there's one that is failing on master for me about rack middleware - I suspect it's because it's using rack 3 now on a fresh install, so let me know if you want me to tidy it up as part of this PR or not. I've already tidied up a few things to get to running rspec + rubocop so it definitely fits - but I also don't want to scope creep or introduce loads of unnecessary change.

mathieujobin · 2024-07-03T14:40:38Z

only one issue with rails 7.1

you don't need to worry about rails 5.x and ruby 2.6, someone else increased the minimum ruby without updating the build matrix.

PanosCodes

Nice , thank you for your time 🙏

It's not clear to me why we need rubocop-rspec_rails in this PR, could we maybe have it in a separate one ? 🤷

PanosCodes · 2024-07-04T17:32:09Z

lib/apipie/generator/swagger/param_description/builder.rb

@@ -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)


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

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.

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.

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.

h-lame · 2024-07-05T15:20:40Z

It's not clear to me why we need rubocop-rspec_rails in this PR, could we maybe have it in a separate one ? 🤷

The commits with rubocop-rspec_rails and the rack changes are all in service of getting to a green build from a fresh checkout of the repo. I agree they're making the actual change I want to make less clear. I'll separate them out into a "get the build green" PR and "fix the warnings about hashes" PR.

h-lame · 2024-07-05T15:31:49Z

you don't need to worry about rails 5.x and ruby 2.6, someone else increased the minimum ruby without updating the build matrix.

Good to know, although it looks like gemspec still says ruby >= 2.6 and rails >= 5? Should I update those references?

h-lame · 2024-07-05T15:56:25Z

I'll separate them out into a "get the build green" PR and "fix the warnings about hashes" PR.

I've extracted those commits out to #939 - and also fixed the broken spec about the recorder middleware caused by using rack 3.

If we can merge that then I can rebase this against that and we can focus on the two "Use method_name ..." commits that are the actual change we care about for this PR.

In Apipie#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.

h-lame · 2024-07-09T16:41:42Z

Rebased now that #939 is merged and that makes the change much smaller and contained. To address is the outstanding comment on a blank @controller_method - I'll try to trace that through and see if it's the test being setup weird, or an expected way of using the code.

mathieujobin · 2024-07-10T23:41:58Z

I'll try to trace that through and see if it's the test being setup weird, or an expected way of using the code.

@h-lame if you could add a test that covers/prove its well handled that'd be most perfect.

mathieujobin

Looking at this one more time, LGTM ! I think I will merge next week unless objections

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): [#<Apipie::MethodDescription:0x0000000126316f60>] -- 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.

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.

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.

mathieujobin · 2024-07-12T08:26:07Z

quick compare link for the last force-push
https://github.com/Apipie/apipie-rails/compare/28ba4a7..6d84540

Thanks

PanosCodes

Nice job 🚀

mathieujobin requested review from ekohl, iNecas and ofedoren July 3, 2024 06:44

mathieujobin reviewed Jul 3, 2024

View reviewed changes

lib/apipie/response_description.rb Outdated Show resolved Hide resolved

mathieujobin approved these changes Jul 3, 2024

View reviewed changes

h-lame force-pushed the fix-error-for-hash-object-warnings-with-delegated-method-descriptions branch from 831c7d8 to b6a2e8e Compare July 3, 2024 09:01

PanosCodes suggested changes Jul 4, 2024

View reviewed changes

h-lame mentioned this pull request Jul 5, 2024

Get the build green for modern ruby and rack and rubocop-rspec #939

Merged

h-lame force-pushed the fix-error-for-hash-object-warnings-with-delegated-method-descriptions branch from b6a2e8e to 28ba4a7 Compare July 9, 2024 16:39

mathieujobin approved these changes Jul 12, 2024

View reviewed changes

h-lame added 3 commits July 12, 2024 08:25

h-lame force-pushed the fix-error-for-hash-object-warnings-with-delegated-method-descriptions branch from 28ba4a7 to 6d84540 Compare July 12, 2024 07:26

PanosCodes approved these changes Jul 12, 2024

View reviewed changes

mathieujobin merged commit fe7afaa into Apipie:master Jul 16, 2024
25 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

h-lame commented Jun 25, 2024 •

edited

Loading

mathieujobin commented Jul 3, 2024

mathieujobin Jul 3, 2024

h-lame Jul 3, 2024

mathieujobin left a comment

h-lame commented Jul 3, 2024

mathieujobin commented Jul 3, 2024

PanosCodes left a comment

PanosCodes Jul 4, 2024 •

edited

Loading

h-lame Jul 5, 2024

h-lame Jul 12, 2024 •

edited

Loading

h-lame Jul 12, 2024

h-lame commented Jul 5, 2024

h-lame commented Jul 5, 2024 •

edited

Loading

h-lame commented Jul 5, 2024

h-lame commented Jul 9, 2024

mathieujobin commented Jul 10, 2024

mathieujobin left a comment

mathieujobin commented Jul 12, 2024

PanosCodes left a comment

	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

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

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

Conversation

h-lame commented Jun 25, 2024 • edited Loading

mathieujobin commented Jul 3, 2024

mathieujobin Jul 3, 2024

Choose a reason for hiding this comment

h-lame Jul 3, 2024

Choose a reason for hiding this comment

mathieujobin left a comment

Choose a reason for hiding this comment

h-lame commented Jul 3, 2024

mathieujobin commented Jul 3, 2024

PanosCodes left a comment

Choose a reason for hiding this comment

PanosCodes Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

h-lame Jul 5, 2024

Choose a reason for hiding this comment

h-lame Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

h-lame Jul 12, 2024

Choose a reason for hiding this comment

h-lame commented Jul 5, 2024

h-lame commented Jul 5, 2024 • edited Loading

h-lame commented Jul 5, 2024

h-lame commented Jul 9, 2024

mathieujobin commented Jul 10, 2024

mathieujobin left a comment

Choose a reason for hiding this comment

mathieujobin commented Jul 12, 2024

PanosCodes left a comment

Choose a reason for hiding this comment

h-lame commented Jun 25, 2024 •

edited

Loading

PanosCodes Jul 4, 2024 •

edited

Loading

h-lame Jul 12, 2024 •

edited

Loading

h-lame commented Jul 5, 2024 •

edited

Loading