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

add route test for r3_tree_insert_route #86

Closed
wants to merge 1 commit into from
Closed

add route test for r3_tree_insert_route #86

wants to merge 1 commit into from

Conversation

coney
Copy link

@coney coney commented Oct 18, 2015

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)

@coney coney mentioned this pull request Oct 18, 2015
@c9s
Copy link
Owner

c9s commented Oct 18, 2015

Try put the shortest path at first position?

@coney
Copy link
Author

coney commented Oct 18, 2015

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
Copy link
Owner

c9s commented Oct 18, 2015

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

@c9s
Copy link
Owner

c9s commented Oct 18, 2015

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

Does your insert function return value?

@coney
Copy link
Author

coney commented Oct 21, 2015

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

@coney
Copy link
Author

coney commented Oct 25, 2015

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
Copy link
Author

coney commented Oct 25, 2015

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
Copy link
Owner

c9s commented Oct 25, 2015

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
Copy link
Author

coney commented Oct 25, 2015

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
Copy link
Owner

c9s commented Oct 25, 2015

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

#75

@c9s
Copy link
Owner

c9s commented Oct 25, 2015

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
Copy link
Owner

c9s commented Oct 25, 2015

still can not match

still can not match which path? the root / ?

@coney
Copy link
Author

coney commented Oct 25, 2015

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

@c9s
Copy link
Owner

c9s commented Oct 25, 2015

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

@c9s c9s added the bug label Oct 25, 2015
@coney
Copy link
Author

coney commented Oct 25, 2015

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

@c9s
Copy link
Owner

c9s commented Oct 25, 2015

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

@coney
Copy link
Author

coney commented Oct 25, 2015

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 added a commit that referenced this pull request Nov 17, 2015
@c9s
Copy link
Owner

c9s commented Nov 17, 2015

Fixed on master

@c9s c9s closed this Nov 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants