Code Monkey home page Code Monkey logo

Comments (10)

ashleysommer avatar ashleysommer commented on June 10, 2024

Immediately obvious solution would be:

def find_route(path, router, basket, extra):
    parts = tuple(path[1:].split(router.delimiter))
    num = len(parts)
    if num > 0:
        basket[0] = parts[0]
        if num == 2:
            if parts[1] == "bar":
                try:
                    basket['__params__']['foo'] = int(basket[0])
                except ValueError:
                    ...
                else:
                    basket['__raw_path__'] = '<foo:int>/bar'
                    return router.dynamic_routes[('<foo:int>', 'bar')], basket
            else:                #<- New else condition
                raise NotFound   #<- New raise NotFound
        try:
            basket['__params__']['foo'] = int(basket[0])
        except ValueError:
            ...
        else:
            basket['__raw_path__'] = '<foo:int>'
            return router.dynamic_routes[('<foo:int>',)], basket
    raise NotFound

But that seems like cheating, I like the idea that the logic falls through to the bottom for NotFound.

Maybe it should be this?:

def find_route(path, router, basket, extra):
    parts = tuple(path[1:].split(router.delimiter))
    num = len(parts)
    if num > 0:
        basket[0] = parts[0]
        if num == 1:
            try:
                basket['__params__']['foo'] = int(basket[0])
            except ValueError:
                ...
            else:
                basket['__raw_path__'] = '<foo:int>'
                return router.dynamic_routes[('<foo:int>',)], basket
        if num == 2:
            if parts[1] == "bar":
                try:
                    basket['__params__']['foo'] = int(basket[0])
                except ValueError:
                    ...
                else:
                    basket['__raw_path__'] = '<foo:int>/bar'
                    return router.dynamic_routes[('<foo:int>', 'bar')], basket
    raise NotFound

from sanic-routing.

ashleysommer avatar ashleysommer commented on June 10, 2024

Or is it just missing a single else?

def find_route(path, router, basket, extra):
    parts = tuple(path[1:].split(router.delimiter))
    num = len(parts)
    if num > 0:
        basket[0] = parts[0]
        if num == 2:
            if parts[1] == "bar":
                try:
                    basket['__params__']['foo'] = int(basket[0])
                except ValueError:
                    ...
                else:
                    basket['__raw_path__'] = '<foo:int>/bar'
                    return router.dynamic_routes[('<foo:int>', 'bar')], basket
        else:  #<-- single new else
            try:
                basket['__params__']['foo'] = int(basket[0])
            except ValueError:
                ...
            else:
                basket['__raw_path__'] = '<foo:int>'
                return router.dynamic_routes[('<foo:int>',)], basket
    raise NotFound

from sanic-routing.

ashleysommer avatar ashleysommer commented on June 10, 2024

Ok, I've been diving into the code and the logic within the router AST generation, and I understand it much better now.
It looks like the optimize() step where it normally injects raise NotFound locations is missing one, the find_route_src should look like this:

def find_route(path, router, basket, extra):
    parts = tuple(path[1:].split(router.delimiter))
    num = len(parts)
    if num > 0:
        basket[0] = parts[0]
        if num == 2:
            if parts[1] == "bar":
                try:
                    basket['__params__']['foo'] = int(basket[0])
                except ValueError:
                    ...
                else:
                    basket['__raw_path__'] = '<foo:int>/bar'
                    return router.dynamic_routes[('<foo:int>', 'bar')], basket
            raise NotFound   #<- New raise NotFound
        try:
            basket['__params__']['foo'] = int(basket[0])
        except ValueError:
            ...
        else:
            basket['__raw_path__'] = '<foo:int>'
            return router.dynamic_routes[('<foo:int>',)], basket
    raise NotFound

But thinking about it further, that won't fix the case where the path is /0/aaa/bbb because in that case num==3, so it falls through to the bucket[0] parameter conversion.

from sanic-routing.

ashleysommer avatar ashleysommer commented on June 10, 2024

I've narrowed down a test which covers all of the concerns above:

def test_use_route_bug16():
    router = Router()
    def h1(foo):
        return "first"
    router.add("/test/<foo:int>", h1)
    def h2(foo):
        return "second"
    router.add("/test/<foo:int>/bar", h2)

    router.finalize()
    with pytest.raises(NotFound):
        (route, handler, params) = router.get("/test/foo/aaaa", "BASE")
    with pytest.raises(NotFound):
        (route, handler, params) = router.get("/test/0/aaaa", "BASE")
    with pytest.raises(NotFound):
        (route, handler, params) = router.get("/test/0/aaaa/bbbb", "BASE")

from sanic-routing.

ahopkins avatar ahopkins commented on June 10, 2024

Nice work, I will take a look later. It feels to me like you mentioned that the problem is in the optimize() method.

from sanic-routing.

ahopkins avatar ahopkins commented on June 10, 2024

I am wondering if we need to do this:

def find_route(path, router, basket, extra):
    parts = tuple(path[1:].split(router.delimiter))
    num = len(parts)
    if num > 0:
        basket[0] = parts[0]
        if num > 2:   #<- New condition
            raise NotFound   #<- New raise NotFound
        elif num == 2:   #<- Changed from if
            if parts[1] == "bar":
                try:
                    basket['__params__']['foo'] = int(basket[0])
                except ValueError:
                    ...
                else:
                    basket['__raw_path__'] = '<foo:int>/bar'
                    return router.dynamic_routes[('<foo:int>', 'bar')], basket
            raise NotFound   #<- New raise NotFound
        try:
            basket['__params__']['foo'] = int(basket[0])
        except ValueError:
            ...
        else:
            basket['__raw_path__'] = '<foo:int>'
            return router.dynamic_routes[('<foo:int>',)], basket
    raise NotFound

from sanic-routing.

ashleysommer avatar ashleysommer commented on June 10, 2024

Yep, I think it's actually two different bugs here on top of each other.

That fix would take care of both of them, but does introduce a new if statement into the route, which may hurt router performance a tiny bit.

from sanic-routing.

ahopkins avatar ahopkins commented on June 10, 2024

That fix would take care of both of them, but does introduce a new if statement into the route, which may hurt router performance a tiny bit.

Yup, which is something that I want to play with still.

Originally, it only ever used > as an operator. But, then I realized that we may want to bail out and use == if there is nothing deeper in the node chain. We potentially could go back, and remove this:

if (
    self.last
    and self.route
    and not self.children
    and not self.route.requirements
):

Or, at least limit it even further in scope where there is nothing at the parent node level as a fallback.

So, in this case it would be:

    if num > 0:
        basket[0] = parts[0]
        if num > 2:   #<- Revert condition so it is not applicable since there is the parent level fallback
            if parts[1] == "bar":
                ...
            raise NotFound   #<- New raise NotFound
        ...

from sanic-routing.

ashleysommer avatar ashleysommer commented on June 10, 2024

I was playing yesterday with changing the logic to use >= for all of the comparisons up the chain, then != for NotFound at the end. But I couldn't get it to work quite how I wanted it, I don't have enough experience with the tree rendering code.

from sanic-routing.

ahopkins avatar ahopkins commented on June 10, 2024

I was playing yesterday with changing the logic to use >= for all of the comparisons up the chain, then != for NotFound at the end. But I couldn't get it to work quite how I wanted it, I don't have enough experience with the tree rendering code.

I'll add some comments so it will be easier to follow and we can start iterating/improving.

from sanic-routing.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.