Repository navigation
Conversation
|
@yoarajota is attempting to deploy a commit to the Flagsmith Team on Vercel. A member of the Team first needs to authorize it. |
Walkthrough
Merge Risk:
|
There was a problem hiding this comment.
Actionable comments posted: 1

Review info
-
Run configuration-
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Organization UI
- Review profile: ASSERTIVE
- Plan: Advanced
- Run ID:
a8b13a4f-c65d-4f8a-9cc0-d2106c5976cb
Files selected for processing: 2-
api/integrations/launch_darkly/services.pyapi/tests/unit/integrations/launch_darkly/test_services.py
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.
-
| # start only. Leading inline flags such as `(?i)` must stay at the start of the expression. | ||
| flags = re.match(r"(?:\(\?[a-zA-Z]+\))+", pattern) | ||
| prefix = flags.group() if flags else "" | ||
| return f"{prefix}.*(?:{pattern[len(prefix) :]})" |
There was a problem hiding this comment.
Match across newlines in the prefix.
If a trait value is "first\nbar" and the LaunchDarkly pattern is bar, LaunchDarkly can find bar, but .*(?:bar) cannot reach it. . does not match a newline by default. Use a newline-matching prefix, such as (?s:.*), without changing the flags of the imported pattern. Add a multiline case to the segment-membership test. (launchdarkly.com)
Proposed change
-
--- "a/api/integrations/launch_darkly/services.py" +++ "b/api/integrations/launch_darkly/services.py" @@ -179,7 +179,7 @@ # start only. Leading inline flags such as `(?i)` must stay at the start of the expression. flags = re.match(r"(?:\(\?[a-zA-Z]+\))+", pattern) prefix = flags.group() if flags else "" - return f"{prefix}.*(?:{pattern[len(prefix) :]})" + return f"{prefix}(?s:.*)(?:{pattern[len(prefix) :]})" def _convert_ld_values(values: list[str], ld_operator: str) -> list[str]:
Committable suggestion
-
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.Suggested changereturn f"{prefix}.*(?:{pattern[len(prefix) :]})" return f"{prefix}(?s:.*)(?:{pattern[len(prefix) :]})"
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8840 +/- ##
==========================================
- Coverage 98.88% 97.98% -0.90%
==========================================
Files 1683 1683
Lines 70275 70288 +13
==========================================
- Hits 69489 68875 -614
- Misses 786 1413 +627 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
docs/if required so people know about the feature.Changes
The LaunchDarkly importer passes
matchespatterns through unchanged asREGEXconditions. LaunchDarkly evaluatesmatcheswith a search (clause_regex.search(context_value)inldclient/impl/operators.pyof the Python server SDK), so the pattern can match anywhere in the value. Flagsmith evaluatesREGEXwithre.compile(pattern).match(value)(flag_engine.segments.evaluator._evaluate_regex), which only matches from the start. An importedmatchesrule therefore misses values that LaunchDarkly accepts.On
main(d031dbe), a segment with the ruleemail matches "gmail\.com"imports as the conditiongmail\.com. An identity withemail = "user@gmail.com"is in the segment in LaunchDarkly and not in the imported Flagsmith segment. The same happens with^gmail|corpanduser@corp.com.matchespatterns now import as.*(?:<pattern>), so they match anywhere in the value. The group keeps alternation scoped to the pattern.(?i)are kept at the start of the expression. Python 3.11+ raisesre.errorfor global flags that are not at the start, so a plain wrap would break patterns that work today.foo[0-9]{0,1}condition.This follows the
startsWithandendsWithfix..does not match a newline, so a trait value with a newline before the match is still not found; I left that as is, consistent with theendsWithpattern.Review effort: 1/5
How did you test this code?
Added
test_process_import_request__matches_clause__matches_anywhere_in_value. It rewrites thedynamic-listfixture segment to use amatchesclause, runsprocess_import_request, creates an identity with anemailtrait and checks membership throughIdentity.get_segments().emailgmail\.comgmail.comgmail\.comuser@gmail.comgmail\.comuser@example.org^gmail|corpuser@corp.com^gmail|corpuser@gmail.com(?i)GMAIL\.comuser@gmail.comWith
integrations/launch_darkly/services.pychecked out frommain, three cases fail (user@gmail.com,user@corp.com,(?i)GMAIL\.com) and three pass. With the fix, all six pass.Also ran, against the
test-dbPostgres settings fromdocker/api/docker-compose.local.yml:pytest tests/unit/integrations/launch_darkly/: 71 passed.mypy integrations/launch_darkly/ tests/unit/integrations/launch_darkly/: no issues.ruff checkandruff format --checkon both paths: clean.The full
make testsuite andpre-commitwere not run.