Exception in `endpoint_http_regex` when invoked from sub-router

[Tectu]

Using a regex endpoint from a sub-router always throws an exception:

terminate called after throwing an instance of 'std::logic_error'
  what():  endpoint_http_regex passed request which does not match: /router1/regex?one=sffff&two=asfaf

Simple example snipped to reproduce:

        // Subrouter
        auto sub_router_1 = std::make_shared<malloy::server::router>();
        sub_router_1->add(method::get, R"(^/regex\?one=(\w+)&two=(\w+)$)", [](const auto& req, const std::vector<std::string>& captures){
            std::string body;
            body += "captures:\n";
            for (std::size_t i = 0; i < captures.size(); i++)
                body += "  " + std::to_string(i) + ": " + captures[i] + "\n";

            response resp{ status::ok };
            resp.body() = body;

            return resp;
        });
        router->add_subrouter("/router1", sub_router_1);

Test target: http://127.0.0.1:8080/router1/regex?one=sffff&two=asfaf

[Tectu]

Seems to be related to #57

Printing the request in endpoint_http_regex::handle_req() reviels that the target was not chopped:

GET /router1/regex?one=sffff&two=asfaf HTTP/1.1

[0x00002a]

I'll add this will only happen with capture groups (as I added the check when testing originally) . I'm guessing the header uri is not modified as well as the target and this checks the uri.

[Tectu]

1 second in posting difference - nice :p are you on this at the moment? PR? Or should I dive into it?

[0x00002a]

I'm a bit busy rn, but I should be able to fix this in a bit, should just be a one or two liner I think. Maybe just change that check and the acompanying regex match to use the target(). Speaking of, this whole mess is entirely my fault but I do think there is an issue with the discrepency between uri() and target(). imo uri() should be calculated on the fly and/or target() should be overriden and synced to it.

[Tectu]

If you think that you can get to this in the next (few) hour(s) I'll leave it gladly to you :p

I also think that the test suite should be extended to catch these types of problems. I'm gonna setup some roundtrip tests similar to what you already initially added for WebSockets.

imo uri() should be calculated on the fly and/or target() should be overriden and synced to it.

I agree that this is adding a lot of discrepancy and makes the code harder to maintain. Initially I've done it this way to prevent having to parse/calculate the URI object every time this is needed. However, that could be at least mitigated by employing some caching technique. My main use case for the uri class was for easier parsing purposes. Given that we have regex with capturing support (thanks to you!) I don't think that I personally have any use for it in my current applications anymore. After all, I ran into this bug when updating an application to use capturing groups.

This needs some consideration but my gut feeling tells me to remove the uri class for now. What do you think?

[0x00002a]

I like the uri class but in all my applications using malloy I've already implemented something like it (since I started off doing what malloy already does in most cases). I also think there are speclisalist libraries for it. So yes I agree that it would be better to remove uri for now.

Shall I remove it as part of fixing this? Or leave it for later?

[Tectu]

Shall I remove it as part of fixing this? Or leave it for later?

Drop it! Drop it now! :p

[Tectu]

Well, this bug is kinda ruining my Saturday afternoon plans :p darnit. Looking very much forward to the first official release. Seems like we're getting close to that.

[0x00002a]

Sorry :p, I think I've fixed it now but just doing some testing

[Tectu]

I don't see this as your fault. This is a very much work-in-progress library. I failed as a reviewer & there aren't any proper test cases to catch these types of issues yet.