Anchor metadata pattern matching so foreach paths do not prefix-match - #3342
Conversation
Greptile SummaryThe PR anchors local metadata pattern matching to the complete metadata value, preventing foreach indices such as
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| metaflow/plugins/metadata_providers/local.py | Narrows metadata filtering to whole-value matches while preserving explicit descendant wildcard patterns. |
| test/unit/test_local_metadata_provider.py | Adds focused regression coverage for foreach prefix collisions and supported wildcard cases. |
Reviews (5): Last reviewed commit: "Merge branch 'master' into fix-foreach-p..." | Re-trigger Greptile
|
Thanks for picking this up. I checked the reasoning rather than taking it on trust, and the three pattern shapes are exactly as you describe — One thing I could not settle from this repo, which might matter for how the fix is framed:
If the service side anchors the same way For context, I filed #3341 — glad it's getting fixed. |
|
Thanks for checking it rather than taking it on trust, and for filing #3341 in the first place. I went looking for the service side, and I think the question is answerable. The filtering happens in Netflix/metaflow-service, if pattern:
conditions.append("regexp_match(value, %s) IS NOT NULL")
values.append(pattern)
So yes, the two providers diverge after this merges, and they already diverged before it on the leading-substring case. Anchoring server-side would mean wrapping the pattern, roughly I agree that does not belong in this PR: it is a different repo, it is a behaviour change for anyone relying on the current substring semantics, and it wants a maintainer's call on whether to anchor the service or relax the client. Happy to open an issue on metaflow-service with the above if a maintainer thinks it is worth tracking. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3342 +/- ##
=========================================
Coverage ? 31.00%
=========================================
Files ? 382
Lines ? 52717
Branches ? 9303
=========================================
Hits ? 16346
Misses ? 35163
Partials ? 1208 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…#3357) ## PR Type - [x] Bug fix ## Summary Handles the OSS service-provider side of #3341. `Task.parent_tasks` and `Task.child_tasks` pass foreach execution-path patterns to the metadata service, whose regex matching is unanchored. An exact path such as `middle:1` could therefore also select `middle:10` and `middle:11`. The local-provider side is handled separately by #3342. ## Root Cause `ServiceMetadataProvider.filter_tasks_by_metadata` forwarded the raw pattern to the metadata service: ```python query_params["pattern"] = pattern ``` The service applies that pattern as an unanchored regex search. ## Fix Wrap the forwarded pattern in full-string anchors: ```python query_params["pattern"] = "^(?:%s)$" % pattern ``` This makes `middle:1` exact while preserving the descendant pattern `middle:1,.*`. The existing `.*` match-all case remains short-circuited and sends no pattern. ## Tests `test/unit/test_service_metadata_provider.py` covers: - exact path anchoring; - descendant pattern anchoring; - the match-all short circuit. Run with: ```bash python -m pytest test/unit/test_service_metadata_provider.py -q ``` ## Non-Goals - The local metadata provider and its tests are handled by #3342. - No metadata-service repository change is required; anchoring happens at the OSS client boundary. ## AI Tool Usage - [x] AI tools were used. Tool disclosed by the author: Claude Code, used to help implement the original change and tests. The service-only rescope retains that implementation. --------- Co-authored-by: Shashank Srikanth <ssrikanth@netflix.com>
Addresses the local-provider side of #3341. The service-provider side is handled by #3357.
LocalMetadataProvider.filter_tasks_by_metadatamatched metadata values withregex.match, which anchors only at the start.Task.parent_tasks/child_tasksbuild their pattern fromforeach-execution-path, so once a foreach has 11 or more items the patternmiddle:1also matched the valuesmiddle:10andmiddle:11, and the client resolved the wrong tasks. Withspinthis turns into a hard failure ("not a join step but gets multiple inputs").The fix switches that one call to
regex.fullmatch, so a pattern has to match the whole value.The reason
fullmatchis safe here is that the patterns this function receives are already written to cover the part of the path they care about. Frommetaflow/client/core.py:middle:1— should match only that task, which is what this fixesf"{current_path},.*"— the trailing.*still consumes the rest of the value, so nested foreach lookups are unaffected".*"for the match-all case — unaffected, and it is short-circuited before the regex anywayI checked the values that a 12 item foreach produces:
re.match(before)re.fullmatch(after)middle:1middle:1,middle:10,middle:11,middle:1,inner:0middle:1middle:1,.*middle:1,inner:0middle:1,inner:0.*Only the over-matching case changes.
The service metadata provider passes the pattern to the backend instead of matching locally, so this is the only client-side matcher affected.
Added
test_filter_tasks_by_metadata_does_not_match_prefixesintest/unit/test_local_metadata_provider.py, covering the exact path, the prefixed sibling, a nested descendant pattern and match-all. Verified it fails before the change (themiddle:1lookup returns three extra tasks) and passes after; the existing test in that file still passes.