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

should improve the representations behavior #72

Closed
hoatle opened this Issue Sep 27, 2017 · 8 comments

Comments

Projects
None yet
3 participants
@hoatle
Member

hoatle commented Sep 27, 2017

hjc 6 hours ago
Couple things. First off, to handle a missing content type from representations we should either throw a specific exception or provide the usage of like a default key in the representations dictionary that is a catch-all function that can always be used. Alternatively, we should just return the data we were given. I don't like using the first key in the dictionary, because dictionary ordering isn't guaranteed until Py3.6, in earlier versions you'd get a random key every time and that sounds SUPER frustrating.

from: https://github.com/teracyhq/flask-classful/pull/71/files#r141177560

@hoatle

This comment has been minimized.

Show comment
Hide comment
@hoatle

hoatle Sep 27, 2017

Member

Also, can we define "find the best match"? For example, if I pass it application/json as my content type, will it look for application/json, json, and application in that order? Or just application/json?

from https://github.com/teracyhq/flask-classful/pull/71/files#r141177560

Member

hoatle commented Sep 27, 2017

Also, can we define "find the best match"? For example, if I pass it application/json as my content type, will it look for application/json, json, and application in that order? Or just application/json?

from https://github.com/teracyhq/flask-classful/pull/71/files#r141177560

@hoatle

This comment has been minimized.

Show comment
Hide comment
@hoatle

hoatle Sep 27, 2017

Member

@hjc let's continue to improve the representations behavior here.

Alternatively, we should just return the data we were given. I don't like using the first key in the dictionary, because dictionary ordering isn't guaranteed until Py3.6, in earlier versions you'd get a random key every time and that sounds SUPER frustrating.

I agree, let's just return the data we were given.

...
Otherwise, ``Flask-Classful`` will try to find the best match between the accepted content
type and the keys in the ``representations`` dictionary, and call the associated output proxy
function to create a ``flask.wrappers.ResponseBase`` instance.
...

=>

...
Otherwise, ``Flask-Classful`` will try to find the best match between the accepted content
type and the keys in the ``representations`` dictionary, and call the associated output proxy
function to create a ``flask.wrappers.ResponseBase`` instance. The best match finding is the
behavior of `werkzeug.datastructures.Accept#best_match`:  "Returns the best match from a
list of possible matches based on the quality of the client. If two items have the same quality,
the one is returned that comes first."
...

Is this doc ok?

Member

hoatle commented Sep 27, 2017

@hjc let's continue to improve the representations behavior here.

Alternatively, we should just return the data we were given. I don't like using the first key in the dictionary, because dictionary ordering isn't guaranteed until Py3.6, in earlier versions you'd get a random key every time and that sounds SUPER frustrating.

I agree, let's just return the data we were given.

...
Otherwise, ``Flask-Classful`` will try to find the best match between the accepted content
type and the keys in the ``representations`` dictionary, and call the associated output proxy
function to create a ``flask.wrappers.ResponseBase`` instance.
...

=>

...
Otherwise, ``Flask-Classful`` will try to find the best match between the accepted content
type and the keys in the ``representations`` dictionary, and call the associated output proxy
function to create a ``flask.wrappers.ResponseBase`` instance. The best match finding is the
behavior of `werkzeug.datastructures.Accept#best_match`:  "Returns the best match from a
list of possible matches based on the quality of the client. If two items have the same quality,
the one is returned that comes first."
...

Is this doc ok?

@hoatle

This comment has been minimized.

Show comment
Hide comment
@hoatle

hoatle Sep 27, 2017

Member

@hjc if you could create a PR for this, it would be fantastic :-)

Member

hoatle commented Sep 27, 2017

@hjc if you could create a PR for this, it would be fantastic :-)

@hoatle

This comment has been minimized.

Show comment
Hide comment
@hoatle

hoatle Sep 27, 2017

Member

@hjc btw, I added you as a collaborator for this project, hopefully, you can help to boost this project development. We need more collaborators for the project.

Member

hoatle commented Sep 27, 2017

@hjc btw, I added you as a collaborator for this project, hopefully, you can help to boost this project development. We need more collaborators for the project.

@hjc

This comment has been minimized.

Show comment
Hide comment
@hjc

hjc Sep 28, 2017

Collaborator

@hoatle - totally agreed about just returning what we were given. Re: the docs from Werkzeug itself, I'm not sure those are clear enough. I'm going to play with that method and see what it really does and then define that. With the way it's written it mentions "quality of the client" but then never really defines what it means by that. The way I read that is that it will try progressively more permissive mime types based on what the client accepts. E.g., if we try to return application/json but the client doesn't accept that, then this will probably try to return text/html or something like that. I also don't know how or when that method is used... so, I could be completely wrong.

I'll take a look at this either tomorrow or over the weekend and open a PR with docs, tests, and a fix. I've also accepted the collaborator invitation and am happy to help out as I can!

Collaborator

hjc commented Sep 28, 2017

@hoatle - totally agreed about just returning what we were given. Re: the docs from Werkzeug itself, I'm not sure those are clear enough. I'm going to play with that method and see what it really does and then define that. With the way it's written it mentions "quality of the client" but then never really defines what it means by that. The way I read that is that it will try progressively more permissive mime types based on what the client accepts. E.g., if we try to return application/json but the client doesn't accept that, then this will probably try to return text/html or something like that. I also don't know how or when that method is used... so, I could be completely wrong.

I'll take a look at this either tomorrow or over the weekend and open a PR with docs, tests, and a fix. I've also accepted the collaborator invitation and am happy to help out as I can!

@hoatle hoatle added stag:in-progress and removed sprt:next labels Sep 30, 2017

@hoatle hoatle assigned hjc and unassigned hoatle Sep 30, 2017

hjc added a commit that referenced this issue Oct 4, 2017

hjc added a commit that referenced this issue Oct 4, 2017

@#72: New default representation handler logic.
Use flask-classful/default for it.

hjc added a commit that referenced this issue Oct 4, 2017

@hjc

This comment has been minimized.

Show comment
Hide comment
@hjc

hjc Oct 4, 2017

Collaborator

@hoatle - see #74 for this work.

Collaborator

hjc commented Oct 4, 2017

@hoatle - see #74 for this work.

@hoatle hoatle closed this in #74 Oct 5, 2017

hoatle added a commit that referenced this issue Oct 5, 2017

@wafflebot wafflebot bot removed the stag:under-review label Oct 5, 2017

@hoatle hoatle added the reso:fixed label Oct 5, 2017

@cript0nauta

This comment has been minimized.

Show comment
Hide comment
@cript0nauta

cript0nauta Oct 11, 2017

Hi! I noted that this fix broke backwards compatibility with the tests of my app. I wrote the following code to demonstrate the issue:

import json
from flask import Flask, make_response
from flask_classful import FlaskView

app = Flask(__name__)

def output_json(data, code, headers=None):
    content_type = 'application/json'
    dumped = json.dumps(data)
    if headers:
        headers.update({'Content-Type': content_type})
    else:
        headers = {'Content-Type': content_type}
    response = make_response(dumped, code, headers)
    return response

class QuotesView(FlaskView):
    representations = {
        'application/json': output_json,
        # 'flask-classful/default': output_json,  # I had to add this to fix it
    }
    def index(self):
        return {"hello": "world"}

QuotesView.register(app)

if __name__ == '__main__':

    # app.run(debug=True)

    client = app.test_client()
    res = client.get('/quotes/')
    assert res.status_code == 200
    assert res.headers['Content-Type'] == 'application/json'
    assert json.loads(res.data) == {'hello': 'world'}
    print 'ALL OK'

This will print ALL OK before this commit, and cause and ugly exception after this commit.

Because now it doesn't use the first representation by default, it makes a response with a dict instead of a string with the encoded JSON. This caused a really ugly bug that (strangely) only showed up in the test cases, and was quite difficult to debug. I had to get in flask and werkzeugs internals, and to do a git bisect to the flask-classful repo in order to find the cause of the bug.

The fix was to add a flask-classful/default representation with output_json, as commented in the code above. I think that this change should be better documented in the changelog or in a migration guide to avoid others to have problems with this.

This could also be fixed on the second code block of this section in the docs: http://flask-classful.teracy.org/#adding-resource-representations-get-real-classy-and-put-on-a-top-hat, since that example code has the issue I detailed above. It comments the fix a few paragraphs below, but in my opinion this isn't enough.

cript0nauta commented Oct 11, 2017

Hi! I noted that this fix broke backwards compatibility with the tests of my app. I wrote the following code to demonstrate the issue:

import json
from flask import Flask, make_response
from flask_classful import FlaskView

app = Flask(__name__)

def output_json(data, code, headers=None):
    content_type = 'application/json'
    dumped = json.dumps(data)
    if headers:
        headers.update({'Content-Type': content_type})
    else:
        headers = {'Content-Type': content_type}
    response = make_response(dumped, code, headers)
    return response

class QuotesView(FlaskView):
    representations = {
        'application/json': output_json,
        # 'flask-classful/default': output_json,  # I had to add this to fix it
    }
    def index(self):
        return {"hello": "world"}

QuotesView.register(app)

if __name__ == '__main__':

    # app.run(debug=True)

    client = app.test_client()
    res = client.get('/quotes/')
    assert res.status_code == 200
    assert res.headers['Content-Type'] == 'application/json'
    assert json.loads(res.data) == {'hello': 'world'}
    print 'ALL OK'

This will print ALL OK before this commit, and cause and ugly exception after this commit.

Because now it doesn't use the first representation by default, it makes a response with a dict instead of a string with the encoded JSON. This caused a really ugly bug that (strangely) only showed up in the test cases, and was quite difficult to debug. I had to get in flask and werkzeugs internals, and to do a git bisect to the flask-classful repo in order to find the cause of the bug.

The fix was to add a flask-classful/default representation with output_json, as commented in the code above. I think that this change should be better documented in the changelog or in a migration guide to avoid others to have problems with this.

This could also be fixed on the second code block of this section in the docs: http://flask-classful.teracy.org/#adding-resource-representations-get-real-classy-and-put-on-a-top-hat, since that example code has the issue I detailed above. It comments the fix a few paragraphs below, but in my opinion this isn't enough.

@hoatle

This comment has been minimized.

Show comment
Hide comment
@hoatle

hoatle Oct 12, 2017

Member

@sh4r3m4n thank you for your information, let me find all the breaking changes and add to the migration sections on docs and change log then. There are two breaking changes that I can see:

  • #47
    and this one.

this issue should fix the problem: #79

Member

hoatle commented Oct 12, 2017

@sh4r3m4n thank you for your information, let me find all the breaking changes and add to the migration sections on docs and change log then. There are two breaking changes that I can see:

  • #47
    and this one.

this issue should fix the problem: #79

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