update hive-apollo-router-plugin to be compatible with apollo-router v2 #6535
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis pull request updates dependency versions and refactors request handling in the router package. In the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant P as PersistedDocumentsPlugin
participant S as Checkpoint Service
C->>P: Send HTTP Request
P->>S: Invoke checkpoint_async(request)
S-->>P: Return Response or Error
alt Successful Response
P->>P: Collect body using .collect()
P->>P: Convert bytes using body_from_bytes
P-->>C: Return Processed Response
else Error Occurred
P-->>C: Return FailedToReadBody (axum_core::Error)
end
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![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 (4)
packages/libraries/router/src/persisted_documents.rs (4)
114-118: Be mindful of large request bodiesCollecting the entire body into memory may be expensive if requests can be large. If your usage involves large payloads, consider streaming or buffering in chunks to avoid memory spikes.
207-211: Add a docstring forbody_from_bytes?This helper function is a good utility. Consider adding a short docstring clarifying its purpose, especially since it handles infallible conversions.
281-292: Security caution withdanger_accept_invalid_certsEnabling invalid SSL acceptance can be useful for testing or special environments but may be risky in production. Ensure it’s disabled in live deployments to avoid MITM vulnerabilities.
370-370: Avoid logging entire response body on failureLogging the raw body can leak sensitive data or cause memory overhead. Consider partial logging or omitting the body altogether if it can contain private information.
Also applies to: 372-375
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
configs/cargo/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
packages/libraries/router/Cargo.toml(2 hunks)packages/libraries/router/src/persisted_documents.rs(19 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`packages/libraries/**`: Most of the dirs here are NPM packa...
packages/libraries/**: Most of the dirs here are NPM packages that provides our customers an integration library, in order to get/fetch data from Hive Console API or Hive CDN.
packages/libraries/router/src/persisted_documents.rspackages/libraries/router/Cargo.toml
🔇 Additional comments (19)
packages/libraries/router/src/persisted_documents.rs (13)
12-15: LGTM on new importsThese added imports from
http_body_util,reqwest_middleware, and environment usage appear consistent with the updated approach for body handling and retry logic. No concerns found.Also applies to: 17-18, 21-21
110-110: Confirm the behavior change to.checkpoint_asyncSwitching from the older
.oneshot_...approach to.checkpoint_asyncaligns with Apollo Router v2 changes. Ensure downstream services are tested to confirm no breakage in request flow.
132-135: Neat usage ofbody_from_bytesUsing
body_from_bytesto rewrap the modified request payload is a clean approach and promotes code reuse. Great job.Also applies to: 181-181
229-229: Error type change acknowledgedSwitching to
axum_core::Erroris consistent with other refactors in this file. Verify that all error handling paths are updated accordingly in the rest of the codebase.
257-258: Extension code for CDN responsesDefining
"FAILED_TO_READ_CDN_RESPONSE"is clear and consistent with other error codes. No issues found.
334-338: Consider robust null checksThe code calls
.unwrap()onself.config.endpoint. Even though you set defaults elsewhere, ensure at runtime this never becomesNone. Otherwise, the service might panic.
353-356: Check CDN response sizeReading the entire CDN response at once might affect memory usage if documents become large. Confirm that the maximum persisted document size is reasonable.
416-416: Import in tests looks goodThe introduction of
hyper::body::Bytesfor test usage is consistent. No issues found.
499-499: Test approach to read full bodyCollecting the entire body is acceptable here for unit tests. No issues found.
505-505: Documentation improvement in testsThe added inline comments clarify the test purpose well. Looks good.
Also applies to: 507-507
742-743: No issues with test configSetting the
keyand endpoint in test configs matches the typical mock usage. Approved.
750-751: Variable passing in persisted requestIncluding
variablesis a good thoroughness check. This correctly tests passing additional request parameters.
768-772: Plugin instantiation in tests is consistentThe test config properly sets up the plugin with mocking. No concerns found.
packages/libraries/router/Cargo.toml (6)
20-21: Major bump toapollo-router& newaxum-coredependencyUpgrading to
^2.0.0for Apollo Router and addingaxum-coreis aligned with the new plugin architecture. Ensure you’ve followed the v2 migration guidelines.
23-27:reqwestfeatures reconfiguredEnabling
"blocking"might raise questions in async contexts, but it could be required if your code does synchronous tasks. Verify no performance regressions or deadlock risk.
33-33:hyperupdated to 1Confirm that any existing code dependent on the
0.14.*hyper APIs has been updated.
40-42:tower,http, andhttp-body-utilupdatesAll appear necessary for Router v2. Good alignment with the new request handling mechanics.
45-46:graphql-toolsfeatures reorganizedReformatting the feature list is fine. No issues.
54-55:jsonschemafeature additionsAllowing
resolve-fileis useful if needed for more advanced usage. Confirm no unexpected runtime overhead.
|
Thanks @yanns ! I'll be the one reviewing this PR. I've approved the workflow, but it might fail anyway because of some security requirements (Docker images can't be pushed to registry from forks...) I'll be reviewing and testing this very soon, thank you for contributing! |
dotansimha
changed the title
Background
The Apollo Federation Router was released in version 2 with some incompatible changes: https://github.com/apollographql/router/releases/tag/v2.0.0
Description
This PR updates the different hive plugins to be compatible with the new version of the router.
It does not change the version. I don't know what will be your policy here.