Modify federation to be possible with edn schema as well #420

hmy3743 · 2022-08-08T14:30:54Z

Extends lacinia's support for federation to add the ability to support federation through the edn schema as well.

CLAassistant · 2022-08-08T14:31:01Z

All committers have signed the CLA.

hlship

Thanks for the initial effort! I've outlined some improvements that could be made.

hlship · 2022-08-12T16:22:41Z

deps.edn

@@ -1,7 +1,8 @@
 {:deps {org.clojure/clojure {:mvn/version "1.10.3"}
        clj-antlr/clj-antlr {:mvn/version "0.2.12"}
        org.flatland/ordered {:mvn/version "1.15.10"}
-        org.clojure/data.json {:mvn/version "2.4.0"}}
+        org.clojure/data.json {:mvn/version "2.4.0"}
+        org.clojure/core.match {:mvn/version "1.0.0"}}


Looking at how match is used in the code, it feels a bit like over kill - most of the cases could be covered with cond or condp, and adding dependencies is always an issue.

Removed match based on review.

hlship · 2022-08-12T16:23:49Z

dev-resources/edn-federation.sdl

@@ -0,0 +1,34 @@
+type _Service{


I wonder if we could get pretty-printed output. It would make things more complicated, for sure.

It's a bit complicated, but I'll try.

hlship · 2022-08-12T16:28:33Z

src/com/walmartlabs/lacinia/federation.clj

+  [description]
+  (if (nil? description)
+    ""
+    (str "\"\"\"\n" description "\n\"\"\"\n")))


What if description itself includes characters, such as ", that need to be escaped?

The input is edn, so there seems to be no problem. Can you give me an example?

hlship · 2022-08-12T16:29:35Z

src/com/walmartlabs/lacinia/federation.clj

+(defn ^:private value->string
+  [value]
+  (match value
+    (string :guard string?) (str "\"" string "\"")


Same here, may need to escape some characters.

hlship · 2022-08-12T16:30:48Z

src/com/walmartlabs/lacinia/federation.clj

+                   (edn-directives->sdl-directives directives)
+                   (edn-fields->sdl-fields fields))))
+       (join "\n")))
+(defn ^:private edn-queries->sdl-queries


I prefer a blank line between def forms. There's a couple of cases of this.

:queries is such a hold over from early days Lacinia; we really should deprecate it, it causes problems.

That being said, should probably fold :queries in the Query object (likewise mutations and subscriptions) and then pretty print that.

It may be ok to skimp on some error checking, such as name collisions between Query fields an names in the :queries map ... incorrect SDL will be generated BUT that will be caught an instant later at schema compilation and/or calls to prevent-collision.

Edited to reflect the review.

hlship · 2022-08-12T16:34:54Z

src/com/walmartlabs/lacinia/federation.clj

+(defn ^:private edn-scalars->sdl-scalars
+  [scalars]
+  (->> (keys scalars)
+       (map name)


I think there's value to sorting by name in each of these blocks, for repeatability.

Sorting has been added to reflect reviews.

hlship · 2022-08-12T16:38:04Z

src/com/walmartlabs/lacinia/federation.clj

+  "Translate the edn lacinia schema to the SDL schema."
+  [schema]
+  (->> schema
+       (map (fn [[key val]]


Again, repeatability; depending on the type of map (array-map vs. hash-map, etc.) this order of all this could shift dramatically; for small maps it's in order of keys added, in larger maps (hash-map) it's related to the hash of the key.

I'd say a good fixed order would be:

directive defs

scalars

enums

unions

interfaces

input-objects

objects (including Query, Mutation, etc., rolled in)

Sorting has been added to reflect reviews.

Another problem I'm just noticing is that the extras directives, types, etc. provided by com.walmartlabs.lacinia.federation/foundation-types need to be filtered back out.

hlship · 2022-08-12T16:38:49Z

src/com/walmartlabs/lacinia/federation.clj

+                :mutations (edn-mutations->sdl-mutations val)
+                :enums (edn-enums->sdl-enums val)
+                :directive-defs (edn-directive-defs->sdl-directives val)
+                :roots "")))


Roots could be important, though my experience with Apollo is that it's fragile if the service schemas don't agree on the names of the root objects. That may have changed since I looked at it > 1 year ago.

Well, I don't understand what you mean, can you elaborate on that?

In the SDL you can override the default names of the Query, Mutation, and Subscription objects using the schema keyword (https://spec.graphql.org/October2021/#sec-Schema).

So this code must honor that, but must also (as necessary) emit the schema content into the output SDL.

Oh, I didn't know there was such a grammar. reflected.

hlship

As I understand the Apollo federation documentation, the @extends and other directive, scalar types, etc. are not explicit in the input SDL but still available, and should not be generated into the output SDL. In the systems I've developed under federation, we start with SDL document and provided that document, as is ... you can see that in the code in master.

hlship · 2022-08-26T16:57:58Z

src/com/walmartlabs/lacinia/federation.clj

+        (nil? hd) ""
+        (= 'non-null hd) (str (apply-list edn-type->sdl-type tl) "!")
+        (= 'list hd) (str "[" (apply-list edn-type->sdl-type tl) "]")
+        (= 'String hd) "String"


These special cases would also be caught by the symbol? case below.

hlship · 2022-08-26T16:59:34Z

src/com/walmartlabs/lacinia/federation.clj

+  "Translate the edn lacinia schema to the SDL schema."
+  [schema]
+  (->> schema
+       (map (fn [[key val]]


Another problem I'm just noticing is that the extras directives, types, etc. provided by com.walmartlabs.lacinia.federation/foundation-types need to be filtered back out.

hmy3743 · 2022-08-28T02:18:45Z

As I understand the Apollo federation documentation, the @extends and other directive, scalar types, etc. are not explicit in the input SDL but still available, and should not be generated into the output SDL. In the systems I've developed under federation, we start with SDL document and provided that document, as is ... you can see that in the code in master.

I am not sure if I understand the review comment well, but if you look at the document about apollo's federation subgraph, it says that the server should provide some schemas. Then shouldn't the schema be exposed to the router through query{_service{sdl}}}?

hlship · 2022-08-28T17:23:22Z

I'll work on resolving this against the Apollo documentation.

hlship · 2022-09-02T16:33:45Z

Federation is a moving target, we're compatible with 1.0, but 2.0 changes things: https://www.apollographql.com/docs/federation/federated-types/federated-directives

In any case, I don't think the current version of this code will work because it includes the Federation (1.0) annotations and objects injected into the schema.

In the current version, based on reading SDL, you can see that those elements aren't in the input SDL (they are provided after that is parsed) and then we use the input SDL as-is when exposing the schema to the Apollo gateway.

I don't know exactly what would happen if we include those elements in the generated SDL but I suspect Apollo will reject the schema.

hlship · 2022-12-01T22:15:52Z

I'm struggling to find time to come back and finish reviewing this. I want to test what happens when the injected Fed 1.0 directives and types are not filtered out when generating SDL from the schema (that is, see how Apollo reacts).

hmy3743 added 3 commits August 8, 2022 23:10

Add edn federation

901178b

minor fix

c20b1e4

update document

d1dadc6

hmy3743 force-pushed the master branch from e168139 to d1dadc6 Compare August 8, 2022 14:31

Add missing enum translate logic

331483b

hlship reviewed Aug 12, 2022

View reviewed changes

hmy3743 added 12 commits August 16, 2022 11:07

add implements translate

e940cc6

remove match

e43ae52

update test for string escape

edf5442

add newline between functions

0810db5

sort root categories and scalars

1431062

fold queries, mutations, subscriptions in objects

a9c70fe

remove unused functions

f5d11f8

fix schema sort bug

2f6c9b6

indent sdl

7e08e79

Add blank line between schema types

04ecbcf

Add roots->schema translation

15c844e

Modify the fold function to match roots

37885e5

hmy3743 force-pushed the master branch from 4167d0f to 37885e5 Compare August 21, 2022 07:52

hlship reviewed Aug 26, 2022

View reviewed changes

Merge branch 'master' of https://github.com/walmartlabs/lacinia

6766a28

Delete a useless cond clause

eff986d

Add string escape and unescape

cfbe115

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify federation to be possible with edn schema as well #420

Modify federation to be possible with edn schema as well #420

hmy3743 commented Aug 8, 2022

CLAassistant commented Aug 8, 2022 •

edited

hlship left a comment

hlship Aug 12, 2022

hmy3743 Aug 16, 2022

hlship Aug 12, 2022

hmy3743 Aug 16, 2022

hlship Aug 12, 2022

hmy3743 Aug 16, 2022

hlship Aug 12, 2022

hlship Aug 12, 2022

hlship Aug 12, 2022

hmy3743 Aug 17, 2022

hlship Aug 12, 2022

hmy3743 Aug 17, 2022

hlship Aug 12, 2022

hmy3743 Aug 17, 2022

hlship Aug 26, 2022

hlship Aug 12, 2022

hmy3743 Aug 17, 2022

hlship Aug 19, 2022

hmy3743 Aug 21, 2022

hlship left a comment

hlship Aug 26, 2022

hlship Aug 26, 2022

hmy3743 commented Aug 28, 2022

hlship commented Aug 28, 2022

hlship commented Sep 2, 2022

hlship commented Dec 1, 2022

Modify federation to be possible with edn schema as well #420

Are you sure you want to change the base?

Modify federation to be possible with edn schema as well #420

Conversation

hmy3743 commented Aug 8, 2022

CLAassistant commented Aug 8, 2022 • edited

hlship left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hlship left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hmy3743 commented Aug 28, 2022

hlship commented Aug 28, 2022

hlship commented Sep 2, 2022

hlship commented Dec 1, 2022

CLAassistant commented Aug 8, 2022 •

edited