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.

Sign up for GitHub

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

Jump to bottom

Implement connection handling for graphql-transport-ws sub protocol. #126

Closed
wants to merge 4 commits into from
Closed

Implement connection handling for graphql-transport-ws sub protocol. #126

wants to merge 4 commits into from

Conversation

atsfour
Copy link
Contributor

@atsfour atsfour commented Apr 23, 2022

Fixes #123

It works. I have checked with demo schema on GraphiQL with graphql-ws client.

But some TODOs remain.

In particular, it is better to deal with the handling of the status code later. I'm not good at pedestal.jetty and didn't know how to solve these problems well.

I would also like to add some tests, but I would appreciate any advice on the testing.

@atsfour
Copy link
Contributor Author

atsfour commented May 5, 2022

Added some tests. I copied test/com/walmartlabs/lacinia/pedestal/subscriptions_test.clj and fix cases to fit graphql-transport-ws sub protocol.

Also fixed status codes, using Session instance directly.

Any request to fix, I'd like to do it.

@hlship
Copy link
Member

hlship commented May 24, 2022

I'd like to look at this, but don't know if you've signed the CLA?

@atsfour
Copy link
Contributor Author

atsfour commented Jun 5, 2022

Thanks for considering.
And, Yes, I've signed the CLA when past PR was accepted.

#118

hlship
hlship reviewed Aug 12, 2022
View reviewed changes
dev-resources/demo-schema.edn
@@ -2,7 +2,7 @@
{:tick
{:description "A subscription response."
:fields {:time_ms {:type String
:description "Time when tick is emittied (as string-ified long milliseconds since epoch)."
:description "Time when tick is emitted (as string-field long milliseconds since epoch)."
Copy link
Member

Choose a reason for hiding this comment

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

Actually, one was a typo, but I meant "string-ified" as the difference between 42 and "42". The latter is string-ified, i.e., a numeric string.

src/com/walmartlabs/lacinia/pedestal/internal.clj
@@ -249,12 +249,13 @@
"}"))

(defn graphiql-response
[api-path subscriptions-path asset-path ide-headers ide-connection-params]
[api-path subscriptions-path asset-path ide-headers ide-connection-params ide-use-legacy-ws-client]
Copy link
Member

Choose a reason for hiding this comment

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

This is about the point where it's time to switch from many parameters to an options map.

src/com/walmartlabs/lacinia/pedestal/subscriptions.clj
specified by [graphql-ws](https://github.com/enisdenjo/graphql-ws/blob/v5.7.0/PROTOCOL.md)"
(reify Ws-Sub-Protocol
(sub-protocol-name [_] "graphql-transport-ws")
(connection-loop [self session _ ws-data-ch response-data-ch context]
Copy link
Member

Choose a reason for hiding this comment

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

Really not using self here. Also, from bitter experience, I tend to have methods in reified objects delegate to a function; otherwise you hit some oddities about debugging and reloading.

dev-resources/demo-schema.edn
@@ -2,7 +2,7 @@
{:tick
{:description "A subscription response."
:fields {:time_ms {:type String
:description "Time when tick is emittied (as string-ified long milliseconds since epoch)."
:description "Time when tick is emitted (as string-field long milliseconds since epoch)."
Copy link
Member

Choose a reason for hiding this comment

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

ified wasn't really a typo, I was trying to express that time was a numeric string, not a number.

@hlship
Copy link
Member

hlship commented Aug 12, 2022

I'm seeing test failures when I build locally.

Testing com.walmartlabs.lacinia.pedestal.subscriptions-graphql-transport-ws-test

FAIL in (client-parallel) (subscriptions_graphql_transport_ws_test.clj:221)
expected: (clojure.core/= {:id right-id, :payload {:data {:ping {:message "right #1"}}}, :type "next"} (com.walmartlabs.lacinia.test-utils/<message!!))
  actual: (not (clojure.core/= {:id 5, :payload {:data {:ping {:message "right #1"}}}, :type "next"} {:type "next", :id 4, :payload {:data {:ping {:message "left #2"}}}}))

FAIL in (client-parallel) (subscriptions_graphql_transport_ws_test.clj:225)
expected: (= 2 (- (clojure.core/deref *ping-subscribes) init-subs))
  actual: (not (= 2 1))

FAIL in (client-parallel) (subscriptions_graphql_transport_ws_test.clj:228)
expected: (clojure.core/= {:id left-id, :payload {:data {:ping {:message "left #2"}}}, :type "next"} (com.walmartlabs.lacinia.test-utils/<message!!))
  actual: (not (clojure.core/= {:id 4, :payload {:data {:ping {:message "left #2"}}}, :type "next"} {:type "next", :id 5, :payload {:data {:ping {:message "right #1"}}}}))

FAIL in (client-parallel) (subscriptions_graphql_transport_ws_test.clj:232)
expected: (clojure.core/= {:id right-id, :payload {:data {:ping {:message "right #2"}}}, :type "next"} (com.walmartlabs.lacinia.test-utils/<message!!))
  actual: (not (clojure.core/= {:id 5, :payload {:data {:ping {:message "right #2"}}}, :type "next"} {:type "complete", :id 4}))

FAIL in (client-parallel) (subscriptions_graphql_transport_ws_test.clj:236)
expected: (clojure.core/= {:id left-id, :type "complete"} (com.walmartlabs.lacinia.test-utils/<message!!))
  actual: (not (clojure.core/= {:id 4, :type "complete"} {:type "next", :id 5, :payload {:data {:ping {:message "right #2"}}}}))

Looks like some kind of off-by-one error on the message ids.

@hlship hlship closed this Aug 12, 2022
@hlship hlship mentioned this pull request Aug 19, 2022
Open
@hlship
Copy link
Member

hlship commented Dec 9, 2022

I'm beginning to ramp up towards a milestone release in January; do you have time to look into the above problems?

@hlship
Copy link
Member

hlship commented Jan 27, 2023

Getting close to a lacinia 1.2 release, it would be nice to pursue this for a lacinia-pedestal release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hlship hlship hlship left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Incompatibility with GraphQL clients for subscriptions
2 participants
@atsfour @hlship