feat: adds get event by id route by ricardogarim · Pull Request #175 · RocketChat/homeserver

ricardogarim · 2025-09-10T23:58:15Z

Summary by CodeRabbit

New Features
- Added a federation API endpoint to fetch an event by ID, returning origin_server_ts, origin, and PDUs.
Improvements
- Consistent server origin embedded in responses.
- Clearer, typed error responses for the new endpoint (401/403/404/500).
- Added descriptive route metadata for federation endpoints, including the transaction send route.

codecov-commenter · 2025-09-10T23:59:54Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.42%. Comparing base (76956e6) to head (a47b32a).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #175   +/-   ##
=======================================
  Coverage   80.42%   80.42%           
=======================================
  Files          58       58           
  Lines        4516     4516           
=======================================
  Hits         3632     3632           
  Misses        884      884

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

packages/homeserver/src/controllers/federation/transactions.controller.ts

coderabbitai · 2025-09-15T15:01:55Z

Caution

Review failed

The pull request is closed.

Walkthrough

Adds GET /_matrix/federation/v1/event/:eventId and related DTOs; integrates ConfigService into federation transactions controller; refactors route registration to chained app.put(...).get(...); GET returns event PDUs with embedded origin and origin_server_ts or a standardized error.

Changes

Cohort / File(s)	Summary
Federation Transactions Controller `packages/homeserver/src/controllers/federation/transactions.controller.ts`	Introduces `ConfigService` via `container.resolve`; refactors route registration to a fluent chain (`app.put(...).get(...)`); adds `GET /_matrix/federation/v1/event/:eventId` handler using `GetEventParamsDto`, returns `GetEventResponseDto` on success (embedding `origin` from `ConfigService` and `origin_server_ts`) and `GetEventErrorResponseDto` on error; retains `PUT /_matrix/federation/v1/send/:txnId` with metadata.
Federation DTOs for GetEvent `packages/homeserver/src/dtos/federation/transactions.dto.ts`	Adds `GetEventParamsDto`, `GetEventResponseDto`, `GetEventErrorResponseDto` and exported types `GetEventParams` / `GetEventResponse`; no other DTO changes.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant C as Federation peer
  participant HS as Homeserver App
  participant TC as TransactionsController
  participant ES as EventService
  participant CS as ConfigService

  rect rgb(240,248,255)
    note over C,HS: GET /_matrix/federation/v1/event/:eventId
    C->>HS: HTTP GET /event/:eventId
    HS->>TC: invoke GET handler (params validated)
    TC->>ES: getEventById(eventId)
    ES-->>TC: event | null
    alt event found
      TC->>CS: read serverName
      CS-->>TC: origin (serverName)
      TC-->>C: 200 { origin_server_ts, origin, pdus:[ event(with origin) ] }
    else not found
      TC-->>C: 404 { errcode: M_NOT_FOUND, error }
    end
  end

sequenceDiagram
  autonumber
  participant FP as Federation peer
  participant HS as Homeserver App
  participant TC as TransactionsController

  rect rgb(245,255,245)
    note over FP,HS: PUT /_matrix/federation/v1/send/:txnId
    FP->>HS: HTTP PUT { pdus, edus }
    HS->>TC: invoke PUT handler
    TC-->>FP: 200 { pdus:[], edus:[] } or 400 on invalid
  end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

feat: adds get event by id route #175 — Adds same GET /_matrix/federation/v1/event/:eventId route and GetEvent DTOs; appears to overlap on controller and DTO changes.

Suggested reviewers

ggazzo

Poem

I hop through routes where events are found,
New paths bloom softly without a sound.
A timestamp twinkles, an origin bright,
PDUs tucked in, all snug for the night.
I nibble DTO carrots and do a small dance—🥕🐇

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title "feat: adds get event by id route" accurately and concisely summarizes the primary change in this PR — adding a GET event-by-id route (and associated DTOs) — and is clear for teammates scanning history.
Docstring Coverage	✅ Passed	No functions found in the changes. Docstring coverage check skipped.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 66fdcf3 and a47b32a.

📒 Files selected for processing (2)

packages/homeserver/src/controllers/federation/transactions.controller.ts (1 hunks)
packages/homeserver/src/dtos/federation/transactions.dto.ts (1 hunks)

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 4

♻️ Duplicate comments (1)

packages/homeserver/src/controllers/federation/transactions.controller.ts (1)

17-41: Prior feedback addressed: routes are properly chained.

The .put().get() chaining resolves the earlier review note.

🧹 Nitpick comments (4)

packages/homeserver/src/dtos/federation/transactions.dto.ts (4)
32-34: Validate Matrix event ID format.

Early-validate :eventId to reduce unnecessary lookups and 404s.
 export const GetEventParamsDto = t.Object({
-	eventId: t.String({ description: 'Event ID' }),
+	eventId: t.String({
+		description: 'Event ID',
+		// Matrix event IDs look like `$something:server`
+		pattern: '^\\$.+:.+$',
+	}),
 });
36-42: Constrain response to exactly one PDU.

Contract says “single PDU”; enforce it in the schema.
 export const GetEventResponseDto = t.Object({
 	origin_server_ts: t.Number({ description: 'Origin server timestamp' }),
 	origin: t.String({ description: 'Origin server' }),
 	pdus: t.Array(EventBaseDto, {
-		description: 'An array containing a single PDU',
+		description: 'An array containing a single PDU',
+		minItems: 1,
+		maxItems: 1,
 	}),
 });
1-2: Optional: Reuse TimestampDto for consistency.

Align the top-level origin_server_ts with the scalar used in events.
-import { EventBaseDto } from '../common/event.dto';
+import { EventBaseDto, TimestampDto } from '../common/event.dto';
-	origin_server_ts: t.Number({ description: 'Origin server timestamp' }),
+	origin_server_ts: TimestampDto,
44-47: Unify error DTOs across federation endpoints.

Consider a shared FederationErrorResponseDto to avoid divergence with ErrorResponseDto and keep err shapes consistent.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4247c69 and 66fdcf3.

📒 Files selected for processing (2)

packages/homeserver/src/controllers/federation/transactions.controller.ts (1 hunks)
packages/homeserver/src/dtos/federation/transactions.dto.ts (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

packages/homeserver/src/dtos/federation/transactions.dto.ts (1)

packages/homeserver/src/dtos/common/event.dto.ts (1)

EventBaseDto (19-36)

packages/homeserver/src/controllers/federation/transactions.controller.ts (3)

packages/federation-sdk/src/services/config.service.ts (1)

ConfigService (74-147)

packages/homeserver/src/dtos/federation/transactions.dto.ts (5)

SendTransactionBodyDto (8-21)

SendTransactionResponseDto (23-30)

GetEventParamsDto (32-34)

GetEventResponseDto (36-42)

GetEventErrorResponseDto (44-47)

packages/homeserver/src/dtos/common/error.dto.ts (1)

ErrorResponseDto (3-6)

🔇 Additional comments (2)

packages/homeserver/src/dtos/federation/transactions.dto.ts (1)

52-53: Type exports look good.
packages/homeserver/src/controllers/federation/transactions.controller.ts (1)
62-67: Align declared 401/403/500 responses with behavior.

Either add auth/permission checks that set these statuses or remove them from the schema to avoid misleading OpenAPI.

Option A: keep and implement (preferred).
Option B (schema-only):
 				response: {
-					401: GetEventErrorResponseDto,
-					403: GetEventErrorResponseDto,
-					500: GetEventErrorResponseDto,
 				},

packages/homeserver/src/controllers/federation/transactions.controller.ts

ricardogarim marked this pull request as ready for review September 11, 2025 00:01

ggazzo reviewed Sep 15, 2025

View reviewed changes

packages/homeserver/src/controllers/federation/transactions.controller.ts Outdated Show resolved Hide resolved

coderabbitai bot reviewed Sep 15, 2025

View reviewed changes

ricardogarim added 4 commits September 15, 2025 12:11

feat: adds get event by id route

1b31747

chore: makes routes chained

961c95e

chore: makes routes chained

62a1d95

chore: makes routes chained

a47b32a

ricardogarim force-pushed the feat/get-event-by-id-route branch from ce3f411 to a47b32a Compare September 15, 2025 15:13

ggazzo approved these changes Sep 15, 2025

View reviewed changes

ggazzo merged commit 7a737f7 into main Sep 15, 2025
2 checks passed

ggazzo deleted the feat/get-event-by-id-route branch September 15, 2025 15:14

This was referenced Sep 15, 2025

feat: adds event ACL #161

Merged

refactor: replace string event IDs with EventID #200

Merged

This was referenced Sep 23, 2025

refactor: rename @hs/federation-sdk to @rocket.chat/federation-sdk #211

Merged

refactor: add isAuthenticated and canAccessResource middlewares #224

Merged

coderabbitai bot mentioned this pull request Oct 31, 2025

feat: introduce FederationSDK class #292

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: adds get event by id route#175

feat: adds get event by id route#175
ggazzo merged 4 commits intomainfrom
feat/get-event-by-id-route

ricardogarim commented Sep 10, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

codecov-commenter commented Sep 10, 2025 •

edited

Loading

Uh oh!

Uh oh!

coderabbitai bot commented Sep 15, 2025 •

edited

Loading

Review failed

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

ricardogarim commented Sep 10, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

codecov-commenter commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

coderabbitai bot commented Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review failed

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ricardogarim commented Sep 10, 2025 •

edited by coderabbitai bot

Loading

codecov-commenter commented Sep 10, 2025 •

edited

Loading

coderabbitai bot commented Sep 15, 2025 •

edited

Loading