Conversation
✅ Docs preview readyThe preview is ready to be viewed. View the preview File Changes 0 new, 4 changed, 0 removedBuild ID: e88179e03589c1cfa39a88e4 URL: https://www.apollographql.com/docs/deploy-preview/e88179e03589c1cfa39a88e4
|
d5c2d60 to
9e28f7d
Compare
f77101a to
ed38fd0
Compare
ed38fd0 to
f350b71
Compare
mabuyo
left a comment
There was a problem hiding this comment.
Minor comments taking Librarian suggestions as well as keeping similar paragraphs consistent between the pages.
Pre-approving since these changes are non-blocking on docs side!
| pub(crate) const MULTIPART_SUBSCRIPTION_SPEC_VALUE: &str = "1.0"; | ||
|
|
||
| pub(crate) const DEFAULT_SOCKET_PATH: &str = "/"; | ||
| pub(crate) const PATH_PREFIX: &str = "path="; |
There was a problem hiding this comment.
nit: should this be named PATH_QUERY_PARAM ?
| pub(crate) const MULTIPART_SUBSCRIPTION_SPEC_PARAMETER: &str = "subscriptionSpec"; | ||
| pub(crate) const MULTIPART_SUBSCRIPTION_SPEC_VALUE: &str = "1.0"; | ||
|
|
||
| pub(crate) const DEFAULT_SOCKET_PATH: &str = "/"; |
There was a problem hiding this comment.
Should this one be #[cfg(unix)] otherwise its dead code on non-unix platforms?
There was a problem hiding this comment.
oh yeah, good eye
| let converted_uri: http::Uri = if let Some(path) = uri.strip_prefix("unix://") { | ||
| let (socket_path, http_path) = parse_unix_socket_url(path); | ||
| hyperlocal::Uri::new(&socket_path, &http_path).into() | ||
| } else { | ||
| uri.parse()? | ||
| }; |
There was a problem hiding this comment.
Would it make sense/be better to do the parse_unix_socket_url call once at configuration time and not on the every request hot-path (similar to whats done in plugins/override_url.rs new) ?
There was a problem hiding this comment.
maybe but only if it proves to be a problem; I suspect it won't be, but changing the APIs of both the router and supergraph builders will introduce a lot of mess
happy to have @carodewig say otherwise though; details: we'd need to add uri to the router and supergraph builders in the external service, and we'd also need to string through the right uri from the relevant callsites (eg, for router, we'd need to inject it from both the router request and response stages for coprocessors)
There was a problem hiding this comment.
I agree with your assessment and maybe its something to consider doing later down the road if deemed necessary or more natural with the rest of the codebase. Im just in the habit of trying by default to shed anything possible from the hot request codepath....
|
|
||
| #[cfg(unix)] | ||
| #[cfg(test)] | ||
| mod unix_socket_url_tests { |
There was a problem hiding this comment.
2 additional tests should be added here:
- parse_socket_path_with_empty_path_param:
?path - parse_socket_path_without_leading_slash:
?path=no_leading_slash
There was a problem hiding this comment.
done! and rstest-ified
| @@ -0,0 +1,6 @@ | |||
| ### Enable Unix Domain Socket paths ([PR #8894](https://github.com/apollographql/router/pull/8894)) | |||
|
|
|||
| Enables Unix Domain Socket (UDS) paths for both coprocessors and subgraphs. The paths _must_ use `?path=` as their delimiter: `unix:///tmp/some.sock?path=some_path` | |||
There was a problem hiding this comment.
I don't think ?path= is really the delimiter - isn't it ?? Based on url_path.find('?')
There was a problem hiding this comment.
ahh, it is, yeah; I was trying to stress that folks must use ?path= rather than something like ?whatever= because we match on path
I'll think through better wording
Co-authored-by: Michelle Mabuyo <michelle@apollographql.com>
Co-authored-by: Michelle Mabuyo <michelle@apollographql.com>
Co-authored-by: Michelle Mabuyo <michelle@apollographql.com>
| /// Requires: | ||
| /// - when using query params, the param must be denoted by `?path=` | ||
| #[cfg(unix)] | ||
| pub(crate) fn parse_unix_socket_url(url_path: &str) -> (String, String) { |
There was a problem hiding this comment.
Nitpick, but is there any reason why this can't return (&str, &str)?
c767d9f to
ad82dc6
Compare
ad82dc6 to
98733ca
Compare
|
I dont have permission to mark my feedback items as resolved, but please consider them resolved/freel free to close them out. |
| )] | ||
| fn parse_socket_url( | ||
| #[case] input: &str, | ||
| #[case] expected_socket: &str, |
There was a problem hiding this comment.
Nit: it looks like all the tests have the same expected_socket, so this param could be removed
|
Checking in here. We've upgraded to 2.12.0, and are anxiously awaiting this capability ;) |
|
I should be able to return to it next week (had an urgent interrupt that's
nearly over); sorry it's taken so long! I still owe you another PR too,
though I need to check my notes on what it was for (mostly trying to
broadcast that this hasn't been forgotten, just a necessary backburnering
for a short while)
…On Fri, Mar 6, 2026 at 11:59 AM Jon Christiansen ***@***.***> wrote:
*theJC* left a comment (apollographql/router#8894)
<#8894 (comment)>
Checking in here. We've upgraded to 2.12.0, and are anxiously awaiting
this capability ;)
—
Reply to this email directly, view it on GitHub
<#8894 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGMABHEHX7IZTVI7PD4MIVD4PL7XNAVCNFSM6AAAAACVOWYLMKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DAMJSHA4TKMRWGM>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Co-authored-by: Michelle Mabuyo <michelle@apollographql.com>
Co-authored-by: Michelle Mabuyo <michelle@apollographql.com>
Checklist
Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.
Exceptions
Note any exceptions here
Notes
Footnotes
It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. ↩
Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩
A lot of (if not most) features benefit from built-in observability and
debug-level logs. Please read this guidance on metrics best-practices. ↩Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩