Conversation
✅ Docs preview readyThe preview is ready to be viewed. View the preview File Changes 0 new, 1 changed, 0 removedBuild ID: 2b77fd6116071d4543676779 URL: https://www.apollographql.com/docs/deploy-preview/2b77fd6116071d4543676779 |
|
⏭️ Changeset check skipped via label |
971cc3c to
e47a117
Compare
Review SummaryClean refactoring that fully removes the deprecated SSE transport variant and associated infrastructure. The changes are thorough and include proper test coverage to prevent SSE from being used. Findings[Consider] The boxing of auth::Config in Transport::StreamableHttp (changing from Optionauth::Config to Option<Boxauth::Config>) appears to be an optimization to reduce the enum's memory footprint. While this is generally good practice per Chapter 2.3 (clippy's large_enum_variant lint), it's worth confirming this was intentional and not an accidental change, as it wasn't mentioned in the PR description. Test Coverage AssessmentGood test coverage:
Following Chapter 5 best practices, the test names are descriptive and focused on single behaviors. Final RecommendationApprove with suggestions - The code is solid and ready to merge. The auth boxing change is worth confirming as intentional. Reviewed by Claude Code Sonnet 4.5 |
| /// Authentication configuration | ||
| #[serde(default)] | ||
| auth: Option<auth::Config>, | ||
| auth: Option<Box<auth::Config>>, |
There was a problem hiding this comment.
Just want to double check the intent behind this Box. :)
There was a problem hiding this comment.
Without it, you will run into this linting issue:
error: large size difference between variants
--> crates/apollo-mcp-server/src/server.rs:59:1
|
59 | / pub enum Transport {
60 | | /// Use standard IO for server <> client communication
61 | | #[default]
62 | | Stdio,
| | ----- the second-largest variant carries no data at all
... |
65 | |/ StreamableHttp {
66 | || /// Authentication configuration
67 | || #[serde(default)]
68 | || auth: Option<auth::Config>,
... ||
84 | || host_validation: HostValidationConfig,
85 | || },
| ||_____- the largest variant contains at least 356 bytes
86 | | }
| |__^ the entire enum is at least 360 bytes
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.92.0/index.html#large_enum_variant
= note: `-D clippy::large-enum-variant` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::large_enum_variant)]`
help: consider boxing the large fields or introducing indirection in some other way to reduce the total size of the enum
|
68 - auth: Option<auth::Config>,
68 + auth: Box<Option<auth::Config>>,
|
error: could not compile `apollo-mcp-server` (lib) due to 1 previous error
warning: build failed, waiting for other jobs to finish...
error: could not compile `apollo-mcp-server` (lib test) due to 1 previous errore47a117 to
d3a0bd9
Compare
Review SummaryThis is a clean refactoring that successfully removes all SSE transport remnants from the codebase. The approach of rejecting SSE at parse time (via serde deserialization) rather than at runtime is a better design. FindingsNo new issues found. The previous review correctly identified the auth boxing change, which follows clippy's large_enum_variant lint guidance (Chapter 2.3). Test Coverage AssessmentExcellent test coverage:
Final RecommendationApprove - The refactoring is well-executed and improves code clarity by removing dead code paths. Reviewed by Claude Code Sonnet 4.5 |
mabuyo
left a comment
There was a problem hiding this comment.
Minor coment on docs addition
docs/source/config-file.mdx
Outdated
|
|
||
| <Note> | ||
|
|
||
| SSE transport is no longer supported by Apollo MCP Server. Use `streamable_http` for HTTP-based connections. |
There was a problem hiding this comment.
Might be helpful to add a version number here if there's a large chance folks are still using older versions! "no longer supported by Apollo MCP Server as of vx.x.x"
Review SummaryThis refactoring successfully removes all SSE transport code following its deprecation in v1.5.0. The latest commit adds appropriate version context to the deprecation notice in documentation. FindingsNo new issues found beyond what was previously identified in earlier reviews regarding the auth Config boxing optimization. Test Coverage AssessmentTest coverage is excellent - the new tests ensure SSE is rejected at parse time, and the removal of the obsolete start_sse_server_returns_unsupported_error test is appropriate since SSE now fails during deserialization rather than at runtime. Final RecommendationApprove - The refactoring is well-executed, removes dead code paths cleanly, and improves maintainability. The auth boxing change follows Rust best practices per Chapter 2.3 (large_enum_variant lint). Reviewed by Claude Code Sonnet 4.5 |
The SSE transport support was removed in v1.5.0, but the enum variant, error path, match arms, and dedicated test were still hanging around and complicating the transport logic. This PR removes
Transport::SSEentirely and cleans up the related code.