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 named path parameters parsing (Implements #1587) #1608

Merged
merged 4 commits into from
Jul 5, 2023

Conversation

bgs99
Copy link
Contributor

@bgs99 bgs99 commented Jul 4, 2023

As discussed with @bugdea1er, I have implemented named path parameter parsing.
New parser is accessible by a different set of Get/Post/Put/Patch/Delete/Options methods with a suffix "Simple" (open for suggestions).

Example of usage from tests:

  svr.GetSimple("/users/:id", [&](const Request &req, Response &) {
    EXPECT_EQ("user-id", req.path_params.at("id"));
    EXPECT_EQ("foo", req.get_param_value("param1"));
    EXPECT_EQ("bar", req.get_param_value("param2"));
  });

@bugdea1er
Copy link
Contributor

bugdea1er commented Jul 4, 2023

@yhirose one of the possible solutions for this naming is to hide the new functions behind an optional compilation flag like you did with ssl support. I doubt that anyone would use both options for paths in the same project

We want your opinion

@yhirose
Copy link
Owner

yhirose commented Jul 4, 2023

Great pull request! Indeed, the 'Simple' suffix might cause confusion among users. Personally, I would prefer to retain both the regex matcher and the path params matcher. How about checking at runtime whether the path includes /: at least once, and based on that, determining if it's in 'path params' mode? The code could be modified as follows:

inline Server &Server::Get(const std::string &pattern, Handler handler) {
  auto matcher = pattern.find("/:") != std::string::npos
    ? detail::make_unique<detail::PathParamsMatcher>(pattern),
    : detail::make_unique<detail::RegexMatcher>(pattern);

  get_handlers_.push_back(std::make_pair(matcher, std::move(handler)));
  return *this;
}

By following the above approach, we can maintain just one Get method with a minimal impact on performance. The only limitation of this approach is that we can no longer utilize /: in the regular expression pattern. However, I believe this constraint is reasonable and won't cause any inconvenience for existing users. What do you think?

@bgs99
Copy link
Contributor Author

bgs99 commented Jul 4, 2023

Great idea!
I was thinking about something like that but was not sure how you would judge the risk of breaking some convoluted setups.
I will update the PR tomorrow, if nothing goes wrong.

@yhirose
Copy link
Owner

yhirose commented Jul 4, 2023

Or something like this?

inline std::unique_ptr<detail::MatcherBase> Server::make_matcher(
    const std::string &pattern) const {
  return pattern.find("/:") != std::string::npos
         ? detail::make_unique<detail::PathParamsMatcher>(pattern),
         : detail::make_unique<detail::RegexMatcher>(pattern);
}

inline Server &Server::Get(const std::string &pattern, Handler handler) {
  get_handlers_.push_back(
      std::make_pair(make_matcher(pattern), std::move(handler)));
  return *this;
}

Could you please also update README to show calling examples? Thanks a lot!

@bgs99
Copy link
Contributor Author

bgs99 commented Jul 5, 2023

Done.
Used an if statement instead of the ternary operator though, as the latter won't compile with different argument types.

@yhirose yhirose merged commit 17fc522 into yhirose:master Jul 5, 2023
@yhirose
Copy link
Owner

yhirose commented Jul 5, 2023

Thanks for the fine contribution! I believe many users will like the new feature. 👍

@yhirose
Copy link
Owner

yhirose commented Sep 4, 2024

@bgs99 could you take a look at #1805? It seems like this PR creates the issue. Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants