api7 / lua-resty-radixtree

Adaptive Radix Trees implemented in Lua / LuaJIT
https://api7.ai/
Apache License 2.0
256 stars 61 forks source link

feat: Custom parameter/segment regex #141

Closed boekkooi-lengoo closed 10 months ago

boekkooi-lengoo commented 11 months ago

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

shreemaan-abhishek commented 10 months ago

@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 commented 10 months ago

@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 commented 10 months ago

@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 commented 10 months ago

I will check this later, thanks for your contribution!

membphis commented 10 months ago

@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 commented 10 months ago

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

membphis commented 10 months ago

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

LGTM ^_^

membphis commented 10 months ago

I checked your another PR: https://github.com/apache/apisix/pull/10645

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

boekkooi-lengoo commented 10 months ago

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

membphis commented 10 months ago

@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 commented 10 months ago

Hey @membphis

Please checkout https://github.com/api7/lua-resty-radixtree/pull/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 commented 10 months ago

I'm closing this as https://github.com/api7/lua-resty-radixtree/pull/142 is superseeding it.