feat(federation): implement backfill endpoint #37087
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughAdds Matrix federation backfill support: AJV schemas/validators and a new GET /v1/backfill/:roomId route that requires Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor HS as Remote Homeserver
participant API as Federation API (/v1/backfill/:roomId)
participant EV as Event Service
HS->>API: GET /v1/backfill/{roomId}?v={eventId(s)}&limit={n}
API->>API: Validate params/query (roomId, v required, limit default 100)
alt Missing v
API-->>HS: 400 M_BAD_REQUEST (missing event ID)
else Valid request
API->>EV: getBackfillEvents(roomId, eventIds, limit)
alt Success
EV-->>API: Backfill events
API-->>HS: 200 BackfillResponse
else Failure
EV-->>API: Error
API-->>HS: 500 M_UNKNOWN ("Failed to get backfill events")
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
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
📜 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 (1)
ee/packages/federation-matrix/src/api/_matrix/transactions.ts(2 hunks)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-7.11.0 #37087 +/- ##
===============================================
Coverage 67.38% 67.39%
===============================================
Files 3328 3328
Lines 113342 113342
Branches 20562 20560 -2
===============================================
+ Hits 76375 76382 +7
+ Misses 34363 34358 -5
+ Partials 2604 2602 -2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ee/packages/federation-matrix/src/api/_matrix/transactions.ts (2)
464-472: Document error responses in the schema.The handler returns 400 (lines 478-484) and 500 (lines 495-500) error responses, but only the 200 response is declared in the schema. While some existing endpoints in this file share this pattern, documenting error responses improves API clarity and enables proper validation.
Consider adding error response schemas:
{ params: isBackfillParamsProps, query: isBackfillQueryProps, response: { + 400: isErrorResponseProps, + 500: isErrorResponseProps, }, tags: ['Federation'], license: ['federation'], },
475-475: Validate limit parameter coercion.
Number(c.req.query('limit') || 100)can produceNaNif the query string contains a non-numeric value (e.g.,?limit=abc). Whenlimitbecomes optional (per the previous comment), the schema validator won't catch malformed values that are present. Consider validating the coerced number or relying on the validator to enforce numeric types before this line executes.One approach is to add explicit NaN handling:
- const limit = Number(c.req.query('limit') || 100); + const rawLimit = c.req.query('limit'); + const limit = rawLimit ? Number(rawLimit) : 100; + if (isNaN(limit)) { + return { + body: { + errcode: 'M_BAD_REQUEST', + error: 'Invalid limit parameter', + }, + statusCode: 400, + }; + }
📜 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 (1)
ee/packages/federation-matrix/src/api/_matrix/transactions.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (5)
ee/packages/federation-matrix/src/api/_matrix/transactions.ts (5)
473-485: Good validation of required event IDs.The check for
eventIds?.lengthcorrectly validates that at least one event ID is provided, and the error response follows Matrix spec conventions with appropriate errcode and message.
255-268: Schema definition looks good.The
BackfillParamsSchemacorrectly validates the room ID with the Matrix room pattern, consistent with other room ID validations in this file (e.g., line 93).
293-314: Response schema properly structured.The
BackfillResponseSchemacorrectly reusesEventBaseSchemafor PDU validation and follows the established pattern of other response schemas in this file.
270-286: Makelimitoptional to match the handler's default behavior.The schema marks
limitas required (line 284), but the handler provides a default value of 100 when it's missing (line 475). This inconsistency will cause the validator to reject valid requests that omit the limit parameter, preventing the default from ever being applied.Apply this diff to align the schema with the implementation:
- required: ['limit', 'v'], + required: ['v'],
487-493: Verify that the SDK implementsgetBackfillEventsmethod.The call to
event.getBackfillEvents(roomId, eventIds as EventID[], limit)on line 488 follows the same pattern as other event service methods in this file (e.g.,getEventById,getState,getStateIds). However, theeventservice is provided by the external@rocket.chat/federation-sdkpackage (version 0.1.11), and thegetBackfillEventsmethod definition cannot be found in this repository.Ensure that:
- The SDK has been updated to include the
getBackfillEventsmethod with the expected signature(roomId: string, eventIds: EventID[], limit: number)- The method returns data conforming to the
BackfillResponseSchemastructure (withorigin,origin_server_ts, andpdusfields)- The SDK version dependency is correctly specified to include this new method
ggazzo
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Co-authored-by: Guilherme Gazzo <5263975+ggazzo@users.noreply.github.com>
Co-authored-by: Guilherme Gazzo <5263975+ggazzo@users.noreply.github.com>
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Chores