YNU-259: add app protocol enforcement#374
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaces string protocol fields with a typed rpc.Version across RPC APIs and SDK, adds VersionNitroRPCv0_2 and an IsSupportedVersion check, enforces protocol validation in CreateApplication, updates router serialization, tests, SDK schemas, and documentation to use NitroRPC/0.2. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Router as RPC Router
participant Service as AppSessionService
participant RPC as rpc (version registry)
rect rgb(245,250,255)
note right of Client: Create Application Session
Client->>Router: POST /create_app_session (AppDefinition{protocol: Version})
Router->>Service: CreateApplication(definition)
Service->>RPC: IsSupportedVersion(definition.Protocol)
alt supported
Service-->>Router: create AppSession
Router-->>Client: 200 OK (AppSession{protocol: Version.String()})
else unsupported
Service-->>Router: RPC error (unsupported protocol)
Router-->>Client: 4xx error
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
Summary of ChangesHello @dimast-x, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the robustness and security of application session management by introducing a dedicated Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces protocol enforcement for application sessions, ensuring only supported protocols like 'NitroRPC/0.2' are used. The changes involve adding a new Protocol type, validating it upon application creation, and updating various parts of the codebase and tests to use this new type. The implementation is clean and effective. I've added a couple of suggestions to improve maintainability and test coverage.
philanton
force-pushed
the
feat/app-protocol-enforcement
branch
from
de2976c to
d4aec0f
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)
clearnode/docs/Entities.md (1)
44-44: Consider updating the type description.The description states
Protocolis a string, but the code now usesrpc.Versiontype. Whilerpc.Versionis likely a string alias, consider updating the documentation to reflect the actual type for consistency.Apply this diff to update the type description:
-- `Protocol` (string): Protocol version used (must be "NitroRPC/0.2") +- `Protocol` (rpc.Version): Protocol version used (must be "NitroRPC/0.2")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
clearnode/app_session.go(1 hunks)clearnode/app_session_service.go(2 hunks)clearnode/app_session_service_test.go(10 hunks)clearnode/docs/API.md(3 hunks)clearnode/docs/Clearnode.protocol.md(1 hunks)clearnode/docs/Entities.md(2 hunks)clearnode/pkg/rpc/README.md(1 hunks)clearnode/pkg/rpc/api.go(3 hunks)clearnode/rpc_router_private_test.go(3 hunks)clearnode/rpc_router_public.go(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- clearnode/docs/API.md
🚧 Files skipped from review as they are similar to previous changes (3)
- clearnode/rpc_router_private_test.go
- clearnode/rpc_router_public.go
- clearnode/app_session_service.go
🧰 Additional context used
🧬 Code graph analysis (2)
clearnode/app_session.go (1)
clearnode/pkg/rpc/api.go (1)
Version(27-27)
clearnode/app_session_service_test.go (1)
clearnode/pkg/rpc/api.go (1)
VersionNitroRPCv0_2(31-31)
⏰ 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 and Publish (Clearnode)
- GitHub Check: Test (Integration) / Test Integration
- GitHub Check: Analyze (go)
🔇 Additional comments (10)
clearnode/app_session.go (2)
6-6: LGTM!The import of the
rpcpackage is appropriate for using therpc.Versiontype.
13-13: No GORM compatibility issues with rpc.Version default
rpc.Version is a simpletype Version stringalias with no customScan/Valuemethods, so thegorm:"default:'NitroRPC/0.2'"tag will function as intended.clearnode/pkg/rpc/README.md (1)
185-185: LGTM!The example has been correctly updated to use the typed constant
rpc.VersionNitroRPCv0_2instead of a string literal, aligning with the new protocol enforcement.clearnode/docs/Entities.md (1)
55-56: LGTM!The added note clearly communicates the protocol enforcement requirement to users of the API.
clearnode/docs/Clearnode.protocol.md (1)
22-22: LGTM!The documentation correctly describes the new protocol version validation requirement in the Virtual Application Creation flow.
clearnode/app_session_service_test.go (2)
7-7: LGTM!The import of the
rpcpackage is necessary for using therpc.VersionNitroRPCv0_2constant throughout the tests.
51-51: LGTM! Tests correctly updated to use the enforced protocol version.All test cases have been updated to use
rpc.VersionNitroRPCv0_2instead of the previous "test-proto" string literal. This ensures tests use the valid, enforced protocol version and will properly exercise the new validation logic.Also applies to: 124-124, 155-155, 182-182, 215-215, 265-265, 321-321, 409-409, 463-463
clearnode/pkg/rpc/api.go (3)
21-49: Well-structured protocol versioning implementation.The typed
Versionapproach with a validation registry is clean and extensible. The implementation correctly uses a map for O(1) lookup.
519-521: LGTM! Type-safe protocol enforcement.Changing
ProtocolfromstringtoVersionenforces type safety while maintaining JSON compatibility. The validation logic inapp_session_service.go(per AI summary) ensures only supported versions are accepted.
544-545: Consistent type change for AppSession.The
Protocolfield change mirrorsAppDefinitionand maintains consistency. JSON serialization is handled correctly viaString()conversion in the router (per AI summary).
3 participants
Todos:
Summary by CodeRabbit
New Features
Documentation
Tests