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

Neo metaphysics #809

Merged
merged 15 commits into from Nov 23, 2017

Conversation

@alloy
Member

alloy commented Nov 14, 2017

This bootstraps schema stitching inside MP. In the below example you’ll see the use of MP’s and Convection’s schemas combined and used.

{
  # This root field comes from MP’s schema.
  artist(id: "marina-abramovic-1") {
    name
  }
  # This root field comes from Convection’s schema.
  submission(id: 186) {
    # This field exists in Convection’s schema
    artist_id
    # This field is generated in the `mergedSchema` module by fetching
    # an `Artist` from MP’s schema using the above `artist_id` field.
    artist {
      name
    }
  }
}
{
  "data": {
    "artist": {
      "name": "Marina Abramović"
    },
    "submission": [
      {
        "artist_id": "marina-abramovic-1",
        "artist": {
          "name": "Marina Abramović"
        }
      }
    ]
  }
}

TODO


@alloy

This comment has been minimized.

Member

alloy commented Nov 14, 2017

I think that, if we feel this works well, we should start removing types from MP’s schema that are expressed in service’s own GraphQL schemas and stitch them in. The end goal would be to have pretty much nothing left in MP but stitching code.

@alloy

This comment has been minimized.

Member

alloy commented Nov 14, 2017

Here’s a few issues I’ve written up previously about other (long term) things to consider in this new stitching world:

index.js Outdated
mergedSchema().then(merged => {
schema = merged
app.listen(port, () => info(`Listening on ${port}`))
})

This comment has been minimized.

@alloy

alloy Nov 14, 2017

Member

Maybe all of the app setup should just move inside this promise and then schema can just be a const.

fragment: `fragment SubmissionArtist on Submission { artist_id }`,
resolve: (parent, args, context, info) => {
const id = parent.artist_id
return mergeInfo.delegate("query", "artist", { id }, context, info)

This comment has been minimized.

@alloy

alloy Nov 14, 2017

Member

This is basically at runtime generating a query like:

{
  artist(id: "<result of artist_id>") {
    # ...
  }
}

This comment has been minimized.

@alloy

alloy Nov 15, 2017

Member

Interesting note is that we may not want to have root fields for each type that we stitch into other types. Maybe we can use the node root field for this? 🤔

This comment has been minimized.

@alloy

alloy Nov 20, 2017

Member

The mergeSchemas author explained to me that this should be done in a few steps:

  1. Add the required root field to the original schema.
  2. Merge the schemas and setup the delegator to use the root field.
  3. Transform the merged schema to remove the root field (the delegator refers to the original schema).
@@ -53,6 +54,7 @@
"graphql": "^0.11.7",
"graphql-depth-limit": "^1.1.0",
"graphql-relay": "0.5.3",
"graphql-tools": "https://github.com/alloy/graphql-tools/releases/download/v2.7.3-alpha.1/graphql-tools-2.7.3-alpha.1.tgz",

This comment has been minimized.

@alloy
schemas: [localSchema, convectionSchema, linkTypeDefs],
// Prefer others over the local MP schema.
onTypeConflict: (_leftType, rightType) => {
console.warn(`[!] Type collision ${rightType}`) // eslint-disable-line no-console

This comment has been minimized.

@alloy

alloy Nov 14, 2017

Member

@orta There’s currently a bunch of consignments related type name conflicts, presumably because you’ve manually added these to MP’s schema before:

19:04:06 web.1       |  [!] Type collision ID
19:04:06 web.1       |  [!] Type collision Submission
19:04:06 web.1       |  [!] Type collision String
19:04:06 web.1       |  [!] Type collision Asset
19:04:06 web.1       |  [!] Type collision JSON
19:04:06 web.1       |  [!] Type collision Boolean
19:04:06 web.1       |  [!] Type collision Int

The ID, String, Boolean, and Int ones are built-in though, it seems weird that graphql-tools would trigger this callback for those 🤔

This comment has been minimized.

@orta

orta Nov 14, 2017

Member

Yeah, that's tricky, to remove Submission entirely in metaphysics - I'd have to recreate a bunch of mutations on convection, feasible, but time consuming

This comment has been minimized.

@orta

orta Nov 14, 2017

Member

That said, the submission type should be the exact same object - so maybe it's fine to return the convection one

This comment has been minimized.

@orta

orta Nov 14, 2017

Member

WRT the JSON type - I would assume they have the same behavior in terms of representing an any - I bet we get these primitives for every Ruby GraphQL

This comment has been minimized.

@sweir27

sweir27 Nov 14, 2017

Contributor

The mutations already exist in convection-- might need some updating but the basic functionality is there.

This comment has been minimized.

@alloy

alloy Nov 14, 2017

Member

Ah, and indeed the mutations are already automatically available in the stitched schema, I haven’t tried them yet, though.

screen shot 2017-11-14 at 19 16 18

@alloy alloy requested a review from saolsen Nov 14, 2017

fetch,
uri: process.env.CONVECTION_GRAPH_URL,
headers: {
Authorization: `Bearer ${process.env.CONVECTION_TOKEN}`,

This comment has been minimized.

@alloy

alloy Nov 14, 2017

Member

@sweir27 I guess this actually needs to be specified by the client at runtime, as it differs per user, right? I think that should be easy using the ‘link’ API.

This comment has been minimized.

@orta

orta Nov 14, 2017

Member

Yeah, the convection loaders have the ability to convert a grav token into a convection token. We'll need something like this for at least convection / impulse

This comment has been minimized.

@alloy

alloy Nov 14, 2017

Member

Ooohhh, we can probably just access those data loaders (including the token loader) during stitching resolving. Will take a look 👍

@alloy alloy requested review from dblock and ashkan18 Nov 14, 2017

return {
...context.headers,
headers: {
// authorization: `XApp ${config.GRAVITY_XAPP_TOKEN}`,

This comment has been minimized.

@alloy

alloy Nov 14, 2017

Member

@sweir27 Does this look like what you had in mind (to allow MP to fetch Convection’s schema on startup)?

This comment has been minimized.

@orta

orta Nov 15, 2017

Member

We chatted about this - it's a hard one, maybe looking into giving Metaphysics 2 client apps, one for the APIs we forward onwards and another for the server to server access. Then the server2server one can have privileged access for getting the schema.

This comment has been minimized.

@alloy

alloy Nov 15, 2017

Member

Just read the discussion in #platform-humans and that makes sense to me. My only question is how easy it is for MP to identify on each incoming request if the user has access to the more privileged app role? cc @mzikherman

This comment has been minimized.

@alloy

alloy Nov 15, 2017

Member

But once we have that we can just load 2 stitches schemas and execute against the correct one depending on the access the authenticated user has.

This comment has been minimized.

@alloy

alloy Nov 15, 2017

Member

Hmm, one thing that comes to mind is that in the more privileged stitched schema we would probably have stitching code that refers to the more privileged fields and thus leak schema details through MP being OSS 🤔

One solution I can think of is to have another app that imports all of MP and then extends the schema with privileged details and make that closed source? Probably only slightly more cumbersome, mostly in having to remember to add fields to the right app.

This comment has been minimized.

@alloy

alloy Nov 15, 2017

Member

Filed a new issue to discuss further #810

if (tokenLoader) {
return tokenLoader().then(({ token }) => {
return {
...context.headers,

This comment has been minimized.

@alloy

alloy Nov 15, 2017

Member

Note to self, this is wrong, it needs to add all headers to the below object instead.

}
`
return mergeSchemas({

This comment has been minimized.

@ashkan18

ashkan18 Nov 15, 2017

Contributor

This basically happens once during start up of MP, so one thing to consider is, lets say Convection's GraphLQ schema got updated and there was a new field, we'd ONLY get that update if we restart MP, right?

For now we might be fine with manual restarts of MP, but eventually if this became more problematic (as we add more and more services) it might make sense to use event stream for reloading schema. Each time an app updated it schema we'd refresh this.

This comment has been minimized.

@alloy

alloy Nov 15, 2017

Member

Correct 👍 And that sounds like a good solution to keep in mind.

headers: {
...context.headers,

This comment has been minimized.

@alloy

alloy Nov 15, 2017

Member

Maybe we don’t even really want to pass on (all) headers that a client specifies? I can imagine that we do want e.g. the request ID that @sweir27 had been working on.

This comment has been minimized.

@orta

orta Nov 16, 2017

Member

Yeah, we'd want the request ID - but that's probably it (at least in convection I've not seen any header handling outside of auth)

This comment has been minimized.

@orta

orta Nov 18, 2017

Member

Note: with #818 there's more headers we want to pass through

index.js Outdated
...loaders,
},
formatError: graphqlErrorHandler(req.body),
validationRules: [depthLimit(queryLimit)],

This comment has been minimized.

@craigspaeth

craigspaeth Nov 16, 2017

Contributor

Nice! Wasn't aware of GraphQL depth limit.

This comment has been minimized.

@alloy

alloy Nov 16, 2017

Member

I… dislike it so much 😞 It’s such an arbitrary 🔨

Of course I understand why we’d want it at the moment, but in the long term I see only allowing persisted queries in production (or limiting depth query for not persisted queries) as a better solution.

function createConvectionLink() {
const httpLink = createHttpLink({
fetch,
uri: `${process.env.CONVECTION_API_BASE}/graphql`,

This comment has been minimized.

@craigspaeth

craigspaeth Nov 16, 2017

Contributor

Seems like this uri might be the main piece of convection-unique code here. I wonder if there's an opportunity to wrap this boilerplate in a way that'd make it easy to do just point to the urls we're stitching.

This comment has been minimized.

@alloy

alloy Nov 16, 2017

Member

The other Convection specific code is using the convectionTokenLoader. I agree that this can probably be made more generic, but was punting on that until we add another service.

@craigspaeth

This comment has been minimized.

Contributor

craigspaeth commented Nov 16, 2017

Very exciting stuff 👏 I think bootstrapping stitching inside MP with the intent to whittle it down to mostly just stitching code and having logic in downstream GraphQL APIs is a great strategy. This is a very exciting step towards a beautiful microservice story where MP is a single orchestrator of downstream GraphQL APIs and we can eliminate redundant fields/logic/caching layers. It'd also be cool to consider writing a HAL > GraphQL stitching library that could introspect the Hypermedia links (and JSON schemas?) and generate GraphQL schemas + resolvers for it 🤔 💥 .

One general comment not to block this PR is that I'm noticing a lot of code that I struggle to follow in these glue code layers. e.g. There's a lot of factory/dependency-injection-like patterns being used in the loader/stitching layers and I wonder if there're simpler ways to organize these modules. It seems like the tools encourage this as well, so maybe they're partly responsible. In any case, just a general comment—not sure how to make it better myself.

👏 amazing work!

@orta

This comment has been minimized.

Member

orta commented Nov 16, 2017

artsy/convection#122 is pretty close to being ready

@stubailo

This comment has been minimized.

stubailo commented Nov 18, 2017

Hey, just found this from the PR here: apollographql/graphql-tools#484

Can you help me figure out how that function existing is messing things up? I see the PR just deletes it, but wanted to learn more.

@alloy

This comment has been minimized.

Member

alloy commented Nov 18, 2017

@stubailo The test in that PR should illustrate it well if you comment out the deletion change.

In short, the problem is that after selecting a subset of fields from the original schema the function in the merged schema may not return true if the selection doesn’t satisfy the data it’s looking for.

For example, a isTypeOf implementation for the Artist type in the merged schema that looks for the data to contain name and birthday keys will fail to pass data returned from the original schema with the following query:

{
  artist(id: “banksy”) {
    name
  }
}
@alloy

This comment has been minimized.

Member

alloy commented Nov 22, 2017

@saolsen Did a bit of refactoring re tracer work ff4083f and I also pass these IDs along to Convection a3c3c13

return {
"X-Request-Id": requestID,
"x-datadog-trace-id": traceId,
"x-datadog-parent-id": parentSpanId,

This comment has been minimized.

@alloy

alloy Nov 22, 2017

Member

@saolsen I noticed that these last two are not strings. I’m sure that all entries in response.headers get coerced to strings by Express before the response is sent to the client, but it was slightly unexpected to see these objects in there. Do you think we can add toString() calls here?

alloy added some commits Nov 14, 2017

@alloy

This comment has been minimized.

Member

alloy commented Nov 22, 2017

@craigspaeth

It'd also be cool to consider writing a HAL > GraphQL stitching library that could introspect the Hypermedia links (and JSON schemas?) and generate GraphQL schemas + resolvers for it 🤔 💥 .

I don’t know how well that would work when we want to design the schema without necessarily following the same shape in which these endpoints return data. Maybe an example of what you have in mind could help?

One general comment not to block this PR is that I'm noticing a lot of code that I struggle to follow in these glue code layers. e.g. There's a lot of factory/dependency-injection-like patterns being used in the loader/stitching layers and I wonder if there're simpler ways to organize these modules. It seems like the tools encourage this as well, so maybe they're partly responsible. In any case, just a general comment—not sure how to make it better myself.

It’s true and I agree that it can be a bit much when reading for the first few times. It does work nicely with how graphql-js works and especially testing, though (no more need for rewiring). I’m on the fence, because thus far everybody ended up understanding how it works.

@alloy alloy assigned orta and unassigned mzikherman Nov 22, 2017

return rightType
},
resolvers: mergeInfo => ({
Submission: {

This comment has been minimized.

@damassi

damassi Nov 23, 2017

Member

@alloy - can you explain how adding in additional schemas will work in relation to the above line? Is this a namespace?

This comment has been minimized.

@alloy

alloy Nov 23, 2017

Member

No this is not a namespace, it’s the way in which schemas are created using graphql-tools https://github.com/apollographql/graphql-tools/blob/master/README.md. In this case the code you’re referring to is the implementation for the schema extension just above (in IDL).

@orta

This comment has been minimized.

Member

orta commented Nov 23, 2017

OK, it's time - let's give this a shot.

@orta orta merged commit 5cd73c2 into master Nov 23, 2017

2 checks passed

Peril All green. Good on 'ya.
Details
ci/circleci Your tests passed on CircleCI!
Details

@orta orta deleted the neo-metaphysics branch Nov 23, 2017

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