search

Stiffstream / restinio

Cross-platform, efficient, customizable, and robust asynchronous HTTP(S)/WebSocket server C++ library with the right balance between performance and ease of use
Other
1.13k stars 93 forks source link

Update path2regex (again) #169

Closed Lord-Kamina closed 1 year ago

Lord-Kamina commented 1 year ago

@eao197 it appears the previous PR had some sort of weird merge issue, because the code was different from what I'd pushed to the performous fork. It should be fixed. If there are still problems with it, could you just let me know but not close the PR, so I can work to fix them?

Most odd was that optional-lite error you posted about, because I don't really know how that related to the PR, or where that issue was coming from.

Original PR description below:

Updated the path2regex code to be in line with the latest version of the javascript code, this version does not rely on std::cregex_iterator to create the matching pattern., and thus solves #166

I haven't tested it too extensively, but I did test it with some of the examples from the wiki, as well as in the context of performous, and it appears to be working as expected.

Of note: This version does not allow modifiers outside a capturing group, so for example to match anything the route would be "(.*)" instead of ".*"

Lord-Kamina commented 1 year ago

I tested this under c++17 initially. Have reproduced the errors you've mentioned under c++14. They evidently have something to do with differences between the real optional/string_view and the nonstd versions.

Will update once I figure how to get it fixed.

Lord-Kamina commented 1 year ago

I just compiled it under c++14 and it seems to be working properly now.

The optional-lite error somehow came about because I did optional_t<const std::string> instead of optional_t<std::string>. There was also another error because it appears I can't use += between std::string and nonstd::string_view, both of these cases worked fine under c++17, which is why I'm assuming it had something to do with the nonstd versions.

eao197 commented 1 year ago

I'm sorry, but this PR can't be accepted too because it breaks the compatibility with existing behavior:

It's a pity that you spent some of your time for code that can't be merged into RESTinio-0.6. Maybe it's better to provide a minimal and self-sufficient example that shows a problem from #166 and we'll try to modify our implementation without breaking the existing behavior?

Lord-Kamina commented 1 year ago

I'm sorry, but this PR can't be accepted too because it breaks the compatibility with existing behavior:

  • there is no more options_t::delimiters(), you replaced it by a new method options_t::prefixes(). It will break code for users that call options_t::delimiters();

I did this because it's what that field is called in the original code, and it took me quite a while to understand that. Your version has options_t::delimiter and options_t::delimiters (which came about because in your code, you merged two separate structs to create options_t), which is kinda confusing. Maybe an acceptable workaround would be to make the change, but keep options_t::delimiters as a (deprecated) alias?

  • your implementation doesn't pass our tests (at least test/router/express reports 31 broken assertions). It means that someone can find his/her routes not working anymore.

Could you give me details on this?

It's a pity that you spent some of your time for code that can't be merged into RESTinio-0.6. Maybe it's better to provide a minimal and self-sufficient example that shows a problem from #166 and we'll try to modify our implementation without breaking the existing behavior?

You can't fix it without changing it fundamentally, because I don't think it's really a problem with your code itself but rather with std::cregex_iterator, which your implementation is based around. It's as simple as, if you create a cregex_iterator after calling std::locale::global, it behaves in unexpected ways.

I am willing to keep working on this until it can be merged, but the way you manage PRs is very awkward and makes it difficult to do so, because I need to make a new PR whenever I make new changes.

eao197 commented 1 year ago

Could you give me details on this?

There is the output from the failed test:

-------------------------------------------------------------------------------
Original tests #7
-------------------------------------------------------------------------------
test/router/express/original_tests_part1.ipp:360
...............................................................................                                          

test/router/express/original_tests_part1.ipp:378: FAILED:
  REQUIRE( rm.match_route( target_path, params ) )
with expansion:
  false

-------------------------------------------------------------------------------
Original tests #8
-------------------------------------------------------------------------------
test/router/express/original_tests_part1.ipp:434
...............................................................................                                          

test/router/express/original_tests_part1.ipp:476: FAILED:
  REQUIRE( params.match() == R"match(/test//)match" )
with expansion:
  /test/ == "/test//"

-------------------------------------------------------------------------------
Original tests #9
-------------------------------------------------------------------------------
test/router/express/original_tests_part1.ipp:508
...............................................................................                                          

test/router/express/original_tests_part1.ipp:526: FAILED:
  REQUIRE( rm.match_route( target_path, params ) )
with expansion:
  false

-------------------------------------------------------------------------------
Original tests #11
-------------------------------------------------------------------------------
test/router/express/original_tests_part1.ipp:591
...............................................................................                                          

test/router/express/original_tests_part1.ipp:609: FAILED:
  REQUIRE( rm.match_route( target_path, params ) )
with expansion:
  false

-------------------------------------------------------------------------------
Original tests #13
-------------------------------------------------------------------------------
test/router/express/original_tests_part1.ipp:732
...............................................................................                                          

test/router/express/original_tests_part1.ipp:750: FAILED:
  REQUIRE( rm.match_route( target_path, params ) )
with expansion:
  false

-------------------------------------------------------------------------------
Original tests #14
-------------------------------------------------------------------------------
test/router/express/original_tests_part1.ipp:790
...............................................................................                                          

test/router/express/original_tests_part1.ipp:808: FAILED:
  REQUIRE( rm.match_route( target_path, params ) )
with expansion:
  false

-------------------------------------------------------------------------------
Original tests #21
-------------------------------------------------------------------------------
test/router/express/original_tests_part2.ipp:50
...............................................................................                                          

test/router/express/original_tests_part2.ipp:68: FAILED:
  REQUIRE( rm.match_route( target_path, params ) )
with expansion:
  false

-------------------------------------------------------------------------------
Original tests #26
-------------------------------------------------------------------------------
test/router/express/original_tests_part2.ipp:339
...............................................................................                                          

test/router/express/original_tests_part2.ipp:357: FAILED:
  REQUIRE( rm.match_route( target_path, params ) )
with expansion:
  false

-------------------------------------------------------------------------------
Original tests #27
-------------------------------------------------------------------------------
test/router/express/original_tests_part2.ipp:396
...............................................................................                                          

test/router/express/original_tests_part2.ipp:396: FAILED:
due to unexpected exception with message:
  unable to process route "/:test*-bar": regex_error

-------------------------------------------------------------------------------
Original tests #28
-------------------------------------------------------------------------------
test/router/express/original_tests_part2.ipp:472
...............................................................................                                          

test/router/express/original_tests_part2.ipp:472: FAILED:
due to unexpected exception with message:
  unable to process route "/:test+": regex_error

-------------------------------------------------------------------------------
Original tests #29
-------------------------------------------------------------------------------
test/router/express/original_tests_part2.ipp:543
...............................................................................                                          

test/router/express/original_tests_part2.ipp:543: FAILED:
due to unexpected exception with message:
  unable to process route "/:test(\d+)+": regex_error

-------------------------------------------------------------------------------
Original tests #30
-------------------------------------------------------------------------------
test/router/express/original_tests_part2.ipp:588
...............................................................................                                          

test/router/express/original_tests_part2.ipp:588: FAILED:
due to unexpected exception with message:
  unable to process route "/route.:ext(json|xml)+": regex_error

-------------------------------------------------------------------------------
Original tests #31
-------------------------------------------------------------------------------
test/router/express/original_tests_part2.ipp:659
...............................................................................                                          

test/router/express/original_tests_part2.ipp:659: FAILED:
due to unexpected exception with message:
  unable to process route "/:test*": regex_error

-------------------------------------------------------------------------------
Original tests #32
-------------------------------------------------------------------------------
test/router/express/original_tests_part2.ipp:742
...............................................................................                                          

test/router/express/original_tests_part2.ipp:742: FAILED:
due to unexpected exception with message:
  unable to process route "/route.:ext([a-z]+)*": regex_error

-------------------------------------------------------------------------------
Original tests #34
-------------------------------------------------------------------------------
test/router/express/original_tests_part2.ipp:877
...............................................................................                                          

test/router/express/original_tests_part2.ipp:895: FAILED:
  REQUIRE( rm.match_route( target_path, params ) )
with expansion:
  false

-------------------------------------------------------------------------------
Original tests #38
-------------------------------------------------------------------------------
test/router/express/original_tests_part2.ipp:1107
...............................................................................                                          

test/router/express/original_tests_part2.ipp:1107: FAILED:
due to unexpected exception with message:
  unable to process route "/:path(abc|xyz)*": regex_error

-------------------------------------------------------------------------------
Original tests #42
-------------------------------------------------------------------------------
test/router/express/original_tests_part3.ipp:121
...............................................................................                                          

test/router/express/original_tests_part3.ipp:139: FAILED:
  REQUIRE( rm.match_route( target_path, params ) )
with expansion:
  false

-------------------------------------------------------------------------------
Original tests #46
-------------------------------------------------------------------------------
test/router/express/original_tests_part3.ipp:400
...............................................................................                                          

test/router/express/original_tests_part3.ipp:437: FAILED:
  REQUIRE_FALSE( rm.match_route( target_path, params ) )
with expansion:
  !true

-------------------------------------------------------------------------------
Original tests #47
-------------------------------------------------------------------------------
test/router/express/original_tests_part3.ipp:445
...............................................................................                                          

test/router/express/original_tests_part3.ipp:445: FAILED:
due to unexpected exception with message:
  unable to process route "/test.:format+": regex_error

-------------------------------------------------------------------------------
Original tests #48
-------------------------------------------------------------------------------
test/router/express/original_tests_part3.ipp:502
...............................................................................                                          

test/router/express/original_tests_part3.ipp:520: FAILED:
  REQUIRE( rm.match_route( target_path, params ) )
with expansion:
  false

-------------------------------------------------------------------------------
Original tests #50
-------------------------------------------------------------------------------
test/router/express/original_tests_part3.ipp:592
...............................................................................                                          

test/router/express/original_tests_part3.ipp:647: FAILED:
  REQUIRE( nps[0].second == R"value(route.html)value" )
with expansion:
  route == "route.html"

-------------------------------------------------------------------------------
Original tests #51
-------------------------------------------------------------------------------
test/router/express/original_tests_part3.ipp:662
...............................................................................                                          

test/router/express/original_tests_part3.ipp:732: FAILED:
  REQUIRE( nps[0].second == R"value(route.json)value" )
with expansion:
  route == "route.json"

-------------------------------------------------------------------------------
Original tests #52
-------------------------------------------------------------------------------
test/router/express/original_tests_part3.ipp:747
...............................................................................                                          

test/router/express/original_tests_part3.ipp:765: FAILED:
  REQUIRE( rm.match_route( target_path, params ) )
with expansion:
  false

-------------------------------------------------------------------------------
Original tests #53
-------------------------------------------------------------------------------
test/router/express/original_tests_part3.ipp:832
...............................................................................                                          

test/router/express/original_tests_part3.ipp:857: FAILED:
  REQUIRE( rm.match_route( target_path, params ) )
with expansion:
  false

-------------------------------------------------------------------------------
Original tests #55
-------------------------------------------------------------------------------
test/router/express/original_tests_part3.ipp:945
...............................................................................                                          

test/router/express/original_tests_part3.ipp:963: FAILED:
  REQUIRE( rm.match_route( target_path, params ) )
with expansion:
  false

-------------------------------------------------------------------------------
Original tests #68
-------------------------------------------------------------------------------
test/router/express/original_tests_part4.ipp:82
...............................................................................                                          

test/router/express/original_tests_part4.ipp:82: FAILED:
due to unexpected exception with message:
  Missing parameter name at: 8

-------------------------------------------------------------------------------
Original tests #70
-------------------------------------------------------------------------------
test/router/express/original_tests_part4.ipp:209
...............................................................................                                          

test/router/express/original_tests_part4.ipp:227: FAILED:
  REQUIRE( rm.match_route( target_path, params ) )
with expansion:
  false

-------------------------------------------------------------------------------
Original tests #75
-------------------------------------------------------------------------------
test/router/express/original_tests_part4.ipp:451
...............................................................................                                          

test/router/express/original_tests_part4.ipp:451: FAILED:
due to unexpected exception with message:
  unable to process route "/:foo+baz": regex_error

-------------------------------------------------------------------------------
Original tests #76
-------------------------------------------------------------------------------
test/router/express/original_tests_part4.ipp:515
...............................................................................                                          

test/router/express/original_tests_part4.ipp:552: FAILED:
  REQUIRE( rm.match_route( target_path, params ) )
with expansion:
  false

-------------------------------------------------------------------------------
Original tests #86
-------------------------------------------------------------------------------
test/router/express/original_tests_part5.ipp:295
...............................................................................                                          

test/router/express/original_tests_part5.ipp:313: FAILED:
  REQUIRE( rm.match_route( target_path, params ) )
with expansion:
  false

-------------------------------------------------------------------------------
Invalid path
-------------------------------------------------------------------------------
test/router/express/additional_tests.ipp:69
...............................................................................

test/router/express/additional_tests.ipp:79: FAILED:
  REQUIRE_THROWS( try_to_create( R"(/:foo([123]+)))" ) )
because no exception was thrown where one was expected:

===============================================================================
test cases:  83 |  52 passed | 31 failed
assertions: 852 | 821 passed | 31 failed