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

broken interface normalize_path_middleware #3669

Closed
vir-mir opened this issue Mar 27, 2019 · 8 comments
Closed

broken interface normalize_path_middleware #3669

vir-mir opened this issue Mar 27, 2019 · 8 comments

Comments

@vir-mir
Copy link
Member

vir-mir commented Mar 27, 2019

Long story short

middleware normalize_path_middleware is not working properly

Expected behaviour

I appeal to /paymethods, there is an endpoint /paymethod

/paymethod -> 200
/paymethods -> 301 -> /paymethod

settings: normalize_path_middleware(remove_slash=True, append_slash=False)

if switch off
/paymethod -> 200
/paymethods -> 404

I think the problem is in the last if merge_slashes and remove_slash

            if merge_slashes:
                paths_to_check.append(re.sub('//+', '/', path))
            if append_slash and not request.path.endswith('/'):
                paths_to_check.append(path + '/')
            if remove_slash and request.path.endswith('/'):
                paths_to_check.append(path[:-1])
            if merge_slashes and append_slash:
                paths_to_check.append(
                    re.sub('//+', '/', path + '/'))
            if merge_slashes and remove_slash:
                merged_slashes = re.sub('//+', '/', path)
                paths_to_check.append(merged_slashes[:-1])

aiohttp == 3.5.4

Your environment

server

@aio-libs-bot
Copy link

GitMate.io thinks the contributor most likely able to help you is @asvetlov.

Possibly related issues are #3086 (add_static breaks normalize_path_middleware), #1178 (Websocket interface change break existing code), #1292 (broken interface for response.url), #1583 (normalize_path_middleware tests failures), and #445 (Is break in finally intentional?).

@bigbag
Copy link

bigbag commented Mar 27, 2019

if we change last condition
if merge_slashes and remove_slash:
on
if merge_slashes and remove_slash and path.endswith('/'):
all will be correct.

@kxepal
Copy link
Member

kxepal commented Mar 27, 2019

async def test_bug_3669(aiohttp_client):
    async def paymethod(request):
        return web.Response(text="OK")

    async def paymethods(request):
        raise web.HTTPPermanentRedirect('/paymethod')

    app = web.Application()
    app.router.add_route('GET', '/paymethods', paymethods)
    app.router.add_route('GET', '/paymethod', paymethod)
    app.middlewares.append(
        web.normalize_path_middleware(append_slash=False, remove_slash=True)
    )

    client = await aiohttp_client(app, server_kwargs={'skip_url_asserts': True})

    resp = await client.get('/paymethods')
    assert resp.status == 200
    assert resp.url.path == '/paymethod'

@vir-mir, this test case passed. How is it different from your's case?

@vir-mir
Copy link
Member Author

vir-mir commented Mar 27, 2019

@kxepal Why did the final URL in the test change?

you send /paymethods
assert resp.url.path == '/paymethod'

@kxepal
Copy link
Member

kxepal commented Mar 27, 2019

@vir-mir Because of redirect? Eventually, it causes new request to new location.

@vir-mir
Copy link
Member Author

vir-mir commented Mar 27, 2019

async def test_bug_3669(aiohttp_client):
    async def paymethod(request):
        return web.Response(text="OK")

    app = web.Application()
    app.router.add_route('GET', '/paymethod', paymethod)
    app.middlewares.append(
        web.normalize_path_middleware(append_slash=False, remove_slash=True)
    )

    client = await aiohttp_client(app, server_kwargs={'skip_url_asserts': True})

    resp = await client.get('/paymethods')
    assert resp.url.path == '/paymethod'

@kxepal
Copy link
Member

kxepal commented Mar 27, 2019

So no redirect involved. That's make a sense. Thanks!

@kxepal
Copy link
Member

kxepal commented Mar 27, 2019

Hm..autoclose failed. Fixed in master via 8260eda

@kxepal kxepal closed this as completed Mar 27, 2019
@lock lock bot added the outdated label Mar 27, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants