For discussion: proposal for describing responses #588

elasti-ron · 2018-01-02T16:16:54Z

This pull request is for supporting a discussion, not for merging into the codebase (as per request from @iNecas).

Also relevant to #273

…nses

iNecas · 2018-01-06T12:53:41Z

First of all thanks @elasti-ron for the proposal: I like it very much. Two suggestions:

introduce an alias for param, something like property, that could be used especially for response only parameters. Maybe property could have implicitly :only_in => :response attribute
I would recommend thinking about possibility for the returns keyword to accept a block. This would allow to use multiple groups in the response, that would be merged to describe the response.

returns do
  param_group :user
  param_group :user_response
end

What do you think?

iNecas · 2018-01-06T12:53:58Z

Also, let me invite @tstrachota and @mbacovsky for more eyes having on it.

elasti-ron · 2018-01-10T10:36:11Z

Thanks @iNecas - I like both of your suggestions.

One thing that should be explicitly stated is the exact structure of a JSON response given a returns statement containing multiple param_group elements.

To maintain consistency with how input definitions are handled, I would expect that a returns statement with multiple param_group fields implies a single JSON object with the property elements of all of the param_group fields unwrapped into the same level.

For example, the following DSL:

param_group :user do
     param :name, String
end
param_group :user_response do
     property :vote, Boolean
end

returns do
    param_group :user
    param_group :user_response
end

would describe a JSON response that might look something like this:

{
    "name": "Joe",
    "vote":  true
}

iNecas · 2018-01-10T14:22:42Z

Yes, that's how multiple param_groups for api description currently should work as well

elasti-ron · 2018-01-10T19:05:55Z

Great. I'll share some experimental implementation code soon to get your thoughts and insights.

elasti-ron · 2018-01-15T13:13:35Z

I pushed an update with a preliminary implementation. It includes the first 3 elements of the proposal (i.e., using a returns keyword with either a param_group or block to describe a response, as well as the :only_in=>:response feature). I'm now starting to implement returns :array_of => <param_group> and adapters.

Please take a look at the new PetsController for usage examples, and swagger_dsl_spec.rb for verification logic on PetsController.

Would love to get your feedback.

elasti-ron · 2018-01-15T16:34:02Z

Now also with returns :array_of => <param_group name>

elasti-ron · 2018-01-25T15:10:08Z

@iNecas - the latest commit includes the 'adapter' mechanism, but as an internal feature, allowing for clearer syntax (I think).

In the original proposal, future integration with Grape::Entity would have looked as follows:

# entity defined using Grape::Entity
class UserView < Grape::Entity
    expose :id, documentation: {type: Integer, desc: "user id", required: true}
    expose :name
end

# API defined using Apipie
# where 'from_grape_entity' is an adapter that exposes the contents of the entity
# as if it were an Apipie::ParamDescription object 
api :GET, "/users", "Get all user records"
returns :array_of => from_grape_entity(UserView), :desc => "the requested user records"

The implementation in the latest commit uses a slightly different approach: instead of indicating from_grape_entity in the returns statement, the integration of the view with Apipie would be in the UserView object itself, for example:

# entity defined using Grape::Entity
class UserView < Grape::Entity
    include "ApipieAdapter"  # this is a hypothetical module - not part of the current version
                             # such a module would include a mixin that implements
                             # a 'describe_own_properties` method, called by Apipie
                             # to get information about the generated view structure
    expose :id, documentation: {type: Integer, desc: "user id", required: true}
    expose :name
end

# API defined using Apipie
# no need to specify the 'from_grape_entity'  adapter because UserView includes the mixin
api :GET, "/users", "Get all user records"
returns :array_of => UserView, :desc => "the requested user records"

Please take a look at PetsUsingSelfDescribingClassesController and PetsUsingAutoViewsController (both in spec/dummy/app/controllers) for working examples.

Looking forward to your feedback.

elasti-ron · 2018-02-08T17:40:56Z

Added ability to define response descriptions at the resource level.

BTW, I'm currently working (in a separate branch) on a feature to validate a JSON response.body against the 'returns' definition (by using the response schema in the generated swagger). Will merge into this branch after some additional cleanups. What I'm trying out there are two mechanisms, both intended to be used in a Controller test in RSpec :

a matcher to validate a response object (expect(response).to match_declared_responses)
an RSpec keyword auto_validate_rendered_views that turns on auto-validation for all responses within an RSpec block (similar to how RSpec's render_views keyword turns on response rendering in a block of a Controller spec)

elasti-ron · 2018-02-12T12:02:39Z

Hi @iNecas - any thoughts on this so far?

abenari · 2018-02-19T12:46:17Z

@iNecas Can you review this PR?

iNecas · 2018-02-20T18:06:14Z

lib/apipie/method_description.rb

@@ -44,9 +48,10 @@ def initialize(method, resource, dsl_data)

      @metadata = dsl_data[:meta]

-      @params_ordered = dsl_data[:params].map do |args|
+      @params_ordered = dsl_data[:params].map { |args|


This is a multilne block, please keep do...end here.

iNecas · 2018-02-20T18:11:48Z

spec/spec_helper.rb

+#           }
+#
+#
+RSpec::Matchers.define :match_field_structure do |expected|


Were are we using those?

Used quite heavily in spec/lib/swagger/swagger_dsl_spec.rb

iNecas · 2018-02-20T18:12:31Z

lib/apipie/param_description.rb

@@ -62,6 +64,9 @@ def initialize(method_description, name, validator, desc_or_options = nil, optio

      @required = is_required?

+      @response_only = (@options[:only_in] == :response)


What about request only case?

Makes sense. I'll add support for this right after merging the 'response validation' mechanism that I'm cleaning up at the moment.

iNecas · 2018-02-20T18:14:01Z

lib/apipie/dsl_definition.rb

+            adapter = ResponseDescriptionAdapter.from_self_describing_class(descriptor)
+          else
+            begin
+                block = Apipie.get_param_group(scope, descriptor) if descriptor


The indentation should be preserved on two spaces

iNecas · 2018-02-20T18:17:25Z

PROPOSAL_FOR_RESPONSE_DESCRIPTIONS.md

+```ruby
+# entity defined using Grape::Entity
+class UserView < Grape::Entity
+    expose :id, documentation: {type: Integer, desc: "user id", required: true}


Indentation :)

iNecas · 2018-02-20T18:19:52Z

lib/apipie/response_description.rb

@@ -0,0 +1,113 @@
+module Apipie
+
+  class ResponseObject


Either nest it under ResponseDescription or put it into separate file, so that the file naming follows the class names

iNecas · 2018-02-20T18:21:38Z

Some comments inline, mainly styling one. I need still to get a bit deeper on the code wise: one of the non-style questions is the lack of :only_if => :request option.

iNecas · 2018-02-26T17:03:04Z

apipie-rails.gemspec

@@ -17,12 +17,12 @@ Gem::Specification.new do |s|
  s.require_paths = ["lib"]

  s.add_dependency "rails", ">= 4.1"
+  s.add_dependency "json-schema", ">= 2.8"


AFAIK we are using this only in tests, therefore it should not be a runtime dependency, unless we have a really good reason for doing so.

iNecas · 2018-02-26T17:08:08Z

lib/apipie/dsl_definition.rb

+            begin
+              block = Apipie.get_param_group(scope, descriptor) if descriptor
+            rescue
+              raise "No param_group or self-describing class named #{descriptor}"


What kind of exceptions do we expect here?

get_param_group raises an exception if there is no param_group with the specified name:

def get_param_group(controller, name) key = "#{controller.name}##{name}" if @param_groups.has_key?(key) return @param_groups[key] else raise "param group #{key} not defined" end end

The new code (raising "No param_group or self-describing class named...") merely changes the message of the exception because the input to returns is not limited to being a param_group.

iNecas · 2018-02-26T17:14:26Z

lib/apipie/rspec/response_validation_helper.rb

+#
+#
+# to use this file in a controller rspec you should
+# require 'apipie/rspec/response_validation_helper' in the spec file


I guess we could put the documentation into the README, so that people find it more easily. Also, I would rather see the people adding the json-schema depednency explicitly, rather than us adding this explicitly, as there are many folks that would not need this dependency at all.

Putting that aside, would you mind holding the whole feature out of this PR: it's already getting quite big/hard to review.

elasti-ron · 2018-02-27T10:35:23Z

I pushed a version with the response-validation feature redacted - hope this helps.

BTW, it occurred to me when removing this feature that its unit tests are the only ones that check the semantic correctness of the swagger, so it would be good to get it back in soon. The unit tests that we currently have are purely syntactic: they verify that the generated swagger is valid, but they don't check that it is correct.

elasti-ron · 2018-03-07T10:24:48Z

Hi @iNecas - did you get a chance to look at the revised code?

abenari · 2018-03-15T09:54:26Z

@iNecas any progress on this?

abenari · 2018-03-26T08:42:19Z

Hi @iNecas,
This is waiting for about a month without any new comment.
We have a few more changes we want to push adding response validation and an improved swagger generation. Will you be able to review our proposal soon?

iNecas · 2018-03-28T14:01:17Z

PROPOSAL_FOR_RESPONSE_DESCRIPTIONS.md

@@ -0,0 +1,244 @@
+# Proposal for supporting response descriptions in Apipie


Since we are getting close to merging this, would you mind turning this into the documentation inside README.rst?

Sure thing.

[Updated Apr-1] I pushed a version now with ~~most of~~ the README.rst work. ~~Still need to document the response description adapter mechanism, but the straightforward use of the returns and property keywords is now there.~~ Would you mind taking a look and letting me know if you have any comments?

iNecas · 2018-03-28T14:02:41Z

Sry for delays, still tweaking my time-management approach to be able to respond faster, but not quite there yet, as you see :) Anyway, I like the direction now. Last one request for turning the proposal into a readme.

…how to document a response using an 'adapter' and/or 'self-describing class'

elasti-ron · 2018-04-10T14:32:38Z

Hi @iNecas - just wanted to let you know that the Ruby project I'm currently working on will end on Apr 22, and my focus will turn to other things.

At that time my availability to help out with this PR (and with integration of additional enhancements that I'm waiting to upload - such as response validation, generation of swagger response objects as references and a couple of other unrelated ones) will be very limited, if at all. It would be great if we could finish this process ASAP.

iNecas · 2018-04-27T06:58:10Z

Thanks. No other comments now. Merging this now and releasing new version.

iNecas · 2018-04-27T10:52:43Z

apipie-rails-0.5.8 with this change has just been released

damuz91 · 2018-05-02T00:40:51Z

@iNecas Amazing, i used apipie 2 years ago and left it behind. Now i am using it again and i see the returns feature which is amazing. I am using it but can't get it to be displayed in the docs :( I will try to get this reproduced and open an issue for this

ajaleelp · 2018-05-27T08:10:14Z

@damuz91 returns are not being displayed for me too 😕 Where you able to figure out why? Or have you opened an issue for this? I couldn't find one.

waissbluth · 2018-06-18T21:20:40Z

@damuz91 @ajaleelp, I'm facing the same issue; did you figure this out? Thanks!

rrameshbtech · 2018-07-10T06:56:22Z

@iNecas, I am still not getting response of the API in documents, even after adding returns block. any suggestions?

iNecas · 2018-07-10T12:09:14Z

No clue right now: I will hopefully will be able to look at this in 2 weeks, but probably not sooner, so if anyone wants to give it a try and debug it more deeply, any help would be appreciated.

X1ting · 2018-10-16T15:37:32Z

@iNecas #635 this PR will fix this issue, can you review pls?

iNecas · 2018-10-17T07:41:02Z

@X1ting thanks, the issue is merged now and available in 0.5.12

For discussion: added a document with a proposal for describing respo…

2e8f7a2

…nses

This was referenced Jan 2, 2018

Describing a response #273

Closed

Added Swagger generation #569

Merged

iNecas reviewed Feb 20, 2018

View reviewed changes

iNecas reviewed Feb 26, 2018

View reviewed changes

response descriptions implemented. does not include response validation.

f2e2dda

elasti-ron force-pushed the describe_responses branch from 4abff67 to f2e2dda Compare February 27, 2018 10:14

iNecas reviewed Mar 28, 2018

View reviewed changes

elasti-ron added 2 commits March 29, 2018 19:01

README.rst now explains the 'returns' keyword. Still need to explain …

2381d13

…how to document a response using an 'adapter' and/or 'self-describing class'

Update README.rst to document self-describing classes for responses

9e066d0

iNecas merged commit 2a14791 into Apipie:master Apr 27, 2018

micapam mentioned this pull request Jul 19, 2018

Response description isn't beign shown in docs #623

Open

X1ting mentioned this pull request Oct 15, 2018

fix returns displaying #635

Merged

		@@ -62,6 +64,9 @@ def initialize(method_description, name, validator, desc_or_options = nil, optio

		@required = is_required?

		@response_only = (@options[:only_in] == :response)

		@@ -0,0 +1,244 @@
		# Proposal for supporting response descriptions in Apipie

For discussion: proposal for describing responses #588

For discussion: proposal for describing responses #588

Conversation

elasti-ron commented Jan 2, 2018 • edited Loading

iNecas commented Jan 6, 2018

iNecas commented Jan 6, 2018

elasti-ron commented Jan 10, 2018

iNecas commented Jan 10, 2018

elasti-ron commented Jan 10, 2018

elasti-ron commented Jan 15, 2018

elasti-ron commented Jan 15, 2018

elasti-ron commented Jan 25, 2018

elasti-ron commented Feb 8, 2018 • edited Loading

elasti-ron commented Feb 12, 2018

abenari commented Feb 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iNecas commented Feb 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasti-ron Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasti-ron commented Feb 27, 2018

elasti-ron commented Mar 7, 2018

abenari commented Mar 15, 2018

abenari commented Mar 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasti-ron Mar 29, 2018 • edited Loading

Choose a reason for hiding this comment

iNecas commented Mar 28, 2018

elasti-ron commented Apr 10, 2018

iNecas commented Apr 27, 2018

iNecas commented Apr 27, 2018

damuz91 commented May 2, 2018

ajaleelp commented May 27, 2018

waissbluth commented Jun 18, 2018

rrameshbtech commented Jul 10, 2018

iNecas commented Jul 10, 2018

X1ting commented Oct 16, 2018

iNecas commented Oct 17, 2018

elasti-ron commented Jan 2, 2018 •

edited

Loading

elasti-ron commented Feb 8, 2018 •

edited

Loading

elasti-ron Feb 27, 2018 •

edited

Loading

elasti-ron Mar 29, 2018 •

edited

Loading