Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Subscription connection parameters support #360

Merged
merged 5 commits into from
Nov 30, 2024

Conversation

HaraldNordgren
Copy link
Contributor

@HaraldNordgren HaraldNordgren commented Nov 14, 2024

Adds support for subscription (websocket) connection parameters support.

We are having an issue with authorization when using subscriptions in genqlient. Our mobile app team is using Apollo for GraphQL requests, and Apollo seems to sends its auth in a way that is different from how genqlient does it:

https://www.apollographql.com/docs/react/data/subscriptions#5-authenticate-over-websocket-optional

This is causing a problems, because we use genqlient for testing the backend. It seems that the way Apollo is implemented, authentication (and other headers) are not actually sent as HTTP headers, but as connection parameters instead.

I have:

  • Written a clear PR title and description (above)
  • Signed the Khan Academy CLA
  • Added tests covering my changes, if applicable
  • Included a link to the issue fixed, if applicable
  • Included documentation, for new features
  • Added an entry to the changelog

//
// connectionParams is a map of connection parameters to be sent to the server
// during the initial connection handshake.
func NewClientUsingWebSocketWithConnectionParams(endpoint string, wsDialer Dialer, headers http.Header, connParams map[string]any) WebSocketClient {
Copy link
Contributor Author

@HaraldNordgren HaraldNordgren Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there was never a a release that includes NewClientUsingWebSocket, we should be free to update the signature if we want, and possible only have one function:

func NewClientUsingWebSocket(endpoint string, wsDialer Dialer, headers http.Header, connParams map[string]any) WebSocketClient

@HaraldNordgren
Copy link
Contributor Author

HaraldNordgren commented Nov 14, 2024

A big gotcha regarding HTTP headers and connection parameters, that I realized recently, is that websocket requests in 99desgins gqlgen enter a different path, going through here:

	gqlgenServer.AddTransport(transport.Websocket{
		InitFunc: func(ctx context.Context, initPayload transport.InitPayload) (context.Context, *transport.InitPayload, error) {
			// initPayload contains connection parameters
		},
	})

than the middleware path that would normally be expected:

	r.With(ah.VerifyAuthMiddleware).Method(http.MethodPost, "/v1", c.GraphServer)
	r.With(ah.VerifyAuthMiddleware).Method(http.MethodGet, "/v1", c.GraphServer)

@HaraldNordgren HaraldNordgren changed the title Allow setting websocket connection parameters Subcription connection parameters support Nov 16, 2024
@HaraldNordgren HaraldNordgren changed the title Subcription connection parameters support Subscription connection parameters support Nov 16, 2024
@HaraldNordgren
Copy link
Contributor Author

Ping @benjaminjkraft and @matthieu4294967296moineau, do you think it would be possible to get a quick look here? 🤗

@jbaldwinroberts
Copy link

this would be useful for us too - thanks

@HaraldNordgren
Copy link
Contributor Author

Hi @benjaminjkraft, I know you are likely very busy. But it would be much appreciated to get some review here 🤗

Copy link
Collaborator

@benjaminjkraft benjaminjkraft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, it's been a busy month. This seems ok, but I wonder if now is the right time to add a more extensible options interface, either an options struct, or functional-style options, either of which could incorporate the optional headers as well. That way we don't end up having to add a ton of different functions over time.

@HaraldNordgren
Copy link
Contributor Author

HaraldNordgren commented Nov 30, 2024

@benjaminjkraft Hope everything is under control and not too stressful 🤗 Thank you for reviewing here, much appreciated!

I think it's a great point regarding options. I have pushed a new version with this added. Let me know what you think!

@HaraldNordgren
Copy link
Contributor Author

HaraldNordgren commented Nov 30, 2024

I never figured out a way to generate the 99designs/gqlgen code for countAuthorized. Running go generate ./... even on the main branch fails for me.

So I created countAuthorized infrastructure myself. I hope didn't introduce any issues. All the tests are passing at least 😃

Copy link
Collaborator

@benjaminjkraft benjaminjkraft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, go generate ./... should work! It works for me. Adding -v -x may make any issues clearer, you can see what packages and commands it's finding. Anyway, you got it right except for one line of whitespace :-) .

On the actual changes, this looks great. I'm not sure if we should change headers to be an option as well, since it's optional. I think I would incline towards yes. As long as it's a 1:1 replacement we can probably just change the existing API since as you say there hasn't been a release, then we don't have to have two. But if you see a reason not to I'm also ok to merge this as-is.

benjaminjkraft added a commit that referenced this pull request Nov 30, 2024
Originally my goal here was to add a check that generated code is up to
date. (In #360 I was reminded that we have that for first-party code,
but not gqlgen.)

Then I figured I'd update the Go version to support 1.23, which required
updating gqlgen and golangci-lint, both of which now have minimum Go
versions of 1.22. So now that's ours too. Those are the only upstream
supported versions anyway, so hopefully people are updated.

I suspect the CI version-setting stuff can get simplified a bit with all
the Go toolchain version stuff, but for now I didn't bother.
@HaraldNordgren
Copy link
Contributor Author

@benjaminjkraft I think it does make sense with the headers.

But it feels a bit tricky to get it right with the important "Sec-WebSocket-Protocol" key, which should always be present in the output. Maybe better to do as a follow-up?

@benjaminjkraft
Copy link
Collaborator

Oh, I see what you mean. Sure, I'll merge this and if we sort it out before we cut a release great, and if not oh well.

@benjaminjkraft benjaminjkraft merged commit 800909d into Khan:main Nov 30, 2024
4 checks passed
benjaminjkraft added a commit that referenced this pull request Nov 30, 2024
…362)

Originally my goal here was to add a check that generated code is up to
date. (In #360 I was reminded that we have that for first-party code,
but not gqlgen.)

Then I figured I'd update the Go version to support 1.23, which required
updating gqlgen and golangci-lint, both of which now have minimum Go
versions of 1.22. So now that's ours too. Those are the only upstream
supported versions anyway, so hopefully people are updated.

I suspect the CI version-setting stuff can get simplified a bit with all
the Go toolchain version stuff, but for now I didn't bother.

I have:
- [x] Written a clear PR title and description (above)
- [x] Signed the [Khan Academy CLA](https://www.khanacademy.org/r/cla)
- [x] Added tests covering my changes, if applicable
- [x] Included a link to the issue fixed, if applicable
- [x] Included documentation, for new features
- [x] Added an entry to the changelog
@HaraldNordgren
Copy link
Contributor Author

Thanks a lot @benjaminjkraft 🚀

@benjaminjkraft
Copy link
Collaborator

Thank you!!

benjaminjkraft pushed a commit that referenced this pull request Dec 2, 2024
Follow-up up on the discussion in
#360 (review).

Move websocket headers to and opt function 'WithWebsocketHeaders'.

Note this is a breaking change for users using the main branch
(but not for users on tagged releases).

I have:
- [x] Written a clear PR title and description (above)
- [x] Signed the [Khan Academy CLA](https://www.khanacademy.org/r/cla)
- [x] Added tests covering my changes, if applicable
- [x] Included a link to the issue fixed, if applicable
- [x] Included documentation, for new features
- [x] Added an entry to the changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants