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

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

Closed
wseibel opened this issue Apr 30, 2024 · 6 comments · Fixed by #240
Closed
Assignees
Labels

Comments

@wseibel
Copy link

wseibel commented Apr 30, 2024

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
Copy link

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
Copy link

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

@wseibel
Copy link
Author

wseibel commented Apr 30, 2024

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

@chriswk chriswk self-assigned this May 6, 2024
@chriswk
Copy link
Member

chriswk commented May 6, 2024

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 added a commit that referenced this issue May 6, 2024
There had been an inversion of variable usage for one of our cases in
the matcher. This PR makes sure to compare contextValue to see if it
starts with the requested value in the constraint, instead of the other
way around.

fixes #238
@chriswk chriswk linked a pull request May 6, 2024 that will close this issue
@chriswk
Copy link
Member

chriswk commented May 6, 2024

Handled in #240

chriswk added a commit that referenced this issue May 6, 2024
* fix: check that contextValue starts with

There had been an inversion of variable usage for one of our cases in
the matcher. This PR makes sure to compare contextValue to see if it
starts with the requested value in the constraint, instead of the other
way around.

fixes #238
@github-project-automation github-project-automation bot moved this from New to Done in Issues and PRs May 6, 2024
@chriswk
Copy link
Member

chriswk commented May 6, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants