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

No way to set custom headers #18

Closed
dryobates opened this issue Oct 16, 2018 · 6 comments
Closed

No way to set custom headers #18

dryobates opened this issue Oct 16, 2018 · 6 comments

Comments

@dryobates
Copy link

In regular flask function I could return headers as last parameter:

return body, status_code, headers

That's how I return e.g. "Location" header.

In flask-rest-api I can't return such tuple nor Response object with custom headers because Api.response requires object to serialize.

I see that there is some headers handling in code https://github.com/Nobatek/flask-rest-api/blob/master/flask_rest_api/response.py#L74 but I do not know how to elegantly set them in my function.

If there's no elegant way then maybe handling return value as in flask (https://github.com/pallets/flask/blob/master/flask/app.py#L1890) should be implemented?

@lafrech
Copy link
Member

lafrech commented Oct 16, 2018

Valid point.

flask-rest-api uses the application context to pass header informations from one decorator to the other. You can access the application context using utils.get_app_context() so you could do
utils.get_app_context()['headers'].update({'my-header': 'value'}). But this looks more like a workaround. I'd rather keep this private API.

I could add the same return value handling as in Flask, allowing to return tuples from view funcs. This way, headers could be returned from the view function.

Using the same mechanism, the view function could also return a status code. This would allow a view function to override the code defined in the @response decorator (related to #15), but I'd be concerned about the consistency with the documentation and I don't like to have different ways of achieving the same thing.

I need to think this through. It looks like a nice improvement, but it's an important change in the logic of the code.

Until then, you can use utils.get_app_context()['headers'] as described above.

BTW, the headers structure in the application context is a dict. It should be a werkzeug.Headers object.

@ocryle
Copy link

ocryle commented Nov 23, 2018

Hi!

I think this is related to the issue.

It seems like there is no way to fully modify response data structure. There is ResponseMixin._prepare_response_content, but it doesn't help when exception happened. Data structure I'm talking about:

{
	data: ...
	status: 'ok'
}

in case of my internal exception

{
	status: 'error',
	message: 'some error message to show to a user'
}

Right now I'm thinking about a hack - override response() and add there my own decorator, something like this:

def response(self, *args, **kwargs):
    decorator = super().response(*args, **kwargs)
    def next_decorator(func):
        @wraps(func)
        def wrapper(*args, **kwargs):
            try:
                r = decorator(func(*args, **kwargs))
                return r
            except AppException as e:
                resp = jsonify(self._prepare_response_content({
                	'status': 'error',
                	'message': str(e)
                }))
                resp.headers.extend(get_appcontext()['headers'])
                resp.status_code = e.status_code
        return wrapper
    return next_decorator

Thanks!

@lafrech
Copy link
Member

lafrech commented Nov 23, 2018

@ocryle, for this use case, I'd abort with a sensible error code and message.

    abort(422, "Foo is not a valid Bar")

Then if I'm not happy with the default error structure, I'd override _prepare_error_reponse_content.

(I just realized the typo in the name s/reponse/response. I shall fix that asap. Edit: Fixed in master.)

This means all your errors look the same. I agree this is a constraint, but I think it is nice from client POV. And it's nice in your views because you just need one call to abort.

@ocryle
Copy link

ocryle commented Nov 23, 2018

Yeah, that seems to be a much better way to do this.

Thank you!

@lafrech
Copy link
Member

lafrech commented Feb 24, 2019

@dryobates, I just sent a PR fixing this (#40). Feedback welcome.

@dryobates
Copy link
Author

@lafrech Thanks. It looks great. I have only one problem with this approach - transition time. Now I use get_context()["headers"] ;) But as it's undocumented and as I have to adjust to 0.13.0 other's backward incompatible changes that's not big deal :)

Thank you very much. New API is beautifully symmetrical with flask's own :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants