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

feat(router): add a router plugin #131

Merged
merged 1 commit into from
Apr 21, 2024
Merged

Conversation

Tieske
Copy link
Contributor

@Tieske Tieske commented Mar 12, 2023

This builds on top of #130

Adds a router plugin.

@wmealing
Copy link
Contributor

wmealing commented Jan 15, 2024

I got tricked for a while, as I had forgotten that the string paths were regular expressions. This post is left for other users who may wonder why pegas is not serving matching the expected path.

Any of the "magic characters"

( ) . % + - * ? [ ^ $

Need to be escaped with %, for example the route "/catch-fish"

local routes = {

   ["/catch%-fish"] = {
      GET = function(req, resp)
         resp:statusCode(200)
         resp:addHeader("Content-Type","text/html")
         resp:write("catch")
      end,
   },

I do not consider this a bug, its likely a bug in my comprehension.

Copy link
Contributor

@wmealing wmealing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm relatively new with lua, so I can't speak for how it should look, however it does seem to work well.

I tested the 'routing' part of this patchset, it works well.

I got tripped up because I thought the path string was.. a string match originally,but after realising it was a regular expression, it was clear. Its possible that maybe this should be in the comments/documentation but its not a dealbreaker.

@Tieske
Copy link
Contributor Author

Tieske commented Apr 3, 2024

@wmealing I think that's actually a bug, we should (for simplicity sake) escape the path before matching. Such that a path "/my-cool-path/{parameter}" can be used without manually escaping the "-" characters.

Other things:

  • I think the shortcut on the path level (passing a function instead of a table) should go. Require users to be explicit and add a wildcard method handler "*" instead.
  • The examples currently refer to a JSON lib, which was removed, so they must be updated.
  • The last segment should be configurable as "{parameter+}" where the "+" means matching more than 1 segment (normally parameters are limited to 1 segment)
  • Test that multiple router plugins can be added, such that multiple independent API components can be served from their own plugin. For example:
    • first plugin on: GET "/my/app"
    • second plugin on: POST "/my/app"
  • Test that we unescape the path before matching, eg incoming "/my%20path" should match a configured router path "/my path" (also applies to the path-prefix on router level)
  • Per router a list of hosts to match (currently only matches on a path prefix)

Any input appreciated.

Copy link
Owner

@EvandroLG EvandroLG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @Tieske

@EvandroLG EvandroLG merged commit 66659a8 into EvandroLG:master Apr 21, 2024
7 checks passed
@Tieske Tieske deleted the router branch April 25, 2024 14:18
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