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

Add support for defining headers for API methods. #341

Merged
merged 1 commit into from
Mar 23, 2015

Conversation

iliabylich
Copy link
Contributor

To be honest, I couldn't find the tests for views, so I've added tests only for model part. I will add missing tests as soon as somebody tells me where they are 😄

@iliabylich
Copy link
Contributor Author

partially closes #340

@alan-andrade
Copy link

Thanks for this 😎 !
Is it possible to use this with resource_description so that any method description includes these inherited headers ?

@ghost
Copy link

ghost commented Mar 19, 2015

@alan-andrade Good point, working on it

@ghost
Copy link

ghost commented Mar 19, 2015

@alan-andrade Done, will do rebase after 'plus one'

@mtparet
Copy link
Contributor

mtparet commented Mar 19, 2015

👍 good beginning! (next, we should be able to add validation on the headers)

@ghost
Copy link

ghost commented Mar 19, 2015

@mtparet I was thinking about validation, do we really need it? As I know, header is always a String (at least, Rack parses it as a String)

class SomeController
  def some_action
    request.headers.keys.map(&:class).uniq
    # => [String]
  end
end

Even if it comes to server as Fixnum, Rack converts it to String

request.headers['SERVER_PORT']
# => '3000'

@iNecas
Copy link
Member

iNecas commented Mar 19, 2015

Maybe just checking that the required headers are present…

@ghost
Copy link

ghost commented Mar 19, 2015

@iNecas do you mean passing required: true option? if so, it's already implemented, check https://github.com/Apipie/apipie-rails/pull/341/files#diff-e61d15662fc6870ce5dcd584e2fd6d11R42

@mtparet
Copy link
Contributor

mtparet commented Mar 19, 2015

Yes require should be enough (I was thinking about the 428 Precondition Required http://tools.ietf.org/html/rfc6585#section-3)

@ghost
Copy link

ghost commented Mar 19, 2015

@mtparet 428 Precondition Required is not a header, it's a status code, just like 200 OK, so it's out of this PR's scope

@ghost
Copy link

ghost commented Mar 19, 2015

Any other comments/missing stuff? I was going to add tests for views, but I couldn't find spec files.

@iNecas
Copy link
Member

iNecas commented Mar 19, 2015

@IlyaBylich I meant the validate_presence (https://github.com/Apipie/apipie-rails#id20) support for the headers, where,when enabled, it would fail if it's not there. But I can live with this just being documented somewhere.

@iNecas
Copy link
Member

iNecas commented Mar 19, 2015

No tests for views so far in apipie, no need to add them for this PR

@mtparet
Copy link
Contributor

mtparet commented Mar 19, 2015

@IlyaBylich I mean I'm not sure there are cases for which we need to verify the presence of a header except when we need to require a precondition HTTP header. (the absence of a precondition HTTP header could result to a 428 HTTP status)

@iliabylich iliabylich force-pushed the feature-add-headers-options branch from 3f20652 to 08159eb Compare March 19, 2015 17:32
@ghost
Copy link

ghost commented Mar 19, 2015

Added small note that headers can't be validate with validate_presence option (requested by @iNecas ). Rebased into single commit. Basically, ready for merge 😄

P.S. Just a little question about validate_presence config option. Why does the gem for building docs is responsible for params validation?

class ResourceDescription

attr_reader :controller, :_short_description, :_full_description, :_methods, :_id,
:_path, :_name, :_params_args, :_errors_args, :_formats, :_parent, :_metadata
:_path, :_name, :_params_args, :_errors_args, :_formats, :_parent, :_metadata,
:_headers

Choose a reason for hiding this comment

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

\ 😃 /
    |
👞  👞

This is great ! Thanks <3

@iNecas
Copy link
Member

iNecas commented Mar 20, 2015

@iliabylich the validations is maybe the most controversal feature. People either love it or don't understand why it's even there :)

The motivation for it was, given it's ruby code anyway, and it has enough data for doing the checking, why not just leveraging that information. The benefits are:

  1. DRY
  2. helps keeping the documentation up-to-date: when you forget to document some parameter, it doesn't work, so with it "when not documented, it literally doesn't exist"
  3. one of the solutions for mass-assignment issue, alternative to strong parameters

So yes, it doesn't belong to a conventional documentation tool. However, the apipie is quite unconventional :)

ACK from the code perspective: leaving some time for others also confirming they are ok with this PR (seems more people got interested). Expecting a new version with this released early next week

@ghost
Copy link

ghost commented Mar 20, 2015

@iNecas ok, it makes sense, I would like to add headers validation (but probably in another PR). And it's nice to hear that you going to release it so soon, I need it in my current project 😄

iNecas added a commit that referenced this pull request Mar 23, 2015
Add support for defining headers for API methods.
@iNecas iNecas merged commit 9a6ad52 into Apipie:master Mar 23, 2015
@iNecas
Copy link
Member

iNecas commented Mar 23, 2015

New version 0.3.3, with the change was just released. Thanks @iliabylich

@ghost
Copy link

ghost commented Mar 23, 2015

@iNecas Thanks for releasing!

dadario pushed a commit to dadario/apipie-rails that referenced this pull request Mar 23, 2015
@alan-andrade
Copy link

This is awesome ! <3 Thanks everyone

@yaneq6
Copy link

yaneq6 commented Sep 18, 2015

Hi, what about auto generation headers in docs based on functional tests? It would be a pretty cool feature.

@yaneq6
Copy link

yaneq6 commented Sep 18, 2015

I think good idea will be provide an option, to specify list of necessary headers that will be automatically include to docs based on tests.

@iliabylich
Copy link
Contributor Author

@yaneq6 Any ideas how to differentiate default rails/rack/whatever headers and custom headers? And very often your headers-related stuff like authentication is mocked in your tests, so you can't fetch headers because you just don't have it.

@yaneq6
Copy link

yaneq6 commented Sep 21, 2015

@iliabylich As I said, "good idea will be provide an option, to specify list of necessary headers". I mean, everyone who would use this feature should specify list of header in their own scope, somewhere in the config files (maybe apipie.rb or application.rb, whatever...) and only that headers should be automatically included in documentation. But please, don't expect from me any pull request, actually my rails skills are to low and i am not able to implement this feature on my own.

@byjus-taha-abbas
Copy link

Missing the following in generation.

add_headers_from_hash(swagger_result, method.resource._headers) if method.resource._headers.present?

@mathieujobin
Copy link
Collaborator

@byjus-taha-abbas this PR is 7 years old...
maybe there is an open PR already with the fix you desire.
otherwise, if you can open a PR, I will review it.
please add a test that confirms the new behavior is correct.

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.

7 participants