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: Custom parameter/segment regex #141

Closed
wants to merge 1 commit into from

Conversation

boekkooi-lengoo
Copy link
Contributor

Good day,

First of thanks for reviewing.

This PR tries to solve one of the issues we see often see which is that we would like to do some basic validation on a parameter.
A case we had was where we would want to route differently depending on if the parameter was a uuid or a number. Another case we see it so avoid passing bad parameters to upstream services.

The solution is this case is to add the regex to the parameter. This is done by adding a colon after the name of the parameter.

In this PR we also move to the usage of regex capture groups for two reasons. The first one being that it allows us to get rid of the names array/table and the second one is because it ensures that capture groups from custom regular expressions should not interfere with the route parameters. However, this change comes with a backwards compatibility break as

Please let me know what you think of the idea

Allow users to use a custom regex to match a parameter.
@@ -263,6 +263,21 @@ local rx = radix.new({
})
```

### Parameters in Path with a custom regex

You can specify parameters on a path and use a regex to match the parameter segment of the path (see `hsegment` of [RFC1738](https://www.rfc-editor.org/rfc/rfc1738.txt).
Copy link
Contributor

Choose a reason for hiding this comment

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

why we need this feature? Are there any other product that supports similar feature?

I took a look at https://www.rfc-editor.org/rfc/rfc1738.txt, I do not sure if we need to support Parameters in Path with a custom regex.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reference is referring to radixtree.lua Line 655 and that the custom regex is for the segment of the uri (aka everything between a /).

In regards to having this type of validation/feature I know of prefix_regex in ambassador, there is also envoy route regex.

However, I think we could also use the var.request_uri in vars to get the feature. This in turn means having a regex for 1) matching the route 2) matching the route with the argument, from my perspective this go's against the performance that APISIX seems to talk about then again I maybe wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a new feature for lua-resty-radixtree, I need more time to research and think. I'll reply you before Friday in this week

If you were hurry about this new feature, feel free to ping me ^_^

@shreemaan-abhishek
Copy link
Member

@boekkooi-lengoo like you said, this can be done with vars check. Although this might not be very high performant but adding regex support seems like a maintenance burden.

@membphis
Copy link
Contributor

@boekkooi-lengoo like you said, this can be done with vars check. Although this might not be very high performant but adding regex support seems like a maintenance burden.

The performance is not the mainly I worry about currently. Only few developer will use this feature, so I think they are all fine.

parameter/segment regex way is easier to understand and maintain for developer.

I've been thinking, is there any other simpler and easier-to-understand configuration way?

@boekkooi-lengoo
Copy link
Contributor Author

@membphis I'm personally not very happy with this implementation as it's horrible for metrics and logs.

I'm personally wondering if it would make sense to allow specifying the regex per param something like the following:

        {
            paths = { "/user/:name" },
            metadata = { "metadata /user/name" },
            methods = { "GET" },
            params = { name = "[a-z]+" }
        },

Would this be something that would work for you? Also I'm not sure if people would not use this as I feel that failing early and at the edge is better then at the service.

@monkeyDluffy6017
Copy link

I will check this later, thanks for your contribution!

@membphis
Copy link
Contributor

@boekkooi-lengoo We can add new variable uri_segment_*** in vars, fetch the value of name field.

I think this style is easy to understand and maintain.

{
    paths = { "/user/:name" },
    metadata = { "metadata /user/name" },
    vars = {
        {"uri_segment_name", "~~", "[a-z]+"}
    }
}

@boekkooi-lengoo
Copy link
Contributor Author

@membphis I like that. Maybe we should name it uri_param_<name> instead of uri_segment_<name>?

@membphis
Copy link
Contributor

@membphis I like that. Maybe we should name it uri_param_<name> instead of uri_segment_<name>?

LGTM ^_^

@membphis
Copy link
Contributor

membphis commented Dec 29, 2023

I checked your another PR: apache/apisix#10645

it seems that you had finished this job in APISIX ^_^ . so we do not need this PR now, all right?

@boekkooi-lengoo
Copy link
Contributor Author

@membphis incorrect as this pr is for more refined routing and better performance. Where as apache/apisix#10645 is for using the Uri parameter value in plugins like proxy rewrite

@membphis
Copy link
Contributor

membphis commented Jan 2, 2024

@boekkooi-lengoo got it. We can follow this style. Waiting for your update

{
    paths = { "/user/:name" },
    metadata = { "metadata /user/name" },
    vars = {
        {"uri_param_name", "~~", "[a-z]+"}
    }
}

@boekkooi-lengoo
Copy link
Contributor Author

Hey @membphis

Please checkout #142 which is an implementation of this using vars.
I didn't want to override this PR as the implementation is very different

Cheers,
Warnar

@boekkooi-lengoo
Copy link
Contributor Author

I'm closing this as #142 is superseeding it.

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.

4 participants