walmartlabs / lacinia-pedestal

Expose Lacinia GraphQL as Pedestal endpoints
http://lacinia-pedestal.readthedocs.io/en/latest/
Other
200 stars 63 forks source link

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

Closed atsfour closed 2 years ago

atsfour commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

atsfour commented 2 years ago

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

https://github.com/walmartlabs/lacinia-pedestal/pull/118

hlship commented 2 years ago

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 commented 1 year ago

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

hlship commented 1 year ago

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