Spike for constraints #27

sighphyre · 2022-02-28T12:34:59Z

This implements extended support for constraint operators, reference implementation on the node SDK is here: https://github.com/Unleash/unleash-client-node/pull/289/files

Note that the client specs have not been rolled forward to cover these cases, since the specs also include cases for custom stickiness, which isn't as of yet implemented in this SDK, this was covered by local testing against the spec

…t first

sighphyre · 2022-02-28T12:40:49Z

src/strategy.rs

+    expression: ConstraintExpression,
+    inverted: Option<bool>,
+    getter: F,
+) -> Evaluate
 where
    F: Fn(&Context) -> Option<&String> + Clone + Sync + Send + 'static,
 {
    match &expression {


This match is getting a little bit long. I'm struggling to break it out into more granular functions due to rust marking closures as unique types. I'm very open to suggestions if someone feels the same as me and has clever ideas here

https://docs.rs/enum_dispatch/latest/enum_dispatch/ is probably the thing to investigate.

We'd want to refactor the api type as well to have layers:

#[serde(flatten)] enum ConstraintExpression{ NotIn(NotInExpression), ...

Or something like that.

I suggest not trying that though, until we've removed the code duplication below.

rbtcollins

the tl;dr is:

the test suite is too long and not interestingly-different: lets bring in proptest
there is too much code duplication in the new code paths

I've made some concrete suggestions on how to address these issues inline in the review.

It will be very nice to have this feature!

rbtcollins · 2022-03-01T08:36:37Z

src/api.rs

    #[serde(flatten)]
    pub expression: ConstraintExpression,
 }

 #[derive(Clone, Serialize, Deserialize, Debug)]
-#[serde(tag = "operator", content = "values")]


It is a shame this needs to change: the enums are much less ergonomic now :( - single attribute structs :(.

I wonder if struct flattening + an internally tagged representation could work: a family of related enums:

one with values

one with value

rbtcollins · 2022-03-01T08:45:06Z

src/context.rs

@@ -40,4 +41,10 @@ pub struct Context {
    pub app_name: String,
    #[serde(default)]
    pub environment: String,
+    #[serde(default = "current_time", rename = "currentTime")]


We should test #[serde(rename_all = "camelCase")] at some point, might be sufficient.

rbtcollins · 2022-03-01T08:47:54Z

src/context.rs

@@ -40,4 +41,10 @@ pub struct Context {
    pub app_name: String,
    #[serde(default)]
    pub environment: String,
+    #[serde(default = "current_time", rename = "currentTime")]
+    pub current_time: String,


This shouldn't be a string.

IIRC correctly chrono doesn't have a sensible default, so I suggest:
a Chrono tuple newtype with serde derived impls, a std::default::Default implementation for that, then inclusion here, which as the chrono::serde default is rfc3339 should just work.

In fact, I'm not sure whether we should be capturing the current time at all. For the proxy use case, clients should not be permitted to time-travel; for the non-proxy use case access to the current time is probably cheap enough, since time access is such a well known thing.

We could compile rules that relate to get compiled into a unix time expression for even faster evaluation, if scaling the proxy is required - though I suspect only unleash themselves would hit that scale.

rbtcollins · 2022-03-01T08:50:22Z

src/strategy.rs

-fn _compile_constraint_string<F>(expression: ConstraintExpression, getter: F) -> Evaluate
+fn _compile_constraint_string<F>(
+    expression: ConstraintExpression,
+    inverted: Option<bool>,


Rather than inverted being Option, I think it wants to be a type parameter: we want to have unchanging code in this function whose types change depending on the parameter.

fn _compile_constraint_string<F, B>(expression: ConstraintExpression, getter: F, boxer:B) -> Evaluate where F: ..., B: Fn(Fn(&Context) -> bool) -> Evaluate { ... ConstraintExpression::In { values } => { let as_set: HashSet<String> = values.iter().cloned().collect(); boxer(move |context: &Context| { getter(context).map(|v| as_set.contains(v)).unwrap_or(false) }) } ....

May need a little code golf to make it work, but I think this would be a nice place to get to.

rbtcollins · 2022-03-01T08:51:47Z

src/strategy.rs

+    expression: ConstraintExpression,
+    inverted: Option<bool>,
+    getter: F,
+) -> Evaluate
 where
    F: Fn(&Context) -> Option<&String> + Clone + Sync + Send + 'static,
 {
    match &expression {


https://docs.rs/enum_dispatch/latest/enum_dispatch/ is probably the thing to investigate.

We'd want to refactor the api type as well to have layers:

#[serde(flatten)] enum ConstraintExpression{ NotIn(NotInExpression), ...

Or something like that.

I suggest not trying that though, until we've removed the code duplication below.

rbtcollins · 2022-03-01T09:26:47Z

src/strategy.rs

+                })
+            }
+        }
+        ConstraintExpression::StrEndsWith {


theres lots of duplication with StrStartsWith: I think a helper function generic over the predicate would be super useful here.

rbtcollins · 2022-03-01T09:26:56Z

src/strategy.rs

+                })
+            }
+        }
+        ConstraintExpression::StrContains {


rbtcollins · 2022-03-01T09:27:21Z

src/strategy.rs

+                })
+            }
+        }
+        ConstraintExpression::NumEq { value } => match value.parse::<f64>() {


and this family of contraints should also be extractable into composable helpers

rbtcollins · 2022-03-01T09:31:22Z

src/strategy.rs

@@ -332,16 +561,17 @@ where
                        }
                        false
                    })
-                    .unwrap_or(false)
+                    .unwrap_or(false);


An alternative idea is a helper trait on Bool adding .inverted(inverted) - that will allow

.unwrap_or(false).inverted(inverted)

rbtcollins · 2022-03-01T09:31:44Z

src/strategy.rs

+            ..Default::default()
+        };
+        // starts with matches string start
+        assert!(super::constrain(


I think we probably want to bring proptest in at this point

chriswk · 2022-03-14T07:24:18Z

Rust SDK: add support for new strategy constraint operators

sighphyre added 10 commits February 28, 2022 11:28

Basic scaffolding for constraints

441e588

feat: implement new operators, need to make the integration tests pas…

e9cb99a

…t first

chore: fix issue with broken default on context object

696ae5a

feat: implement inversion on contraint expressions

b30bc34

feat: tests for inversion

344d8fe

feat: implement case insensitivity for string operators

0c2f502

fix: patch not in constraint to always default to true

787da2f

fix: roll back client spec until custom stickiness is implemented

5454a8e

chore: formatting

83d7913

chore: trim out str-utils dep since it wasn't needed

53c1d79

sighphyre commented Feb 28, 2022

View reviewed changes

sighphyre requested review from rbtcollins and chriswk February 28, 2022 12:42

rbtcollins requested changes Mar 1, 2022

View reviewed changes

sighphyre added 2 commits March 2, 2022 20:04

chore: messy spike for enum dispatch

0209713

refactor constraint enums to use enum_dispatch

55d0a6a

Meemaw mentioned this pull request Jul 16, 2023

feat: implement semver strategy constraints #65

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spike for constraints #27

Spike for constraints #27

sighphyre commented Feb 28, 2022

sighphyre Feb 28, 2022

rbtcollins Mar 1, 2022

rbtcollins left a comment

rbtcollins Mar 1, 2022

rbtcollins Mar 1, 2022

rbtcollins Mar 1, 2022

rbtcollins Jun 23, 2022

rbtcollins Mar 1, 2022

rbtcollins Mar 1, 2022

rbtcollins Mar 1, 2022

rbtcollins Mar 1, 2022

rbtcollins Mar 1, 2022

rbtcollins Mar 1, 2022

rbtcollins Mar 1, 2022

chriswk commented Mar 14, 2022

Spike for constraints #27

Are you sure you want to change the base?

Spike for constraints #27

Conversation

sighphyre commented Feb 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbtcollins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriswk commented Mar 14, 2022