fix(call_rate): honor trailing slashes in HttpRequestMatcher and HttpRequestRegexMatcher path matching - #1158
devin-ai-integration[bot] wants to merge 2 commits into
Conversation
…cher path matching Co-Authored-By: bot_apk <apk@cognition.ai>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. 💡 Show Tips and TricksTesting This CDK VersionYou can test this version of the CDK using the following: # Run the CLI from this branch:
uvx 'git+https://github.com/airbytehq/airbyte-python-cdk.git@devin/1789429746-call-rate-trailing-slash#egg=airbyte-python-cdk[dev]' --help
# Update a connector to use the CDK from this branch ref:
cd airbyte-integrations/connectors/source-example
poe use-cdk-branch devin/1789429746-call-rate-trailing-slashPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
There was a problem hiding this comment.
🟡 Changes recommended
Literal matching can incorrectly match sibling endpoints such as /users-other; a path boundary must be preserved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates HTTP request rate-limit matchers to handle trailing slashes consistently and adds regression coverage.
Changes:
- Normalizes literal matcher paths.
- Supports regex matching against normalized and original paths.
- Adds matcher and API budget tests.
File summaries
| File | Summary |
|---|---|
unit_tests/sources/streams/test_call_rate.py |
Adds trailing-slash matcher and budget regression tests. |
airbyte_cdk/sources/streams/call_rate.py |
Updates HTTP path matching behavior. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-Authored-By: bot_apk <apk@cognition.ai>
Overview
👉 TL;DR: Rate-limit request matchers whose URL or regex ends in a trailing slash could never match any request, so the affected API budget policies were silently skipped. This fixes the matchers so trailing slashes on either side are handled consistently.
Specifically,
HttpRequestMatcher.__init__now strips the trailing slash from its literal path and appends a(?=/|$)path boundary, andHttpRequestRegexMatcher.__call__matchesurl_path_patternagainst both the slash-stripped and the original request path.Resolves https://github.com/airbytehq/airbyte-internal-issues/issues/17248:
Changes
HttpRequestRegexMatcher.__call__: previouslyrequest_path = path.rstrip("/")was the only value the regex was applied to, so a pattern like^/users/$(or there.escaped literal/users/) could never match. Now:HttpRequestMatcher.__init__:url_path = parsed_url.path.rstrip("/") or Noneandurl_path_pattern = re.escape(url_path) + r"(?=/|$)", sourl="https://example.com/users/"matchesGET /users,GET /users/andGET /users/123but notGET /users-other; the root-URL special case (/→ no path constraint) is preserved.url_path_patternupdated to describe the matching semantics.unit_tests/sources/streams/test_call_rate.py: both matcher classes with/without trailing slash on pattern and request, the literal path boundary, plusAPIBudget.get_matching_policy/acquire_callselection for a trailing-slash request.Design notes: why keep stripping the request path
Show/Hide Content
Applying the regex only to the raw path would have been the "honor regexes literally" option, but many connector manifests in
airbytehq/airbyterely on the current stripping behavior with patterns such as^groups$(source-gitlab),^/api/lists$(source-klaviyo),reports/2021-06-30/reports$(source-amazon-seller-partner) and expect them to match/foo/requests too. Matching against the stripped path first keeps all of those working unchanged, and additionally trying the original path fixes the previously-impossible slash-requiring patterns.Risks
HttpRequestMatcherliterals without a trailing slash (e.g.url=".../users") previously also matched sibling paths like/users-othervia unanchoredre.search; with the(?=/|$)boundary they no longer do.Test plan
poetry run pytest unit_tests/sources/streams/test_call_rate.py -q(52 passed; the new trailing-slash cases fail onmainwithout the source change)poetry run ruff check .,poetry run ruff format .poetry run mypy --config-file mypy.ini airbyte_cdk/sources/streams/call_rate.pyRequested via
/ai-fixon the linked issue (triage session: https://app.devin.ai/sessions/7ae5f89db63d417d92c2f6a058c46eab).Link to Devin session: https://app.devin.ai/sessions/56f24b43ca1d43ab91527d2ee0c9db30
Open in Devin Desktop: https://app.devin.ai/desktop/session/56f24b43ca1d43ab91527d2ee0c9db30?variant=devin