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

Check context's type and return context if it is not a dict #15

Merged
merged 6 commits into from
Jul 9, 2015

Conversation

tark-hidden
Copy link
Contributor

@aiohttp_jinja2.template('index.jinja2')
def check(request):
    param = request.match_info['param']
    if param == 'first':
        # success
        return dict(text='First case')
    elif param == 'second':
        # success
        return dict(text='Second case', user='Tark')
    elif param == 'another':
        # fail!
        # context should be mapping, not <class 'aiohttp.web_reqrep.Response'>
        return aiohttp_jinja2.render_template(
            'another.jinja2', request=request, context=dict(user='Tark', sum=0))
    elif param == 'json':
        s = dict(text='JSON', user='Tark')
        # fail!
        # context should be mapping, not <class 'aiohttp.web_reqrep.Response'>
        return web.Response(body=bytes(json.dumps(s), encoding='utf-8'),
                            content_type='application/json')
    elif param == 'redirect':
        # fail!
        # context should be mapping, not <class 'aiohttp.web_exceptions.HTTPFound'>
        return HTTPFound('http://google.com')
    return dict(text='Hello!')

It can be in a real-world project. @aiohttp_jinja2.template has been used for a few lines shorter code.
But with checking context's type in template decorator it will work and http://stackoverflow.com/questions/29322753/aiohttp-and-aiohttp-jinja2-response-error will never be asked.

@asvetlov
Copy link
Member

asvetlov commented Jul 9, 2015

  1. Your check is not correct, collections.abc.Mapping is not equal to dict.
  2. Currently aiohttp_jinja2 has strict checking and I don't want to relax it. At least for now.
  3. Please run test suite before publishing pull request :)

@asvetlov asvetlov closed this Jul 9, 2015
@tark-hidden
Copy link
Contributor Author

  1. My bad. I thought it was exactly a dict.

  2. Now with template decorator you cannot even do a redirect. It is usual case, I guess.

  3. You are right, my bad again.

        if isinstance(context, web.Response):
            return context
    

Test passed!

@asvetlov asvetlov reopened this Jul 9, 2015
@asvetlov
Copy link
Member

asvetlov commented Jul 9, 2015

Well, redirection is good point!
As well as raising HTTPExceptions deep inside web-handler calling chain.

@@ -80,6 +80,9 @@ def wrapped(*args):
else:
coro = asyncio.coroutine(func)
context = yield from coro(*args)
if isinstance(context, web.Response):
Copy link
Member

Choose a reason for hiding this comment

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

Should be web.StreamResponse -- top class in responses hierarchy.

@asvetlov
Copy link
Member

asvetlov commented Jul 9, 2015

Tests are required for any pull request.

@tark-hidden
Copy link
Contributor Author

I don't really know what should I check here... Test for checking context is response is ready.

@asvetlov
Copy link
Member

asvetlov commented Jul 9, 2015

Don't do redirect, return HTTPForbidden instead and check for status code.

asvetlov added a commit that referenced this pull request Jul 9, 2015
Check context's type and return context if it is not a dict
@asvetlov asvetlov merged commit bc3331d into aio-libs:master Jul 9, 2015
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.

2 participants