Unleash / unleash-client-java

Unleash client SDK for Java
https://docs.getunleash.io
Apache License 2.0
121 stars 70 forks source link

String operator STR_STARTS_WITH is not evaluated correctly when case-sensative is disabled #238

Closed wseibel closed 7 months ago

wseibel commented 7 months ago

Describe the bug

When using the STR_STARTS_WITH operator as a constraint on any context field with case-sensitive de-activated, the constraint is always evaluates to false.

Steps to reproduce the bug

Create an activation strategy for an enabled feature-toggle and update it with the following definition

curl --location --request PUT '<your-host>/api/admin/projects/<your-project>/features/test/environments/<your-environment>/strategies/<your-strategy-id>' \
    --header 'Authorization: INSERT_API_KEY' \
    --header 'Content-Type: application/json' \
    --data-raw '{
  "name": "flexibleRollout",
  "title": "",
  "constraints": [
    {
      "values": [
        "0"
      ],
      "inverted": false,
      "operator": "STR_STARTS_WITH",
      "contextName": "userId",
      "caseInsensitive": true
    }
  ],
  "parameters": {
    "rollout": "100",
    "stickiness": "default",
    "groupId": "test"
  },
  "variants": [
    {
      "stickiness": "default",
      "name": "a",
      "weight": 1000,
      "payload": {
        "type": "string",
        "value": "a"
      },
      "weightType": "variable"
    }
  ],
  "segments": [],
  "disabled": false
}'

Now evaluate the feature toggle you updated the strategy for with any user-id that starts with "0". The result is "disabled".

Expected behavior

The evaluation result should return the variation with the name "a".

Logs, error output, etc.

No response

Screenshots

No response

Additional context

When looking into the code the error is easy to spot

return contextValue
                .map(
                        c ->
                                values.stream()
                                        .anyMatch(
                                                v -> {
                                                    if (caseInsensitive) {
                                                        return v.toLowerCase(comparisonLocale) // <-- v.startsWith should be c.startsWith
                                                                .startsWith(
                                                                        c.toLowerCase(
                                                                                comparisonLocale));
                                                    } else {
                                                        return c.startsWith(v); // <-- this is correct
                                                    }
                                                }))
                .orElse(false);

The comparison is the wrong way around as can be seen in the else-branch or any other method in this class, where it's done correctly.

Unleash version

No response

Subscription type

None

Hosting type

None

SDK information (language and version)

unleash-client-java:9.2.0

rishiraj88 commented 7 months ago

Very apt catch, @wseibel .

// if-clause checks v.startsWith(c) if (caseInsensitive) {return v.toLowerCase(..).startsWith(c.toLowerCase(..)); }else {// else-clause checks c.startsWith(v) (as opposite of the if-clause check!!) return c.startsWith(v); // <-- this is correct }

It is obvious to fix.

rishiraj88 commented 7 months ago

I think v and c both need to be interchanged in if clause. What do you think, @wseibel ?

wseibel commented 7 months ago

@rishiraj88 thanks for the quick reply. You're absolutely right.

chriswk commented 7 months ago

A big thanks to both of you, great report, I'll get a fix out, and add a test so we catch it if it happens again.

chriswk commented 7 months ago

Handled in #240

chriswk commented 7 months ago

Added in 9.2.1, should be synced to most mirrors by now. This https://repo1.maven.org/maven2/io/getunleash/unleash-client-java/9.2.1/ is at least present on central