fix: get_missing_events endpoint payload according to spec #301
Conversation
WalkthroughThe PR removes the composed Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SDK as FederationSDK
participant PS as ProfilesService
participant ES as EventService
rect `#dfefff`
Note over Client,PS: Previous flow (via ProfilesService)
Client->>SDK: getMissingEvents(...)
SDK->>PS: getMissingEvents(...)
PS->>ES: getMissingEvents(...)
ES-->>PS: events
PS-->>SDK: events
SDK-->>Client: events
end
rect `#e8ffe6`
Note over Client,ES: New flow (direct to EventService)
Client->>SDK: getMissingEvents(...)
SDK->>ES: getMissingEvents(...)
ES-->>SDK: events
SDK-->>Client: events
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #301 +/- ##
==========================================
- Coverage 53.03% 52.82% -0.22%
==========================================
Files 96 96
Lines 12571 12551 -20
==========================================
- Hits 6667 6630 -37
- Misses 5904 5921 +17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
sampaiodiego
force-pushed
the
fix-get-missing-events
branch
from
dcfb896 to
d0b77f9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/homeserver/src/controllers/federation/profiles.controller.ts (1)
98-122: Query parameter handling needs clarification.The
verquery parameter is extracted but unused (line 103), yet the route acceptsquery: t.Any(). Additionally, the room version is hardcoded as['10'](line 105-106).Consider either:
- Implementing the
verquery parameter to support version negotiation- Removing the unused destructuring if version negotiation isn't needed yet
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
packages/core/src/index.ts(0 hunks)packages/core/src/procedures/getMissingEvents.ts(0 hunks)packages/federation-sdk/src/sdk.ts(1 hunks)packages/federation-sdk/src/services/profiles.service.ts(0 hunks)packages/homeserver/src/controllers/federation/profiles.controller.ts(1 hunks)packages/homeserver/src/dtos/federation/profiles.dto.ts(1 hunks)
💤 Files with no reviewable changes (3)
- packages/core/src/procedures/getMissingEvents.ts
- packages/core/src/index.ts
- packages/federation-sdk/src/services/profiles.service.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/homeserver/src/controllers/federation/profiles.controller.ts (5)
packages/homeserver/src/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(4-69)packages/federation-sdk/src/index.ts (4)
federationSDK(207-207)UserID(36-36)RoomID(37-37)EventID(35-35)packages/room/src/types/_common.ts (3)
UserID(20-20)RoomID(16-16)EventID(8-8)packages/homeserver/src/dtos/federation/profiles.dto.ts (7)
MakeJoinParamsDto(52-55)MakeJoinResponseDto(87-122)GetMissingEventsParamsDto(124-126)GetMissingEventsBodyDto(128-146)GetMissingEventsResponseDto(148-152)EventAuthParamsDto(154-157)EventAuthResponseDto(159-163)packages/homeserver/src/middlewares/canAccessResource.ts (1)
canAccessResourceMiddleware(25-69)
🔇 Additional comments (6)
packages/federation-sdk/src/sdk.ts (1)
273-277: LGTM! Delegation to EventService is correct.The delegation from
profilesServicetoeventServicealigns with the architectural changes in this PR. The parameter typing is correctly updated.Note: Codecov indicates line 276 lacks test coverage. Consider adding a test case that exercises this delegation path.
packages/homeserver/src/controllers/federation/profiles.controller.ts (4)
24-27: LGTM! Improved route structure.The refactoring to a returning style with explicit route grouping enhances readability and maintainability.
45-95: LGTM! Proper E2EE endpoint stubs.The keys and devices endpoints are correctly stubbed with 501 status codes and clear error messages indicating E2EE is not implemented. The request/response typing is complete.
124-147: LGTM! Endpoint correctly implements Matrix spec.The change from GET to POST aligns with the Matrix Server-Server API specification (v1.16). The default value handling (
body.limit || 10,body.min_depth || 0) correctly compensates for optional DTO fields that don't auto-populate.
148-166: LGTM! Clean event_auth endpoint implementation.The endpoint is properly structured with correct parameter validation and response typing.
packages/homeserver/src/dtos/federation/profiles.dto.ts (1)
131-145: Based on the specification search results, I can confirm part of the changes but cannot definitively verify the limit maximum.Constraint changes verified against Matrix specification.
The Matrix federation specification example shows
"limit": 10, "min_depth": 0. The changes tomin_depthare correct: the previous defaults and minimums (min: 1, default: 1) violated the spec, and the new values (min: 0, default: 0) align with it.However, the specification examples do not explicitly define a maximum constraint for the
limitparameter. The change from 100 → 20 cannot be verified as spec-compliant or non-compliant based on available documentation. Consider documenting whether this reduction to 20 is:
- A deliberate implementation-specific limit for resource protection
- Aligned with recommendations from federated server implementations (e.g., Synapse)
The defaults and minimum constraints are now spec-compliant.
Summary by CodeRabbit
New Features
Bug Fixes
Chores