Skip to content

Corrected error in description of Models (definitions) #629

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.

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

Closed
wants to merge 1 commit into from
Closed

Corrected error in description of Models (definitions) #629

wants to merge 1 commit into from

Conversation

kum-deepak
Copy link

I had created models using Grape Entities. In the generated Swagger JSON, each of the model was getting description, which was description of one of the API end points, which is incorrect.

Using a debugger I could figure out the line of code that is attaching the description. Removing it solves the problem.

I am not sure why this code was there. Please suggest if there is a better way to solve the issue.

@coveralls
Copy link

coveralls commented Aug 18, 2017

Coverage Status

Coverage decreased (-0.003%) to 97.642% when pulling 8886968 on kum-deepak:master into d2b9f67 on ruby-grape:master.

@LeFnord
Copy link
Member

LeFnord commented Aug 19, 2017

@kum-deepak …thanks, but wouldn't it be better to ensure, the correct description is taken instead of removing it, as your PR title suggest?

@kum-deepak
Copy link
Author

That will be a great idea. The current implementation is definitely incorrect, as it picks up one of the API endpoint's description as description of an entity.

I have not been able to figure out from where to pickup the correct description. If there was a desc for the Grape::Entity that would be suitable. Not sure if it is possible to add desc for the Grape::Entity.

Any suggestions are welcome.

@LeFnord
Copy link
Member

LeFnord commented Aug 20, 2017

I would suggest as starting point to have a look at the changing status code part, there a model description can be set, but at the moment only on the response object (→ it seems like a bug), think this description can also be used for the model definition …

the problem with that solution would be, that a resource can have multiple endpoints with the same response model, so the handling should be checked … for that I see following possibilities

  1. all must have the same description
  2. only one description is enough
  3. all description will be concatenated

what did you think?

@kum-deepak
Copy link
Author

kum-deepak commented Aug 21, 2017

I have started using Grape and Swagger about 6 months back. Thanks for all the wonderful work!!

I understand that (the same) Grape::Entity can be used in following ways:

  • Entire params (as Entity.documentation)
  • As return value (as entity, success, or failure)
  • Nested within another Grape::Entity

I tend to write long descriptions (including examples) to quite few of my API end points. The problem happened when one of models, that was nested within another, started showing description of another unrelated API end-point.

In my opinion it is quite risky to pickup description of an Entity from the API endpoint where it is used.

I realized that, at least in my case, setting an empty description does not hamper quality of documentation.

Is it possible to extend Grape::Entity to allow setting description of the model? That will allow to set the description where it belongs.

There can still be sub-issues, like the cases where the generated Swagger has models which are not explicitly declared (I currently do not understand in which all cases it happens).

@LeFnord LeFnord mentioned this pull request Feb 8, 2018
@owenbendavies
Copy link

I also find using one of the random descriptions from an endpoint as a model description provides stange results. I may get time to work on this, if I do I see 3 solutions

  1. Not set any model description as it is not a required attribute in Swagger
  2. Add a new description attribute to Grape::Entity and use that if present
  3. In add_swagger_documentation under models, allow descriptions to be set similar to how tags are set

Do people have any preference?

@kum-deepak
Copy link
Author

My suggestion will be option 2, to add a description attribute to Grape::Entity and use that if present.

Please let me know if I can be of any help.

As side note - it seems that in certain cases a Grape::Entity is automatically generated based on params. We should include a test case covering the same.

@LeFnord
Copy link
Member

LeFnord commented Mar 30, 2018

maybe this ruby-grape/grape-entity#187 can be helpful for option 2

@OlivierB
Copy link

Maybe solution 1 is a good start.

  • The use of the last desc for the model description gives irrelevant information
  • It seems no one has time to correct this minor problem, so the simplest solution could be the best

@LeFnord
Copy link
Member

LeFnord commented Mar 21, 2020

maybe someone can take it over?

@kum-deepak
Copy link
Author

Thanks @LeFnord! I can take a shot.

@LeFnord
Copy link
Member

LeFnord commented Sep 3, 2020

closed via #804

@LeFnord LeFnord closed this Sep 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants