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

Is it possible to implement extended response #9

Closed
asyncee opened this issue Sep 7, 2018 · 17 comments
Closed

Is it possible to implement extended response #9

asyncee opened this issue Sep 7, 2018 · 17 comments
Labels

Comments

@asyncee
Copy link

asyncee commented Sep 7, 2018

Hello!

First of all, thank you for such a great library!

I have and old legacy API that returns responses like so:

{
    "type": "error",
    "kind": "validation",
    "message": { ... some message ...}  # this is built upon exception text
}

# or

{
    "type": "success",
    "result": { ... some data ...}  # this is what view returns
}

As you can see, the responses have envelope. Is it possible to implement such functionality in pythonic way still using @blp.response decorator?

Error condition can be determined on any exception (or specific like ValidationError to set kind and message fields) and on status method. Same for successful response.

Thanks!

@lafrech
Copy link
Member

lafrech commented Sep 21, 2018

Hi @asyncee. Thanks for the positive feedback.

flask-rest-api was built over a few opiniated choices, to fit our own needs, and this is not the format we're using, so it is not supported out-of-the-box.

However, after a very quick look, I think there may be a way to do it easily by just

  • subclassing Blueprint to override response decorator, wraping its output content into your structure
  • wrapping or overriding handle_http_exception to do the same with error messages

You might have to also subclass Api to override init_app to register your handler in place of the one in flask-rest-api (I don't know what happens if multiple handlers are registered).

Please tell us how it goes if you give it a try. I could take the error handler registration out of init_app into a separated method if it helps subclassing.

@asyncee
Copy link
Author

asyncee commented Sep 21, 2018

Thanks! I will give it a try in a few days and will report back about results.

@asyncee
Copy link
Author

asyncee commented Sep 23, 2018

I wrote some test code with subclassing and overriding and it works.

Main problem of such approach i see is that too much logic got to be overriden, so if base library will change some decorator-specific logic, then things may break and my custom overriden decorators will differ from library provided ones.

What i'm talking about is, for example, response decorator:

# this logic is overriden in decorator

      if disable_etag:
            disable_etag_for_request()

            # Check etag precondition
            check_precondition()

            # Store etag_schema in AppContext
            set_etag_schema(etag_schema)

            # Execute decorated function
            result = func(*args, **kwargs)

            # Verify that check_etag was called in resource code if needed
            verify_check_etag()

Also error handler decorator — it has payload and headers mangling code.

So any of these decorators has 2 responsibilities:

  1. build response payload that will be jsonified later
  2. build json response

The problem is in second — once response is built it is hard to manipulate with, so approach to decorate provided decorators is not working, and one must override it as a whole.

It would be great if you can split these decorators into two parts:

  1. function or class that prepares response body (python datastructure)
  2. function that builds flask response (jsonify, update headers, etc)

So example may look like this:

"""Exception handler"""

from flask import jsonify, current_app
from werkzeug.exceptions import HTTPException, InternalServerError


def prepare_error(error) -> dict:
    payload = {
        'status': str(error),
    }

    # Get additional info passed as kwargs when calling abort
    # data may not exist if HTTPException was raised not using webargs abort
    # or if not kwargs were passed (https://github.com/sloria/webargs/pull/184)
    data = getattr(error, 'data', None)
    if data:
        # If we passed a custom message
        if 'message' in data:
            payload['message'] = data['message']
        # If we passed "errors"
        if 'errors' in data:
            payload['errors'] = data['errors']
        # If webargs added validation errors as "messages"
        # (you should use 'errors' as it is more explicit)
        elif 'messages' in data:
            payload['errors'] = data['messages']
        # If we passed additional headers
        if 'headers' in data:
            headers = data['headers']

    return payload


def handle_http_exception(error, prepare_func):
    log_info = True

    if not isinstance(error, HTTPException):
        error = InternalServerError()
        # Flask logs uncaught exceptions as ERROR already, no need to log here
        log_info = False

    headers = {}

    payload = prepare_func(error)

    # Log error as INFO, including payload content
    if log_info:
        log_string_content = [str(error.code), ]
        for key in ('message', 'errors'):
            if key in payload:
                log_string_content.append(str(payload[key]))
        current_app.logger.info(' '.join(log_string_content))

    return jsonify(payload), error.code, headers


# This is part of App.init_app method
    ...py

    for code in default_exceptions:
        # App may define prepare_error_response function as prepare_func above.
        # Same would go for prepare_success_response
        error_handler = partial(handle_http_exception, prepare_func=self.prepare_error_response)
        app.register_error_handler(code, handle_http_exception)


     @staticmethod
     def prepare_error_response():
           return error_handler.prepare_error

So basically payload generation is split from flask response generation, so to implement custom logic, one just need to override Api.prepare_error_response and Api.prepare_success_response methods.

@lafrech
Copy link
Member

lafrech commented Sep 23, 2018

This makes total sense.

I'll be looking into this. (Can't promise when.)

@asyncee
Copy link
Author

asyncee commented Sep 23, 2018

That is just great, thank you very much.

@lafrech
Copy link
Member

lafrech commented Sep 24, 2018

Did you also manage to get the docs to correctly expose the wrapping structure?

@asyncee
Copy link
Author

asyncee commented Sep 24, 2018

Nope, i missed this part, sorry. I think that currently available doc-generating machinery could be in use without changes if prepare_error_response and prepare_success_response will define it's marshmallow schema for wrapping response.

@lafrech
Copy link
Member

lafrech commented Sep 26, 2018

@asyncee, I gave it a try. I took a different approach. I'm not 100% happy with it yet. It looks a bit awkward, but fonctionally, it seems to provide flexibility.

https://github.com/Nobatek/flask-rest-api/tree/dev_rework_response_and_error

Please tell me what you think.

Note we still lack the possibility to define error response schema. This is a TODO, independent from this feature.

@asyncee
Copy link
Author

asyncee commented Sep 26, 2018

I have quickly read the code and think it will make it. I will check it out in more detail in a day or two and post back.

Thanks!

@asyncee
Copy link
Author

asyncee commented Sep 26, 2018

It works for me.

This is my code (i removed all comments to make it cleaner):

class ReworkApi(Api):

    @staticmethod
    def _prepare_error_reponse_content(error):

        headers = {}
        payload = {'status': str(error), 'type': 'error'}

        # Note: added <or getattr(error, 'description')> to get description from HTTPException
        data = getattr(error, 'data', None) or getattr(error, 'description')
        if data:
            if 'message' in data:
                payload['message'] = data['message']
            if 'errors' in data:
                payload['errors'] = data['errors']
            elif 'messages' in data:
                payload['errors'] = data['messages']
            if 'headers' in data:
                headers = data['headers']

        return payload, headers


class ReworkBlueprint(Blueprint):
    @staticmethod
    def _make_doc_response_schema(schema):
        if not schema:
            return

        class WrappingSchema(ma.Schema):
            type = ma.fields.String(required=True)
            data = ma.fields.Nested(schema)

        return WrappingSchema()

    @staticmethod
    def _make_response(data):
        data = {"type": "success", "data": data}
        return jsonify(data)

It gives me such documentation examples:

2018-09-26_14-55-14

image

Thank you for your efforts!

Although it looks like there are hidden entity, ResponseHandler or something like this. New Api and Blueprint mixins in fact solve same problem — prepare response and format payload, so it looks like this functionality may be extracted to specific class, so if one want to tune responses it could be done from one single place in the system.

Anyway, the code is usable for my specific task.

@lafrech
Copy link
Member

lafrech commented Sep 28, 2018

Regarding the response schema, in you case, I think I would have sloppily wrapped all schemas and passed wrapped schemas to existing response decorator. This can be done with a factory that takes the original schema and returns the wrapped one.

But yes, with the rework, it is more elegantly solved at framework level.

Although it looks like there are hidden entity, ResponseHandler or something like this. New Api and Blueprint mixins in fact solve same problem — prepare response and format payload, so it looks like this functionality may be extracted to specific class, so if one want to tune responses it could be done from one single place in the system.

Perhaps. I get your point but I'm not sure it would be practically doable. But I'm open to refactor suggestions.

I've pushed in the same branch a similar refactor of the pagination decorator. I'm happy with how it looks. It should be merge soon. I need to update the docs first. Time to provide insights if you get the time.

Thanks.

@asyncee
Copy link
Author

asyncee commented Sep 28, 2018

Great to hear! I think it is okay to use current code version and to not to push deeper (yet).

I think issue can be closed after release, i will integrate the code into existing project and test it, it may take some time.

Anyway for me this project is the best (compared to more mature projects), because it uses marshmallow and saves time on writing documentation.

Thanks again!

@lafrech
Copy link
Member

lafrech commented Sep 29, 2018

I just pushed the whole rework into master branch. I shall release 0.9.0 soonish.

@lafrech
Copy link
Member

lafrech commented Oct 1, 2018

0.9.0 released!

@lafrech lafrech closed this as completed Oct 1, 2018
@asyncee
Copy link
Author

asyncee commented Oct 2, 2018

Great to hear, thanks!

@azzamsa
Copy link

azzamsa commented Jun 25, 2020

Then how to envelope our response?

I don't find any clue in the docs: https://flask-smorest.readthedocs.io/en/latest/response.html

I tried, to extend my Api class with the code above, but didn't work:

"""Api extension initialization

Override base classes here to allow painless customization in the future.
"""
import marshmallow as ma
import flask_smorest as flasksm
from flask import jsonify


class Blueprint(flasksm.Blueprint):
    """Blueprint override"""

    @staticmethod
    def _make_doc_response_schema(schema):
        if not schema:
            return

        class WrappingSchema(ma.Schema):
            type = ma.fields.String(required=True)
            data = ma.fields.Nested(schema)

        return WrappingSchema()

    @staticmethod
    def _make_response(data):
        data = {"type": "success", "data": data}
        return jsonify(data)


class Api(flasksm.Api):
    """Api override"""

    def __init__(self, app=None, *, spec_kwargs=None):
        super().__init__(app, spec_kwargs=spec_kwargs)

    @staticmethod
    def _prepare_error_reponse_content(error):

        headers = {}
        payload = {"status": str(error), "type": "error"}

        # Note: added <or getattr(error, 'description')> to get description from HTTPException
        data = getattr(error, "data", None) or getattr(error, "description")
        if data:
            if "message" in data:
                payload["message"] = data["message"]
            if "errors" in data:
                payload["errors"] = data["errors"]
            elif "messages" in data:
                payload["errors"] = data["messages"]
            if "headers" in data:
                headers = data["headers"]

        return payload, headers

        # Register custom Marshmallow fields in doc
        # self.register_field(CustomField, 'type', 'format')

        # Register custom Flask url parameter converters in doc
        # self.register_converter(CustomConverter, 'type', 'format')


class Schema(ma.Schema):
    """Schema override"""

My other attemp is using marsmallow @post_dump, but will litter the model in generated swagger doc.

thank you

@azzamsa
Copy link

azzamsa commented Jun 25, 2020

solution:

The correct way to extends _prepare_response_content is not under class Api(flasksm.Api): but class Blueprint(flasksm.Blueprint)

class Blueprint(flasksm.Blueprint):
    """Blueprint override"""

    @staticmethod
    def _prepare_response_content(data):
        if data is not None:
            return {"data": data}
        return None


class Api(flasksm.Api):
    """Api override"""

    def __init__(self, app=None, *, spec_kwargs=None):
        super().__init__(app, spec_kwargs=spec_kwargs)


class Schema(ma.Schema):
    """Schema override"""

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

No branches or pull requests

3 participants