Skip to content

fix: not_regex should handle all value types like regex - #305

Open
eeshsaxena wants to merge 1 commit into
PostHog:mainfrom
eeshsaxena:fix/not-regex-value-types
Open

fix: not_regex should handle all value types like regex#305
eeshsaxena wants to merge 1 commit into
PostHog:mainfrom
eeshsaxena:fix/not-regex-value-types

Conversation

@eeshsaxena

Copy link
Copy Markdown
Contributor

Problem

The regex and not_regex operators handle value types differently. regex coerces both sides with valueToString, so it accepts strings, ints, floats, and bools:

if operator == "regex" {
    r, err := getOrCompileRegex(valueToString(value))
    if err != nil { return false, nil }
    return r.MatchString(valueToString(override_value)), nil
}

not_regex instead uses a manual string/int type switch and returns an error for anything else:

if valueString, ok := override_value.(string); ok {
    match = r.MatchString(valueString)
} else if valueInt, ok := override_value.(int); ok {
    match = r.MatchString(strconv.Itoa(valueInt))
} else {
    return false, errors.New("value type not supported")
}

The important gap is float64: JSON numbers deserialize to float64, so a not_regex condition on a numeric property value errors out even though the same value works with regex:

regex     ^1  vs float64(123)  ->  (true,  <nil>)
not_regex ^1  vs float64(123)  ->  (false, "value type not supported")   ❌

Fix

Make not_regex mirror regex, coercing both the pattern and the property value with valueToString. This handles all value types and keeps the two operators consistent. Added TestMatchPropertyNotRegexHandlesAllValueTypes (string, int, float64).

Testing

go test . passes (full package suite, including the new test).

The not_regex operator used a manual string/int type switch and returned an
error for any other type, most notably float64 (what JSON numbers deserialize
to), so it failed on a numeric property value even though regex handled the
same value. Coerce both the pattern and the property value with valueToString,
mirroring the regex operator, and add a regression test.
@eeshsaxena
eeshsaxena requested a review from a team as a code owner August 28, 2026 04:49
@eeshsaxena

Copy link
Copy Markdown
Contributor Author

This pairs with the exact/is_not consistency fix in #299 and is independent of it. The practical trigger is common: a not_regex flag condition on a numeric property (an ID, a version code, an age) that arrives as a JSON number, which becomes float64 in Go, currently errors instead of evaluating. Routing through valueToString keeps not_regex in step with regex for every value type.

@dustinbyrne dustinbyrne left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @eeshsaxena, thanks for another pull request here! i ran an agentic review for compatibility, and it had one minor finding i've described below. it's small enough in scope that it seems worth fixing.

Comment thread featureflags.go
// string/int type switch errored on other types, most notably float64,
// which is what JSON numbers deserialize to, so not_regex failed on a
// numeric property value even though regex handled it.
r, err := getOrCompileRegex(valueToString(value))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a bit of an edge case, but since we're in here, one minor deviation from the feature flags evaluation service is if value is nil, the string representation returned is <nil>

e.g. with the following properties:

  { "k": null }

and condition:

  {
    "key": "k",
    "operator": "not_regex",
    "value": "^null$"
  }

this would match remotely, but not match here.

@dustinbyrne
dustinbyrne requested a review from a team August 28, 2026 19:25
@marandaneto
marandaneto requested a review from a team August 31, 2026 07:05
@dustinbyrne

Copy link
Copy Markdown
Contributor

also, your commits will need to be signed in order to be eligible for merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants