Add GetNewestEnvelope API Endpoint #741
Conversation
WalkthroughThis change introduces a new API method, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Service
participant DB_Queries
Client->>API_Service: GetNewestEnvelope(topics)
API_Service->>DB_Queries: SelectNewestFromTopics(topics)
DB_Queries-->>API_Service: Newest envelopes per topic
API_Service-->>Client: GetNewestEnvelopeResponse (ordered envelopes)
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (5)
📒 Files selected for processing (13)
✅ Files skipped from review due to trivial changes (5)
🚧 Files skipped from review as they are similar to previous changes (8)
⏰ Context from checks skipped due to timeout of 90000ms (5)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
pkg/api/message/service.go (2)
439-440: Incorrect logger method name.The logger context is using "GetInboxIds" method name instead of "FetchKeyPackages".
- logger := s.log.With(zap.String("method", "GetInboxIds")) + logger := s.log.With(zap.String("method", "FetchKeyPackages"))
441-444: Add validation for empty installation keys.The method doesn't check if the installationKeys slice is empty, which could lead to unnecessary database query.
queries := queries.New(s.store) installationKeys := req.GetInstallationKeys() + if len(installationKeys) == 0 { + return &message_api.FetchKeyPackagesResponse{ + OriginatorEnvelopes: []*envelopesProto.OriginatorEnvelope{}, + }, nil + } topics := make([][]byte, len(installationKeys)) originalSort := make(map[string]int)pkg/api/message/keyPackages_test.go (1)
61-133: Comprehensive test for the new FetchKeyPackages API.The test covers important scenarios: fetching multiple installation IDs, a single ID, and out-of-order IDs. It verifies both ordering preservation and that only the newest key package per installation is returned.
Consider adding test cases for error scenarios such as:
- Empty installation keys list
- Installation key with no matching key package
- Database query errors (could be mocked)
This would improve test coverage for error handling paths in the API method.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
dev/gen/protosis excluded by!**/gen/**pkg/proto/openapi/mls/api/v1/mls.swagger.jsonis excluded by!pkg/proto/**pkg/proto/openapi/xmtpv4/message_api/message_api.swagger.jsonis excluded by!pkg/proto/**pkg/proto/xmtpv4/message_api/message_api.pb.gois excluded by!**/*.pb.go,!pkg/proto/**pkg/proto/xmtpv4/message_api/message_api.pb.gw.gois excluded by!**/*.pb.gw.go,!pkg/proto/**pkg/proto/xmtpv4/message_api/message_api_grpc.pb.gois excluded by!**/*.pb.go,!pkg/proto/**tools/go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
pkg/api/message/keyPackages_test.go(1 hunks)pkg/api/message/service.go(1 hunks)pkg/db/queries/congestion.sql.go(1 hunks)pkg/db/queries/db.go(1 hunks)pkg/db/queries/envelopes.sql.go(8 hunks)pkg/db/queries/identityUpdates.sql.go(1 hunks)pkg/db/queries/indexer.sql.go(1 hunks)pkg/db/queries/models.go(1 hunks)pkg/db/queries/node.sql.go(1 hunks)pkg/db/queries/nonceManager.sql.go(1 hunks)pkg/db/queries/payerReports.sql.go(1 hunks)pkg/db/sqlc/envelopes.sql(1 hunks)tools/go.mod(9 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/db/queries/envelopes.sql.go (3)
pkg/db/queries/db.go (1)
Queries(23-25)pkg/db/queries/models.go (1)
GatewayEnvelope(27-34)pkg/envelopes/originator.go (1)
OriginatorEnvelope(13-16)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd-cli)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd)
- GitHub Check: Upgrade Tests
- GitHub Check: Test (Node)
- GitHub Check: Lint-Go
- GitHub Check: Code Review
🔇 Additional comments (26)
pkg/db/queries/nonceManager.sql.go (1)
3-3: Sqlc tool version updated to v1.29.0The version of the sqlc code generation tool has been updated from v1.28.0 to v1.29.0. This is consistent with similar version bumps in other generated files.
pkg/db/queries/envelopes.sql.go (6)
3-3: Sqlc tool version updated to v1.29.0The version of the sqlc code generation tool has been updated from v1.28.0 to v1.29.0, matching the version bump in other generated files.
28-32: SQL formatting changesThe SQL query has been reformatted for better readability while preserving its functionality.
63-67: SQL formatting changesThe SQL query has been reformatted for better readability while preserving its functionality.
76-93: SQL formatting changesThe SQL query has been reformatted with improved indentation and line breaks for better readability while preserving its functionality.
119-122: SQL formatting changesThe SQL query has been reformatted for better readability while preserving its functionality.
141-150: SQL formatting changesThe SQL query has been reformatted with improved indentation and line breaks for better readability while preserving its functionality.
pkg/db/sqlc/envelopes.sql (8)
1-17: SQL formatting changesThe SQL query for InsertGatewayEnvelope has been reformatted with improved indentation and line breaks for better readability while preserving its functionality.
19-27: SQL formatting changesThe SelectGatewayEnvelopes query has been reformatted for better readability while preserving its functionality.
29-31: SQL formatting changesThe InsertStagedOriginatorEnvelope query has been reformatted for better readability while preserving its functionality.
33-38: SQL formatting changesThe SelectStagedOriginatorEnvelopes query has been reformatted for better readability while preserving its functionality.
44-50: SQL formatting changesThe SelectVectorClock query has been reformatted for better readability while preserving its functionality.
52-55: SQL formatting changesThe GetLatestSequenceId query has been reformatted for better readability while preserving its functionality.
57-61: SQL formatting changesThe GetLatestCursor query has been reformatted for better readability while preserving its functionality.
63-74: New query for fetching newest envelopes by topicThis new SQL query implements the core functionality for the FetchKeyPackages API by:
- Using a subquery to find the maximum gateway_time for each topic in the provided list
- Joining this back to the gateway_envelopes table to get the complete envelope records
- Ordering results by topic to ensure consistent return ordering
The implementation is efficient as it uses a single query with a join rather than multiple separate queries.
pkg/db/queries/node.sql.go (1)
3-3: Sqlc tool version updated to v1.29.0The version of the sqlc code generation tool has been updated from v1.28.0 to v1.29.0, consistent with similar version bumps in other generated files.
pkg/db/queries/db.go (1)
3-3: Update sqlc version annotation to v1.29.0.
Only the code‐generation header was bumped; no functional code was modified.pkg/db/queries/payerReports.sql.go (1)
3-3: Update sqlc version annotation to v1.29.0.
This is an auto-generated file; no logic changes.pkg/db/queries/congestion.sql.go (1)
3-3: Update sqlc version annotation to v1.29.0.
No functional changes aside from the tooling version bump.pkg/db/queries/models.go (1)
3-3: Update sqlc version annotation to v1.29.0.
Generated code only; structure and types unchanged.pkg/db/queries/identityUpdates.sql.go (1)
3-3: Update sqlc version annotation to v1.29.0.
Only metadata was updated; SQL queries and Go methods remain identical.pkg/db/queries/indexer.sql.go (1)
3-3: Code generation version updated.The sqlc version has been updated from v1.28.0 to v1.29.0 in the generated code comment.
pkg/api/message/service.go (1)
435-439: FetchKeyPackages method implementation looks good.The method appropriately handles the request to fetch the latest key package envelopes for specified installation keys.
tools/go.mod (1)
85-85: Dependencies updated to latest versions.Multiple indirect dependencies have been updated, including:
- sqlc from v1.28.0 to v1.29.0 (aligns with the generated code comment changes)
- gRPC, protobuf, and other core dependencies
- Various cloud provider SDKs and utility libraries
These updates keep the codebase current with the latest dependency versions.
Also applies to: 125-126, 143-143, 157-157, 162-162, 194-194, 249-250, 255-255, 285-285, 288-288, 293-293, 299-300, 316-316, 319-323, 330-331, 334-334, 339-339, 344-346, 358-358, 360-361, 363-365
pkg/api/message/keyPackages_test.go (2)
19-46: Good test helper for creating key package test data.The
writeKeyPackagehelper function effectively sets up test data by creating and inserting key packages with the given installation key.
48-59: Well-structured envelope parsing helper.The
parseEnvelopeshelper function makes test assertions cleaner by converting protobuf envelopes to internal envelope objects.
| const selectNewestFromTopics = `-- name: SelectNewestFromTopics :many | ||
| SELECT e.gateway_time, e.originator_node_id, e.originator_sequence_id, e.topic, e.originator_envelope, e.payer_id | ||
| FROM gateway_envelopes e | ||
| INNER JOIN ( | ||
| SELECT topic, | ||
| MAX(gateway_time) AS max_time | ||
| FROM gateway_envelopes | ||
| WHERE topic = ANY($1::BYTEA []) | ||
| GROUP BY topic | ||
| ) t ON e.topic = t.topic | ||
| AND e.gateway_time = t.max_time | ||
| ORDER BY e.topic | ||
| ` | ||
|
|
||
| func (q *Queries) SelectNewestFromTopics(ctx context.Context, topics [][]byte) ([]GatewayEnvelope, error) { | ||
| rows, err := q.db.QueryContext(ctx, selectNewestFromTopics, pq.Array(topics)) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer rows.Close() | ||
| var items []GatewayEnvelope | ||
| for rows.Next() { | ||
| var i GatewayEnvelope | ||
| if err := rows.Scan( | ||
| &i.GatewayTime, | ||
| &i.OriginatorNodeID, | ||
| &i.OriginatorSequenceID, | ||
| &i.Topic, | ||
| &i.OriginatorEnvelope, | ||
| &i.PayerID, | ||
| ); err != nil { | ||
| return nil, err | ||
| } | ||
| items = append(items, i) | ||
| } | ||
| if err := rows.Close(); err != nil { | ||
| return nil, err | ||
| } | ||
| if err := rows.Err(); err != nil { | ||
| return nil, err | ||
| } | ||
| return items, nil | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
New function to retrieve newest envelopes by topic
This new query and method implementation correctly retrieves the newest gateway envelopes per topic by:
- Finding the maximum gateway_time for each requested topic
- Joining this back to the gateway_envelopes table to get the complete envelope records
- Ordering results by topic for consistent return order
The implementation follows the same pattern as other similar functions in this file, with proper error handling and resource cleanup.
Let's verify that no edge cases are missed:
🏁 Script executed:
#!/bin/bash
# Check if there are any existing test cases that verify behavior when topics don't exist
rg -A 5 -B 5 "SelectNewestFromTopics.*empty|no matching|not found" --type goLength of output: 4185
Add tests for empty and non-matching topics in SelectNewestFromTopics
The new query implementation correctly retrieves the latest envelopes per topic, but there are currently no tests covering these edge cases. Please add test cases to verify that:
- Passing an empty slice of topics returns an empty result (no error).
- Passing topics that don’t exist in
gateway_envelopesreturns an empty result (no error).
Add these to your test suite (e.g. in pkg/db/queries/envelopes_test.go) to ensure consistent behavior.
There was a problem hiding this comment.
This is generated code
There was a problem hiding this comment.
@neekolas You're absolutely right. I missed the "Code generated by sqlc. DO NOT EDIT" header. Since this is auto-generated code, we shouldn't modify it directly.
Looking at the PR objectives, I see you've already included test coverage in newestEnvelope_test.go. If you need to test the edge cases, those should be handled in your service-level tests rather than in the generated files.
Thanks for the correction!
neekolas
force-pushed
the
04-24-add_new_fetchkeypackages_api
branch
from
6704f6b to
154a119
Compare
neekolas
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
dev/gen/protosis excluded by!**/gen/**pkg/proto/openapi/xmtpv4/message_api/message_api.swagger.jsonis excluded by!pkg/proto/**pkg/proto/xmtpv4/message_api/message_api.pb.gois excluded by!**/*.pb.go,!pkg/proto/**pkg/proto/xmtpv4/message_api/message_api.pb.gw.gois excluded by!**/*.pb.gw.go,!pkg/proto/**pkg/proto/xmtpv4/message_api/message_api_grpc.pb.gois excluded by!**/*.pb.go,!pkg/proto/**tools/go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
pkg/api/message/keyPackages_test.go(1 hunks)pkg/api/message/service.go(1 hunks)pkg/db/queries/congestion.sql.go(1 hunks)pkg/db/queries/db.go(1 hunks)pkg/db/queries/envelopes.sql.go(8 hunks)pkg/db/queries/identityUpdates.sql.go(1 hunks)pkg/db/queries/indexer.sql.go(1 hunks)pkg/db/queries/models.go(1 hunks)pkg/db/queries/node.sql.go(1 hunks)pkg/db/queries/nonceManager.sql.go(1 hunks)pkg/db/queries/payerReports.sql.go(1 hunks)pkg/db/sqlc/envelopes.sql(1 hunks)tools/go.mod(9 hunks)
✅ Files skipped from review due to trivial changes (7)
- pkg/db/queries/node.sql.go
- pkg/db/queries/payerReports.sql.go
- pkg/db/queries/db.go
- pkg/db/queries/indexer.sql.go
- pkg/db/queries/nonceManager.sql.go
- pkg/db/queries/models.go
- pkg/db/queries/identityUpdates.sql.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/db/queries/congestion.sql.go
- pkg/db/queries/envelopes.sql.go
- pkg/api/message/service.go
- pkg/db/sqlc/envelopes.sql
- tools/go.mod
🧰 Additional context used
🪛 GitHub Check: Lint-Go
pkg/api/message/keyPackages_test.go
[failure] 147-147:
File is not properly formatted (gofumpt)
🪛 GitHub Actions: Lint
pkg/api/message/keyPackages_test.go
[error] 147-147: File is not properly formatted (gofumpt)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd-cli)
- GitHub Check: Test (Node)
- GitHub Check: Upgrade Tests
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd)
- GitHub Check: Code Review
🔇 Additional comments (6)
pkg/api/message/keyPackages_test.go (6)
1-17: LGTM! Well-structured imports.The package is correctly defined as
message_testfor black-box testing, and imports are organized appropriately.
19-46: LGTM! Well-implemented helper function.The
writeKeyPackagehelper encapsulates all the necessary steps to create and insert a test key package envelope. Good use of the testutils package for random generation.
48-62: LGTM! Robust envelope parsing.The
parseEnvelopeshelper correctly handles nil envelopes and converts the protobuf format to internal envelope objects.
64-85: LGTM! Clear test setup.Good test setup with multiple installation IDs and creation of multiple key packages for installation ID 1 to verify that only the newest is returned.
86-123: LGTM! Comprehensive test cases.Well-designed test cases covering multiple scenarios:
- Fetching multiple installation IDs
- Single ID fetching
- Order independence
- Handling missing envelopes
The expected values are clearly defined for each test case.
125-159: LGTM! Thorough verification logic.The test execution and verification are comprehensive, checking both the topics and sequence IDs of the returned envelopes. Good handling of nil values in the verification stage.
🧰 Tools
🪛 GitHub Check: Lint-Go
[failure] 147-147:
File is not properly formatted (gofumpt)🪛 GitHub Actions: Lint
[error] 147-147: File is not properly formatted (gofumpt)
neekolas
force-pushed
the
04-24-add_new_fetchkeypackages_api
branch
from
154a119 to
4ae0630
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/api/message/newestEnvelope_test.go (1)
97-101: Consider adding test case with empty topics arrayWhile the current test cases are comprehensive, it might be worth adding a test case for an empty topics array to ensure the API handles this edge case gracefully.
{ name: "all three installation IDs", requestedTopics: []topic.Topic{ topic1, topic2, topic3, }, expectedNumReturned: 3, expectedTopics: []*topic.Topic{&topic1, &topic2, &topic3}, expectedSequenceIDs: []uint64{seq1, seq2, seq3}, }, + { + name: "empty topics array", + requestedTopics: []topic.Topic{}, + expectedNumReturned: 0, + expectedTopics: []*topic.Topic{}, + expectedSequenceIDs: []uint64{}, + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
dev/gen/protosis excluded by!**/gen/**pkg/proto/openapi/xmtpv4/message_api/message_api.swagger.jsonis excluded by!pkg/proto/**pkg/proto/xmtpv4/message_api/message_api.pb.gois excluded by!**/*.pb.go,!pkg/proto/**pkg/proto/xmtpv4/message_api/message_api.pb.gw.gois excluded by!**/*.pb.gw.go,!pkg/proto/**pkg/proto/xmtpv4/message_api/message_api_grpc.pb.gois excluded by!**/*.pb.go,!pkg/proto/**tools/go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
pkg/api/message/newestEnvelope_test.go(1 hunks)pkg/api/message/service.go(1 hunks)pkg/db/queries/congestion.sql.go(1 hunks)pkg/db/queries/db.go(1 hunks)pkg/db/queries/envelopes.sql.go(8 hunks)pkg/db/queries/identityUpdates.sql.go(1 hunks)pkg/db/queries/indexer.sql.go(1 hunks)pkg/db/queries/models.go(1 hunks)pkg/db/queries/node.sql.go(1 hunks)pkg/db/queries/nonceManager.sql.go(1 hunks)pkg/db/queries/payerReports.sql.go(1 hunks)pkg/db/sqlc/envelopes.sql(1 hunks)tools/go.mod(9 hunks)
✅ Files skipped from review due to trivial changes (6)
- pkg/db/queries/db.go
- pkg/db/queries/identityUpdates.sql.go
- pkg/db/queries/models.go
- pkg/db/queries/node.sql.go
- pkg/db/queries/payerReports.sql.go
- pkg/db/queries/indexer.sql.go
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/db/queries/nonceManager.sql.go
- pkg/db/queries/congestion.sql.go
- pkg/api/message/service.go
- pkg/db/queries/envelopes.sql.go
- tools/go.mod
- pkg/db/sqlc/envelopes.sql
🧰 Additional context used
🪛 GitHub Check: Lint-Go
pkg/api/message/newestEnvelope_test.go
[failure] 147-147:
File is not properly formatted (gofumpt)
🪛 GitHub Actions: Lint
pkg/api/message/newestEnvelope_test.go
[error] 147-147: File is not properly formatted (gofumpt)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd-cli)
- GitHub Check: Upgrade Tests
- GitHub Check: Test (Node)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd)
🔇 Additional comments (2)
pkg/api/message/newestEnvelope_test.go (2)
1-159: Excellent test coverage for the new API endpointThe test file provides comprehensive coverage for the
GetNewestEnvelopeAPI endpoint with well-structured helper functions and thorough test cases. The tests cover multiple scenarios including:
- Multiple installation IDs
- Single installation ID queries
- Out-of-order topic requests
- Topics with no existing envelopes
The helper functions
writeKeyPackageandparseEnvelopesare well-designed to support the tests and make the code more readable.🧰 Tools
🪛 GitHub Check: Lint-Go
[failure] 147-147:
File is not properly formatted (gofumpt)🪛 GitHub Actions: Lint
[error] 147-147: File is not properly formatted (gofumpt)
19-46:Details
✅ Verification successful
Verify database isolation between test runs
The
writeKeyPackagefunction uses a fixed nodeID (100) for all entries. While this works for the current tests, it could potentially cause issues if tests are run in parallel or if the database isn't properly cleaned between test runs.
🏁 Script executed:
#!/bin/bash # Check if there are test isolation mechanisms in place grep -r "t.Parallel()" pkg/api/message/*_test.go grep -r "cleanup" pkg/testutils/api/api.goLength of output: 172
I’ll inspect the test setup to see how the database is provisioned and whether each test gets its own isolated instance or cleanup logic.
🏁 Script executed:
#!/bin/bash # Show the top of the message tests to find any setup/teardown or DB initialization sed -n '1,200p' pkg/api/message/newestEnvelope_test.go # Show the test utils API helper to spot any DB cleanup or per-test DB creation sed -n '1,200p' pkg/testutils/api/api.goLength of output: 10207
Let’s locate the definition of
NewTestReplicationAPIClientand inspect howtestutils.NewDBsets up and tears down the database:
🏁 Script executed:
#!/bin/bash # Find the test API client initializer grep -R "NewTestReplicationAPIClient" -n pkg/testutils/api/api.go # Inspect how the test DB is created and cleaned up grep -R "func NewDB" -n pkg/testutilsLength of output: 349
Database isolation confirmed; fixed NodeID is safe
The
NewTestReplicationAPIClienthelper callstestutils.NewDB, which creates a fresh database instance for each top-level test and registers its cleanup viat.Cleanup. Additionally, no tests inpkg/api/messageuset.Parallel(). As a result, each invocation ofTestGetNewestEnveloperuns against its own isolated database, and the hard-codednodeID = 100cannot conflict across tests.
neekolas
force-pushed
the
04-24-add_new_fetchkeypackages_api
branch
5 times, most recently
from
a6f8d2c to
5d6c603
Compare
Add GetNewestEnvelope API endpoint to retrieve the most recent envelope for requested topics
📍Where to StartStart with the Macroscope summarized c06c2ea. |
neekolas
force-pushed
the
04-24-add_new_fetchkeypackages_api
branch
from
5d6c603 to
a75465d
Compare
neekolas
force-pushed
the
04-24-add_new_fetchkeypackages_api
branch
from
a75465d to
c06c2ea
Compare
Why do this?
originator_idandsequence_idof the resultImplement GetNewestEnvelope API endpoint to retrieve newest key packages for specified installation IDs
GetNewestEnvelopeAPI endpoint in service.go that retrieves the newest key packages for given installation IDsSelectNewestFromTopicsin envelopes.sql to fetch latest envelopes by gateway time/mls/v2/get-newest-envelope📍Where to Start
Start with the
FetchKeyPackagesmethod implementation in service.go, which contains the main business logic for retrieving key packages.Macroscope summarized 6704f6b.
Summary by CodeRabbit