flaviostutz / ruller-dsl-feature-flag

A feature flag engine that can be used to enable, change or rollout features of a system dynamically based on system or user attributes
MIT License
2 stars 6 forks source link

Adding multiple field match to contains function #15

Closed milhomens closed 4 years ago

milhomens commented 4 years ago

I need the ability to match two or more input fields in the contains function.

For this, I propose the following solution: Adding the "," separator to the input regex at the contains and changing the groupContains function to have an array as parameter and iterate over this array adding some character such as ":" and in the group file have the fields concatenated using the same character.

This way, the condition would be something like: "contains(group:examplegroup,input:first_input,input:second_input,input:third_input)" and for the inputs: first_input: 1 secont_input: 2 third_input: 3 the match would be "1:2:3"

flaviostutz commented 4 years ago

Let me validate my understanding. So when someone calls ruller with input variables 'first_input=1, second_input=2, third_input=3', we will verify if the group contains element "1:2:3", right? We will concatenate the input fields and verify if the contatenated contents exists on the group, right?

milhomens commented 4 years ago

Correct!

flaviostutz commented 4 years ago

Maybe we could create a "virtual input" so that this kind of manipulation could be used in any other contexts.

Something like:

{
    "_config": {
        "virtual_inputs": {
            "customer_id_name": {
                 "op": "concat",
                 "separator": ":",
                 "inputs": [ "customer_id", "customer_name" ]
            }
        }
    },
    "_items": [{
            "provider": "aws",
            "_condition": "randomPerc(10, input:customer_id_name) and contains(group:engineers_id_name,input:customer_id_name)"
     }]
}

What do you think?

@abilioesteves @tiagostutz @jairsjunior @milhomens

milhomens commented 4 years ago

That's a good solution. Thinking of issue #12, this way we could test for multiple inputs for null without having to add multiple conditions. Just concatenating them and testing for something like null:null:null:null (or :::) depending on how it would treat a null input.

eabili0 commented 4 years ago

@flaviostutz and @milhomens, shouldn't we simply create a concat function, such that we could do the following instead?

{
    "_items": [{
        "_condition": "contains(group:engineers_id_name, concat(input:customer_id, ':', input:customer_name)"
    }]
}
flaviostutz commented 4 years ago

I like it. Much simpler and easier to implement!

"concat(...)" should support a variable number of elements for concatenation. This is simple to implement with https://gobyexample.com/variadic-functions

Great! Something else?

milhomens commented 4 years ago

That's even better @abilioesteves I made a small test locally, using your idea, but in order to make it work I had to change the regex on the outer function that has the concat inside. Example: (rules.json) "_condition": "contains(group:anygroup,concat(input:anyinput,':',input:otherinput))"

(main.go - had to change the regex from contains - see cause below) groupRegex := regexp.MustCompile("contains\(\sgroup:([a-z0-9_-]+)\s,\s([0-9a-zA-Z:_,()'.\\"\[\]]+)\s\)")

Since the concat resolves to something like: concatString(ctx.Input["anyinput"].(string),":",ctx.Input["otherinput"].(string)), the outer function regex need to accept all the characters in resolved string.

Maybe I'm doing something wrong, but it worked. Any comments on that? @flaviostutz @abilioesteves

flaviostutz commented 4 years ago

Hm... this is very prone to bugs. Have you reached a good regex for this?

Sent from my iPhone

On 1 Jul 2020, at 13:29, Hugo Milhomens notifications@github.com wrote:

 That's even better @abilioesteves I made a small test locally, using your idea, but in order to make it work I had to change the regex on the outer function that has the concat inside. Example: (rules.json) "_condition": "contains(group:anygroup,concat(input:anyinput,':',input:otherinput))"

(main.go - had to change the regex from contains - see cause below) groupRegex := regexp.MustCompile("contains(\sgroup:([a-z0-9-]+)\s,\s*([0-9a-zA-Z:,()'.\"[]]+)\s*)")

Since the concat resolves to something like: concatString(ctx.Input["anyinput"].(string),":",ctx.Input["otherinput"].(string)), the outer function regex need to accept all the characters in resolved string.

Maybe I'm doing something wrong, but it worked. Any comments on that? @flaviostutz @abilioesteves

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

milhomens commented 4 years ago

Actually i used the one from my previous comment, but maybe I'm implementing it the wrong way. If used as a inner function, I have to change the regex of the outer function. I just did it for the contains, but will need to do in randomPerc/randomPercRage for example (Can't think of a use for this, but the idea is that any function that is possible to have concat inside should change it's regex)

flaviostutz commented 4 years ago

So, do you have any working code already? Maybe it could be useful to create a unit test for this.

On Thu, Jul 2, 2020 at 11:59 AM Hugo Milhomens notifications@github.com wrote:

Actually i used the one from my previous comment, but maybe I'm implementing it the wrong way. If used as a inner function, I have to change the regex of the outer function. I just did it for the contains, but will need to do in randomPerc/randomPercRage for example (Can't think of a use for this, but the idea is that any function that is possible to have concat inside should change it's regex)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/flaviostutz/ruller-dsl-feature-flag/issues/15#issuecomment-653057424, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB3N4XBZRVV6GERT6NHB3XTRZSOFFANCNFSM4OMPEQQQ .

milhomens commented 4 years ago

Maybe instead of making the regex too "open" and prone to errors given that we should use it in all functions, what if we used both @flaviostutz and @abilioesteves solutions together, as in:

 "_config": {
        "virtual_inputs": {
            "customer_id_name": concat(input:customer_id, ":" , input:customer_name)
        }
    },
    "_items": [{
        "_condition": "contains(group:engineers_id_name, input:customer_id_name)"
    }]
flaviostutz commented 4 years ago

I like Abilio’s design. Let’s try to implement that way. Did you find any problem already?

Sent from my iPhone

On 6 Jul 2020, at 13:22, Hugo Milhomens notifications@github.com wrote:

 Maybe instead of making the regex too "open" and prone to errors given that we should use in all functions, what if we used both @flaviostutz and @abilioesteves solutions together, as in: "_config": { "virtual_inputs": { "customer_id_name": concat(input:customer_id, ":" , input:customer_name) } "_items": [{ "_condition": "contains(group:engineers_id_name, input:customer_id_name)" }]

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

milhomens commented 4 years ago

You mean using Abilio's design alone? The only "problem" was having to change the regex. But it worked fine (only changed for contains function). If you want I can send a PR

flaviostutz commented 4 years ago

Yep... Great! Please send the PR!

Sent from my iPhone

On 7 Jul 2020, at 15:50, Hugo Milhomens notifications@github.com wrote:

 You mean using Abilio's design alone? The only "problem" was having to change the regex. But it worked fine (only changed for contains function). If you want I can send a PR

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.