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.

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

Fix handling of HTTP status codes from routes #669

Merged
merged 1 commit into from
Mar 30, 2018

Conversation

milgner
Copy link
Contributor

@milgner milgner commented Mar 6, 2018

After updating Grape, the documentation could not be generated anymore.

Upon investigation it turned out that the HTTP codes were not stored in a hash (anymore) but as a two-element array with the status code first and a message second. After applying this patch, documentation was generated successfully again.

@milgner
Copy link
Contributor Author

milgner commented Mar 6, 2018

Damn, did this drive-by-change and forgot to adapt the specs... Will do so...

@milgner
Copy link
Contributor Author

milgner commented Mar 6, 2018

After additional investigation it looks like there are multiple ways to define status codes and they result in different representations in the internal structures.

The routes used for the specs in this gem use

          http_codes: [
            { code: 200, message: 'get Splines', model: Api::Entities::Splines },
            { code: 422, message: 'SplinesOutError' }
          ]

whereas the Grape documentation (and our application) uses

  failure [[401, 'Unauthorized', 'Entities::Error']]

which also ends up in the same structure.

After checking the Grape README, I did not explicitly see http_codes used. But in the end it seems like grape-swagger will need to support both versions. So I'll have to adapt the code to deal with both constellations.

@LeFnord
Copy link
Member

LeFnord commented Mar 6, 2018

the keys has changed, see: grape#describing-methods, so former entity is success and http_codesare4 failure (should also be plural imho), bugt internaly is no difference

the difference is, that failure can accept an Array of Arrays in grape and in grape-swagger also an Array of Hashes

please have in mind these keys would only be used by grape-swagger

@coveralls
Copy link

coveralls commented Mar 7, 2018

Coverage Status

Coverage increased (+0.009%) to 97.257% when pulling 42ff185 on evopark:fix/http-codes into 3057a2c on ruby-grape:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 97.909% when pulling 9efefe1 on evopark:fix/http-codes into 9e5720a on ruby-grape:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 97.909% when pulling 9efefe1 on evopark:fix/http-codes into 9e5720a on ruby-grape:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 97.909% when pulling 9efefe1 on evopark:fix/http-codes into 9e5720a on ruby-grape:master.

@milgner
Copy link
Contributor Author

milgner commented Mar 26, 2018

Just a quick update: I haven't forgotten about this PR, just been a bit low on time for OSS work - I have a couple of free days coming up this week and will take care of getting all the different variations of this covered then.

@LeFnord
Copy link
Member

LeFnord commented Mar 27, 2018

@milgner thanks for the update 😄

@milgner
Copy link
Contributor Author

milgner commented Mar 28, 2018

Oh no, looks like my changes tipped the metrics for Metrics/ClassLength of Endpoint over the edge... 🙈

@grape-bot
Copy link

grape-bot commented Mar 28, 2018

1 Error
🚫 One of the lines below found in CHANGELOG.md doesn’t match the expected format. Please make it look like the other lines, pay attention to periods and spaces.
* [#667](https://github.com/ruby-grape/grape-swagger/pull/667):  Make route summary optional - [@obduk](https://github.com/obduk).

Generated by 🚫 danger

@LeFnord
Copy link
Member

LeFnord commented Mar 29, 2018

thanks @milgner … please can you rebase and update the CHANGELOG with your entry

@milgner
Copy link
Contributor Author

milgner commented Mar 30, 2018

👍 Done!

@LeFnord
Copy link
Member

LeFnord commented Mar 30, 2018

thanks again …

@LeFnord LeFnord merged commit 6402c2c into ruby-grape:master Mar 30, 2018
LeFnord pushed a commit to LeFnord/grape-swagger that referenced this pull request Feb 9, 2019
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.

4 participants