fix(profiler): דגלי $project נשמרים אמיתיים, אחרת השאילתה השמורה היא פעולה אחרת - #3350
Merged
Merged
Conversation
…פעולה אחרת
``$project`` נשאר בשלד המנורמל, ולכן כל שאילתה שנשמרה עם ערכים אמיתיים
והכילה אותו יצאה עם ``{"file_name": "<value>"}``. וזו אינה היטלה מוסתרת:
מחרוזת שאינה מתחילה ב-``$`` בתוך היטלה נקראת כ**קבוע**, כלומר "החזר את
הקבוע ``<value>``" במקום "החזר את השדה ``file_name``".
נמדד מול MongoDB 8.0.32: ה-``explain`` מחזיר את השלב בתור
``{"file_name": {"$const": "<value>"}}``, ו-``queryShapeHash`` שלו שונה
מזה של ההיטלה האמיתית — כלומר המנוע עצמו סופר אותן כשתי שאילתות. שלב
שמושך את ``code`` נותח כשלב שאינו קורא שום שדה.
הכשל היה שקט לגמרי: בניגוד ל-``$limit``, מחרוזת בהיטלה אינה גורמת למונגו
לזרוק, ולכן ``_fix_pipeline_for_explain`` — שמתקנת רק את השלבים שזורקים —
לא נגעה בה.
הוולידציה צרה בכוונה, וזו אינה החמרה קוסמטית. התיעוד של ``$project`` אומר
"Non-zero integers are also treated as true", כלומר
``{"file_name": 6865105071}`` הוא היטלת הכללה חוקית לגמרי שמזהה משתמש
יושב בה בגלוי; סריקת הבעלות עוברת על גופי ``$match`` בלבד ולעולם לא תראה
אותו. לכן מותרים ``0``/``1``/בוליאני/נתיב שדה בלבד, ברקורסיה גם על היטלה
מקוננת.
הכל-או-כלום מכוון: ``$project`` שיש בו ולו ביטוי אחד חוזר כשלד **שלם**
ולא כתערובת של ערכים אמיתיים ו-``<value>`` — שלב שחציו אמיתי נראה שלם
ומתפרש אחרת ממה שרץ, וזה הכשל עצמו בקנה מידה קטן יותר. ובניגוד
ל-``$limit``, הענף הזה **אינו זורק**: ``$project`` שאינו דגלים הוא
לגיטימי, ולזרוק שם היה מוחק מהדוח שאילתות תקינות.
``$addFields``/``$set`` ו-``$unset`` של אגרגציה נשארו בחוץ — אין להם היום
מופע אמיתי לאמת מולו, כמו העמדה שכבר ננקטת בקובץ לגבי ``$search``.
אימות: הפלט של הקוד המתוקן הורץ דרך ``explain`` האמיתי, ו-``queryShapeHash``
שלו זהה לזה של השאילתה שרצה בפועל (``312C72B3…``) ושונה מזה של הקוד הישן
(``E6E56114…``). בקרה שלילית: היטלה אחרת מקבלת hash שלישי (``B48196ED…``),
כלומר ה-hash אכן רגיש להיטלה וההשוואה אינה ריקה.
חמישה מהטסטים החדשים הורצו על הקוד שלפני התיקון ונפלו.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UBugD1DV8LhHBSGnvpAgzK
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Sorry @amirbiron, you've used your own review budget of 250,000 diff characters for the last 7 days.
You can request another review in 1 hour and 56 minutes by commenting @sourcery-ai review. Upgrade to get a review now.
Contributor
🧯 Dangerous deletes guard reportPolicy: see .cursorrules — dangerous deletions are blocked unless wrapped safely. Summary:
Flagged findings (file:line:snippet): Excluded matches (by path pattern) |
Contributor
Reviewer's GuideThe PR fixes profiler query replay by narrowly recognizing recursive, flag-only Sequence diagram for profiler query replay with $projectsequenceDiagram
participant Profiler
participant StructuralStage as _structural_stage_value
participant Projection as _projection_structure
participant RawPipeline as _raw_pipeline
participant MongoDB
Profiler->>StructuralStage: _structural_stage_value("$project", body)
StructuralStage->>Projection: _projection_structure(body)
alt projection contains only structural flags
Projection-->>StructuralStage: validated projection
StructuralStage-->>RawPipeline: preserve projection
else projection contains an expression
Projection-->>StructuralStage: _NOT_STRUCTURAL
StructuralStage-->>RawPipeline: normalized projection skeleton
end
RawPipeline->>MongoDB: explain(saved pipeline)
MongoDB-->>Profiler: queryShapeHash
Flow diagram for replayable MongoDB projection handlingflowchart TD
A["$project body"] --> B{_projection_structure}
B -->|"Only 0, 1, booleans, $ paths, or nested flags"| C["Preserve real projection"]
B -->|"Contains an expression or constant"| D["Keep projection as normalized skeleton"]
C --> E["_raw_pipeline stores query_raw"]
D --> E
E --> F["Profiler explain uses matching query shape"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Warning Review limit reachedNext included review available in 43 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughהפרופיילר שומר כעת מבני Changesשחזור שאילתות עם
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to This change preserves valid $project structure in profiler query records while retaining normalization for unsupported or mixed projections. No actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant Test
participant PersistentQueryProfilerService
participant MongoDB
Test->>PersistentQueryProfilerService: הפעלת _decide_raw_query
PersistentQueryProfilerService->>PersistentQueryProfilerService: אימות _projection_structure
PersistentQueryProfilerService-->>Test: הפייפליין השמור
Test->>MongoDB: הרצת explain וחישוב queryShapeHash
MongoDB-->>Test: חתימת צורת השאילתה
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 28 functions across 3 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
Contributor
⏱️ Performance report(No performance test durations collected. Mark tests with |
Contributor
📖 Documentation PreviewThe documentation has been built successfully!
To view locally:
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…תקלה **בלוק הכותרת סתר את ההערה החדשה.** הוא הצהיר "מה נשמר עם ערכים אמיתיים: תנאי סינון בלבד... כל שאר השלבים נשארים בשלד" — בעוד ההערה בסריקת הבעלות אומרת שצרוּת ``_projection_structure`` היא הדבר היחיד שמונע מזהה משתמש להישמר דרך ``$project``. הבלוק מתאר עכשיו שתי משפחות: תנאי סינון (ולידציה מול רשימה) וערכי מבנה (ולידציה מול צורה צרה), ואומר מפורשות שסריקת הבעלות אינה עוברת על השנייה. ובאותו מעבר תועד גם מה שהתיקון **אינו** מכסה: המשפחה השנייה חלה על ``query_raw`` בלבד. ``query_shape`` ממשיך להחליף דגלי ``$project`` ב-``<value>``, וכפתור הניתוח נופל עליו כשאין ``query_raw`` — כלומר בברירת המחדל, כש-``PROFILER_UNREDACTED_USER_IDS`` ריק. **``except (ServerSelectionTimeoutError, Exception)`` בלע הכל.** האיבר הראשון מת מול השני, ולכן כל תקלת קונפיגורציה — ``mongodb+srv://`` בלי ``dnspython``, אימות שגוי — הייתה הופכת לדילוג עם הסיבה השקרית "שרת לא נגיש", ושתי הבדיקות שהן ההוכחה היחידה מקצה לקצה לתיקון היו נעלמות בשקט. עכשיו נתפסת אי-נגישות בלבד, הודעת הדילוג נוקבת בשגיאה עצמה, וכל השאר עולה בקול. ובנוסף: בדיקת הנגישות ירדה מרמת המודול ל-fixture. קודם היא פתחה חיבור בכל **איסוף** של pytest, גם בהרצה שאינה כוללת את הקובץ הזה. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UBugD1DV8LhHBSGnvpAgzK
amirbiron
pushed a commit
that referenced
this pull request
Sep 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
✨ תיאור קצר
$projectנשאר בשלד המנורמל, ולכן כל שאילתה שנשמרה ב-query_rawעם ערכים אמיתיים והכילה אותו יצאה עם{"file_name": "<value>"}. וזו אינה היטלה מוסתרת — מחרוזת שאינה מתחילה ב-$בתוך היטלה נקראת כקבוע, כלומר "החזר את הקבוע<value>" במקום "החזר את השדהfile_name". התיקון מסווג דגלי היטלה כמבנה, בדיוק כמו שכיווני$sortכבר מסווגים.📦 שינויים עיקריים
פירוט:
_projection_structureחדשה: גוף$projectשכולו0/1/בוליאני/נתיב שדה מסווג כמבנה ונשמר אמיתי, ברקורסיה גם על היטלה מקוננת_structural_stage_valueמקבל ענף$project— ובניגוד ל-$limit/$sort, הוא אינו זורק_structural_stage_value,_raw_pipeline, וההנמקה של סריקת הבעלות)docs/observability/query-performance-profiler.rst+docs/whats-new.rst🔬 מה בדיוק היה שבור
נמדד מול MongoDB 8.0.32. ה-
explainמחזיר את השלב השמור בתור:{"$project": {"file_name": {"$const": "<value>"}, "code": {"$const": "<value>"}}}$const— ארבע השמות של קבועים. שלב שבמציאות מושך אתcode, השדה הכבד ביותר באוסף, נותח כשלב שאינו קורא שום שדה.והכשל היה שקט לגמרי: בניגוד ל-
$limit, מחרוזת בהיטלה אינה גורמת למונגו לזרוק, ולכן_fix_pipeline_for_explain— שמתקנת רק את השלבים שזורקים ($limit,$skip,$sample) — לא נגעה בה.🔒 למה הוולידציה צרה, וזו אינה החמרה קוסמטית
התיעוד של
$projectאומר "Non-zero integers are also treated as true". כלומר{"$project": {"file_name": 6865105071}}הוא היטלת הכללה חוקית לגמרי, ומזהה משתמש יושב בה בגלוי. סריקת הבעלות עוברת על גופי$matchבלבד ולעולם לא תראה אותו — ולכן הדבר היחיד שמונע אותו הוא צרוּת הוולידציה.0ו-1בלבד סוגרים את הדלת; יש טסט ייעודי על כך.מקור: https://www.mongodb.com/docs/manual/reference/operator/aggregation/project/
הכל-או-כלום מכוון:
$projectשיש בו ולו ביטוי אחד חוזר כשלד שלם, ולא כתערובת של ערכים אמיתיים ו-<value>. שלב שחציו אמיתי נראה שלם ומתפרש אחרת ממה שרץ — זה הכשל עצמו בקנה מידה קטן יותר.ההיקף שנשאר בחוץ, במפורש:
$addFields/$setו-$unsetשל אגרגציה. בשניהם ערך שהוא נתיב שדה בלבד הוא מבנה זהה, אבל הסיכון לקבוע חופשי גבוה יותר ואין להם היום מופע אמיתי לאמת מולו — כמו העמדה שהקובץ כבר נוקט לגבי$search. יש טסטים שמקבעים שהם נשארו בשלד.🧪 בדיקות
291 טסטי פרופיילר עוברים, אפס רגרסיות.
tests/test_profiler_projection_is_replayable.py(19 טסטים) — לוגיקה בפייתון טהור, רץ בכל מקום.tests/test_profiler_projection_mongo.py(2 טסטים) — הצרכן האמיתי, מדלג בליMONGODB_URL.חמישה מהטסטים החדשים הורצו על הקוד שלפני התיקון ונפלו:
השאר הם טסטי גדר — הם אמורים לעבור בשני הצדדים, כי הם מאמתים התנהגות שלא השתנתה.
אימות מקצה לקצה, מול
explainהאמיתיהפלט של הקוד המתוקן הורץ דרך
explain, והושווה ב-queryShapeHash— טביעת האצבע שמונגו עצמה נותנת לצורת שאילתה:queryShapeHash312C72B3…2556312C72B3…2556✅E6E56114…9717B48196ED…B46Bהבקרה השלילית נחוצה: בלעדיה ההשוואה הראשונה הייתה עוברת גם אם ה-hash מתעלם מ-
$projectלגמרי, ואז היא מוודאת שהשוואת מחרוזות עובדת ותו לא.📝 סוג שינוי
✅ צ'קליסט
docs/observability/query-performance-profiler.rst| המשפט: "וגם ערכי המבנה:$limit,$skipוכיווני$sort. אלה אינם נתוני משתמש אלא צורת השאילתה... הם נשמרים אמיתיים כי בלעדיהם הניתוח מתאר שאילתה אחרת." — זה בדיוק הנימוק שהורחב כאן ל-$project, ולכן העמוד עודכן.docs/doc-authoring.rst(בלי ספירת מופעים בפרוזה),docs/versioning-stable-anchors.rst(What's New),docs/testing.rst(מוסכמת טסטי מונגו)🧩 השפעות/סיכונים
query_raw, שהוא העשרה לניתוח.1.0נדחה).$matchוהסינון לא נגעו כלל, ויש טסטים שמקבעים זאת.0/1— נחסמת במפורש ונבדקת.🔗 קישורים
ה-docstring של
tests/test_note_boards_mongo.pyטוען "מתי הן רצות: כש-MONGODB_URLמוגדר והשרת נענה. ב-CI זה תמיד". זה אינו נכון: ג'ובUnit Testsרץruns-on: ubuntu-latestבליcontainer:, והשירותmongodbמוגדר בליports:, ולכן שם השירות אינו נפתר וכל הטסטים האלה מדלגים בשקט.docs/testing.rstו-.github/workflows/ci.ymlעצמו (בהערה בשורה 236) מתעדים זאת נכון — רק ה-docstring סותר. לא תוקן כאן כי זה מחוץ להיקף.🤖 Generated with Claude Code
https://claude.ai/code/session_01UBugD1DV8LhHBSGnvpAgzK
Generated by Claude Code
Summary by Sourcery
Fix profiler query replay by preserving safely validated
$projectstructure in raw aggregation pipelines.Bug Fixes:
$projectprojection flags and field paths in profiler raw queries so replayed explains represent the original query instead of treating placeholders as constants.Enhancements:
$projectbodies while keeping expressions redacted and rejecting non-standard numeric flags that could expose identifiers.Documentation:
$projectreplay behavior.Tests:
queryShapeHashvalues for saved, actual, and deliberately different projections.