🐛(backend) support unencoded S3 notification object keys#1455
Conversation
|
@coderabbitai full review |
✅ Action performedFull review finished. |
Confidence Score: 5/5Safe to merge — the change is minimal and correctly normalises S3 object keys for both raw and pre-encoded formats. The normalization logic is correct: No files require special attention. Important Files Changed
Reviews (6): Last reviewed commit: "✅(backend) cover encoded S3 keys with pl..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Caution Review failedFailed to post review comments. We encountered an issue with GitHub. Use ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Walkthrough
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/backend/core/recording/event/parsers.py`:
- Around line 168-169: The filepath normalization in BaseS3Parser is still
re-encoding valid S3 keys because quote_plus(..., safe="%") converts literal
plus signs in already-encoded prefixes, which breaks validate() matching. Update
the parsing logic in the relevant filepath handling so it preserves plus signs
as well as existing percent-encoded separators, or only normalizes the separator
without re-encoding the whole key. Add a regression test around
BaseS3Parser.validate() for an encoded key like recordings+folder%2F<id>.mp4 to
ensure this case still matches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2859aec8-d2e2-4e1e-b12d-ccb131a8d32c
📒 Files selected for processing (3)
CHANGELOG.mdsrc/backend/core/recording/event/parsers.pysrc/backend/core/tests/recording/event/test_parsers.py
| # Preserve already URL-encoded separators while encoding raw S3 object keys. | ||
| filepath = quote_plus(filepath, safe="%") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Parser outline =="
ast-grep outline src/backend/core/recording/event/parsers.py --view expanded || true
echo
echo "== Relevant parser slice =="
nl -ba src/backend/core/recording/event/parsers.py | sed -n '90,190p'
echo
echo "== Existing tests around S3 filepath normalization =="
nl -ba src/backend/core/tests/recording/event/test_parsers.py | sed -n '340,430p'
echo
echo "== Search for filepath regex / output folder encoding assumptions =="
rg -n -C3 'filepath_regex|RECORDING_OUTPUT_FOLDER|quote_plus|quote\(' src/backend/core src/backend/core/testsRepository: suitenumerique/meet
Length of output: 1218
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== parser slice =="
sed -n '145,185p' src/backend/core/recording/event/parsers.py
echo
echo "== tests slice =="
sed -n '330,450p' src/backend/core/tests/recording/event/test_parsers.py
echo
echo "== quote_plus probe =="
python3 - <<'PY'
from urllib.parse import quote_plus
samples = [
"recordings+folder%2Fid.mp4",
"recordings folder%2Fid.mp4",
"recordings%2Bfolder%2Fid.mp4",
"recordings+folder/id.mp4",
]
for s in samples:
print(s, "->", quote_plus(s, safe="%"))
PYRepository: suitenumerique/meet
Length of output: 5445
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '80,145p' src/backend/core/recording/event/parsers.pyRepository: suitenumerique/meet
Length of output: 2634
Avoid re-encoding S3 keys here. quote_plus(..., safe="%") still turns already-encoded + into %2B, so valid notifications with spaces in a prefix stop matching BaseS3Parser.validate(). Preserve + as well, or normalize only the separator, and add a regression test for an encoded key like recordings+folder%2F<id>.mp4.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/core/recording/event/parsers.py` around lines 168 - 169, The
filepath normalization in BaseS3Parser is still re-encoding valid S3 keys
because quote_plus(..., safe="%") converts literal plus signs in already-encoded
prefixes, which breaks validate() matching. Update the parsing logic in the
relevant filepath handling so it preserves plus signs as well as existing
percent-encoded separators, or only normalizes the separator without re-encoding
the whole key. Add a regression test around BaseS3Parser.validate() for an
encoded key like recordings+folder%2F<id>.mp4 to ensure this case still matches.
There was a problem hiding this comment.
Done. I switched the normalization to preserve + as well as existing percent-encoded separators, and added a regression test for an already encoded S3 key with a + in the prefix.
It verifies that valid notifications with spaces encoded as + keep matching BaseS3Parser.validate().
|
let's connect on matrix https://matrix.to/#/#meet-official:matrix.org |
Add docstrings to the S3 parser tests
Correction to preserve already encoded plus signs in addition to slashes.
Add a regression test for already encoded S3 notification keys that contain plus signs in the prefix.
lebaudantoine
force-pushed
the
fix/s3parser-urlencode
branch
from
544941b to
735b4cd
Compare
|
2 participants
Purpose
Fixes #1454.
Some S3-compatible storage providers, such as Ceph RGW, send notification object keys with raw path separators:
However, the recording event parser currently expects URL-encoded separators:
As a result, valid storage notifications are ignored, recordings remain in the
stoppedstate, and notification emails are not sent.This pull request normalizes S3 notification object keys before filepath validation while preserving already encoded separators.
Changes
%2Finto%252F.Testing
Verified that the parser accepts both:
Before this change, the first representation raised
InvalidFilepathError.