c9s / r3

libr3 is a high-performance path dispatching library. It compiles your route paths into a prefix tree (trie). By using the constructed prefix trie in the start-up time, you may dispatch your routes with efficiency
http://c9s.github.com/r3/bench.html
MIT License
816 stars 83 forks source link

add route test for r3_tree_insert_route #86

Closed coney closed 8 years ago

coney commented 8 years ago
95%: Checks: 20, Failures: 1, Errors: 0
check_tree.c:738:F:testcase:test_insert_route:0: Assertion 'r != ((void*)0)' failed
FAIL check_tree (exit status: 1)
c9s commented 8 years ago

Try put the shortest path at first position?

coney commented 8 years ago

it works well if i put the "/" at the first position.

but this limitation is a bit annoying.

is it easy to fix(or improve) it?

c9s commented 8 years ago

Yeah, you can fix that in r3_treeinsert*, I think for existing node (root) should increase the mount_point by one.

c9s commented 8 years ago

Might be caused from here: https://github.com/c9s/r3/blob/master/src/node.c#L734

Does your insert function return value?

coney commented 8 years ago

sorry, i'm too busy these day, i will try it later

coney commented 8 years ago

I'm using r3_tree_insert_routel_ex to create nodes:

    route * r = r3_tree_insert_routel_ex((node*)tree, method, 
        path, strlen(path), data, errstr);
    printf("insert %s(data:%p) return %p\n", path, data, r);

when i insert / after all other routes, it returns a not-null value, but still can't match

insert /students(data:0x50dda0) return 0x50dea0
insert /students/{sid}(data:0x50f8f0) return 0x50f920
insert /students/{sid}/books(data:0x50fc30) return 0x50fed0
insert /students/{sid}/books(data:0x5109a0) return 0x5109d0
insert /students/{sid}/books/{bid}(data:0x510b80) return 0x510bb0
insert /students/{sid}/books/{bid}(data:0x510950) return 0x510fd0
insert /(data:0x511280) return 0x50fe90

dumped tree looks like this:

(o) endpoint:0
  |-"/"
  (o) endpoint:0
    |-"students"
    (o) endpoint:1 data:0x50dda0
      |-"/"
      (o) regexp:^([^/]+) endpoint:0
        |-"{sid}" opcode:3
        (o) endpoint:1 data:0x50f8f0
          |-"/books"
          (o) endpoint:2 data:0x50fc30
            |-"/"
            (o) regexp:^([^/]+) endpoint:0
              |-"{bid}" opcode:3
              (o) endpoint:2 data:0x510b80

    |-""
    (o) endpoint:1 data:0x511280
coney commented 8 years ago

If i put / in the first place, the result is

insert /(data:0x50dda0) return 0x50dea0
insert /students(data:0x50f8f0) return 0x50f920
insert /students/{sid}(data:0x50fb40) return 0x50fb70
insert /students/{sid}/books(data:0x510160) return 0x510190
insert /students/{sid}/books(data:0x510c40) return 0x510c70
insert /students/{sid}/books/{bid}(data:0x50fe60) return 0x510f00
insert /students/{sid}/books/{bid}(data:0x5112f0) return 0x511320

and the dump

(o) endpoint:0
  |-"/"
  (o) endpoint:1 data:0x50dda0
    |-"students"
    (o) endpoint:1 data:0x50f8f0
      |-"/"
      (o) regexp:^([^/]+) endpoint:0
        |-"{sid}" opcode:3
        (o) endpoint:1 data:0x50fb40
          |-"/books"
          (o) endpoint:2 data:0x510160
            |-"/"
            (o) regexp:^([^/]+) endpoint:0
              |-"{bid}" opcode:3
              (o) endpoint:2 data:0x50fe60
c9s commented 8 years ago

when i insert / after all other routes, it returns a not-null value, but still can't match

I don't understand. match which path?

coney commented 8 years ago

when i insert / after all other routes, it returns a not-null value, but still can't match I don't understand. match which path? still can not match

c9s commented 8 years ago

If you're using greedy pattern in the middle for different routes, please see this issue:

https://github.com/c9s/r3/issues/75

c9s commented 8 years ago

r3_tree_insert_routel_ex calls r3_tree_insert_path, they are the same except that r3_tree_insert_routel_ex set different data structure on the endpoint.

c9s commented 8 years ago

still can not match

still can not match which path? the root / ?

coney commented 8 years ago

yes, can't match the root uri, the rests are good

c9s commented 8 years ago

Another suspicion:

/students is branched into / and students successfully, however, it inserts the rest path "" under the root node, thus / doesn't have endpoint = 1.

Route matching require node's endpoint > 0

https://github.com/c9s/r3/blob/master/src/node.c#L717

coney commented 8 years ago

this PR simulates the problem, I add a new test case

c9s commented 8 years ago

Sorry I won't merge this until it has a fix, and I am too busy right now to fix this issue.

coney commented 8 years ago

putting the / in the top could solve this problem, it's not a fatal bug. this lib helps me a lot, it's very cool

c9s commented 8 years ago

Fixed on master