Relay Global Object Identification support. #313

Merged
merged 16 commits into from Jul 6, 2016

Conversation

Projects
None yet
6 participants
@alloy
Contributor

alloy commented Jun 11, 2016

This adds the Relay Node ID support that we need to make full use of Relay’s capabilities.

  • It adds a Node interface, which identifies an object with the __id field. From the __id value it is able to deduce the actual type and its type-specific ID.
  • Article, Artist, Artwork, Partner, and PartnerShow have gained the Node interface. Any other types that will be used with Relay in the future will need to get support added.
  • All types that have a id field now also have a __id field, because even though you can’t look all of those up through the Node interface yet, the globally unique IDs does mean that Relay can already cache them efficiently.
  • I started using GraphQLID for all id fields instead of GraphQLString. I did not yet add the non-null type to all of those, but I have a feeling that there are lots of places where a value (such as an ID) cannot be null.
@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Jun 11, 2016

Contributor

Fetch Artwork Global ID

{
  artwork(id: "mr-brainwash-charlie-chaplin-red") {
    __id
  }
}
{
  "data": {
    "artwork": {
      "__id": "QXJ0d29yazptci1icmFpbndhc2gtY2hhcmxpZS1jaGFwbGluLXJlZA=="
    }
  }
}

Fetch Artwork through Node interface

{
  node(__id: "QXJ0d29yazptci1icmFpbndhc2gtY2hhcmxpZS1jaGFwbGluLXJlZA==") {
    __typename
    ... on Artwork {
      __id
      _id
      id
      title
    }
  }
}
{
  "data": {
    "node": {
      "__typename": "Artwork",
      "__id": "QXJ0d29yazptci1icmFpbndhc2gtY2hhcmxpZS1jaGFwbGluLXJlZA==",
      "_id": "575a0dcfcd530e4f0d00022e",
      "id": "mr-brainwash-charlie-chaplin-red",
      "title": "Charlie Chaplin (Red)"
    }
  }
}
Contributor

alloy commented Jun 11, 2016

Fetch Artwork Global ID

{
  artwork(id: "mr-brainwash-charlie-chaplin-red") {
    __id
  }
}
{
  "data": {
    "artwork": {
      "__id": "QXJ0d29yazptci1icmFpbndhc2gtY2hhcmxpZS1jaGFwbGluLXJlZA=="
    }
  }
}

Fetch Artwork through Node interface

{
  node(__id: "QXJ0d29yazptci1icmFpbndhc2gtY2hhcmxpZS1jaGFwbGluLXJlZA==") {
    __typename
    ... on Artwork {
      __id
      _id
      id
      title
    }
  }
}
{
  "data": {
    "node": {
      "__typename": "Artwork",
      "__id": "QXJ0d29yazptci1icmFpbndhc2gtY2hhcmxpZS1jaGFwbGluLXJlZA==",
      "_id": "575a0dcfcd530e4f0d00022e",
      "id": "mr-brainwash-charlie-chaplin-red",
      "title": "Charlie Chaplin (Red)"
    }
  }
}
@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Jun 11, 2016

Contributor

The ID is a base64 encoded combination of the type and its real ID, so that it can be deconstructed and resolved regardless of type.

ruby -r base64 -e 'p Base64.decode64("QXJ0d29yazptci1icmFpbndhc2gtY2hhcmxpZS1jaGFwbGluLXJlZA==")'
"Artwork:mr-brainwash-charlie-chaplin-red"
Contributor

alloy commented Jun 11, 2016

The ID is a base64 encoded combination of the type and its real ID, so that it can be deconstructed and resolved regardless of type.

ruby -r base64 -e 'p Base64.decode64("QXJ0d29yazptci1icmFpbndhc2gtY2hhcmxpZS1jaGFwbGluLXJlZA==")'
"Artwork:mr-brainwash-charlie-chaplin-red"
@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Jun 12, 2016

Contributor

Decided to try to get Relay to use a differently named ID field so we don’t have to update all clients, especially not Eigen, which uses it in 1 place for analytics facebook/relay#1061 (comment)

Contributor

alloy commented Jun 12, 2016

Decided to try to get Relay to use a differently named ID field so we don’t have to update all clients, especially not Eigen, which uses it in 1 place for analytics facebook/relay#1061 (comment)

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Jun 15, 2016

Contributor

Well, seems like trying to change that requirement in upstream Relay might take longer than I want, so for now I’ll just continue as-is, but if there’s a hint of things changing in Relay before this PR is finalise I’ll switch it over.

Contributor

alloy commented Jun 15, 2016

Well, seems like trying to change that requirement in upstream Relay might take longer than I want, so for now I’ll just continue as-is, but if there’s a hint of things changing in Relay before this PR is finalise I’ll switch it over.

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Jun 16, 2016

Contributor

@ashfurrow I see you’re using the id field from MP https://github.com/artsy/eigen/blob/5427294088fdd8c5a51bc7d3cdeb644c9f0f42ef/Artsy/Networking/ARRouter.m#L1029 & https://github.com/artsy/eigen/blob/5427294088fdd8c5a51bc7d3cdeb644c9f0f42ef/Artsy/Networking/ARRouter.m#L1031. Would you be able to say if that ID is used outside of the context of MP? i.e. do you expect it be an ID that you can use to fetch data from other APIs?

Contributor

alloy commented Jun 16, 2016

@ashfurrow I see you’re using the id field from MP https://github.com/artsy/eigen/blob/5427294088fdd8c5a51bc7d3cdeb644c9f0f42ef/Artsy/Networking/ARRouter.m#L1029 & https://github.com/artsy/eigen/blob/5427294088fdd8c5a51bc7d3cdeb644c9f0f42ef/Artsy/Networking/ARRouter.m#L1031. Would you be able to say if that ID is used outside of the context of MP? i.e. do you expect it be an ID that you can use to fetch data from other APIs?

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Jun 16, 2016

Contributor

If it turns out that there’s more already deployed iOS code that relies on id fields, then I guess it makes more sense to push for adding support for a different field to Relay.

(Although in the case I linked to in the previous comment the model is a Sale and we’re not using that in Eigen atm, I think it would be very confusing API to have id be something different in different places.)

Contributor

alloy commented Jun 16, 2016

If it turns out that there’s more already deployed iOS code that relies on id fields, then I guess it makes more sense to push for adding support for a different field to Relay.

(Although in the case I linked to in the previous comment the model is a Sale and we’re not using that in Eigen atm, I think it would be very confusing API to have id be something different in different places.)

test/schema/artist/carousel.js
@@ -54,8 +54,8 @@ describe('ArtistCarousel type', () => {
const gravity = ArtistCarousel.__get__('gravity');
const query = `
{
- artist(id: "foo-bar") {
- id
+ artist(slug_or_id: "foo-bar") {

This comment has been minimized.

@alloy

alloy Jun 16, 2016

Contributor

Thoughts on the name of this parameter?

@alloy

alloy Jun 16, 2016

Contributor

Thoughts on the name of this parameter?

This comment has been minimized.

@ashfurrow

ashfurrow Jun 16, 2016

Member

Makes sense.

@ashfurrow

ashfurrow Jun 16, 2016

Member

Makes sense.

This comment has been minimized.

@dzucconi

dzucconi Jun 16, 2016

Contributor

We can't just call it id and it do the right thing?

@dzucconi

dzucconi Jun 16, 2016

Contributor

We can't just call it id and it do the right thing?

This comment has been minimized.

@dzucconi

dzucconi Jun 16, 2016

Contributor

:reads above:

@dzucconi

dzucconi Jun 16, 2016

Contributor

:reads above:

This comment has been minimized.

@dzucconi

dzucconi Jun 16, 2016

Contributor

gid? key?

@dzucconi

dzucconi Jun 16, 2016

Contributor

gid? key?

@ashfurrow

This comment has been minimized.

Show comment
Hide comment
@ashfurrow

ashfurrow Jun 16, 2016

Member

We use the id and _id fields of the Sale object in different ways. For _id, we call it the causalityID because it's what we need to connect and authenticate against the causality web socket API. The id field we call the liveSaleID and is used as a more traditional gravity id (basically as a slug) to direct the user to auction registration URLs and to fetch bidders and sale info from gravity directly. The distinction between the two isn't immediately obvious to me, and I regret that in my haste to get causality integration working, I may have misused the fields.

Please advise on next steps – it's possible to disable the native live experience and fall back to a mobile web view (indeed, we're planning on doing so already). If this is an infrastructure change that eigen needs to make, we can roll it into our existing update.

Member

ashfurrow commented Jun 16, 2016

We use the id and _id fields of the Sale object in different ways. For _id, we call it the causalityID because it's what we need to connect and authenticate against the causality web socket API. The id field we call the liveSaleID and is used as a more traditional gravity id (basically as a slug) to direct the user to auction registration URLs and to fetch bidders and sale info from gravity directly. The distinction between the two isn't immediately obvious to me, and I regret that in my haste to get causality integration working, I may have misused the fields.

Please advise on next steps – it's possible to disable the native live experience and fall back to a mobile web view (indeed, we're planning on doing so already). If this is an infrastructure change that eigen needs to make, we can roll it into our existing update.

@@ -1,5 +1,6 @@
{
"presets": ["es2015"],
+ "plugins": ["transform-object-rest-spread"],

This comment has been minimized.

@dzucconi

dzucconi Jun 16, 2016

Contributor

Thank youuu

@dzucconi

dzucconi Jun 16, 2016

Contributor

Thank youuu

This comment has been minimized.

@alloy

alloy Jul 1, 2016

Contributor

Oh you had wanted this? I reverted it, because it seems like I won’t need it after all :/

Let me know if anyone wants this in anyways and I’ll re-apply the patches.

@alloy

alloy Jul 1, 2016

Contributor

Oh you had wanted this? I reverted it, because it seems like I won’t need it after all :/

Let me know if anyone wants this in anyways and I’ll re-apply the patches.

This comment has been minimized.

@alloy

alloy Jul 4, 2016

Contributor

Ok, re-adding it after all.

@alloy

alloy Jul 4, 2016

Contributor

Ok, re-adding it after all.

alloy added some commits Jun 11, 2016

[package] Add graphql-relay dependency.
Had to use an older version because we use an older version of the
graphql package.
@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Jul 2, 2016

Contributor

It seems like my code is failing on Node 5, I’m using Node 6 locally.

Ok, was using Array.prototype.includes which isn’t available on Node 5 yet.

Contributor

alloy commented Jul 2, 2016

It seems like my code is failing on Node 5, I’m using Node 6 locally.

Ok, was using Array.prototype.includes which isn’t available on Node 5 yet.

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Jul 2, 2016

Contributor

Paging @broskoski, now that @dzucconi has officially left.

Contributor

alloy commented Jul 2, 2016

Paging @broskoski, now that @dzucconi has officially left.

@alloy alloy changed the title from [WIP] Relay Global Object Identification support. to Relay Global Object Identification support. Jul 2, 2016

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Jul 2, 2016

Contributor

Oops, forgot that @broskoski is out for the week, paging @mzikherman, sir.

Contributor

alloy commented Jul 2, 2016

Oops, forgot that @broskoski is out for the week, paging @mzikherman, sir.

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Jul 4, 2016

Contributor

Noticed I still need to add support to to Author and Partner.

Contributor

alloy commented Jul 4, 2016

Noticed I still need to add support to to Author and Partner.

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Jul 4, 2016

Contributor

For review-sake, here’s a full diff of the schema: https://gist.github.com/alloy/6dcf4e26303de0eb922a264b6b1d50d5

Contributor

alloy commented Jul 4, 2016

For review-sake, here’s a full diff of the schema: https://gist.github.com/alloy/6dcf4e26303de0eb922a264b6b1d50d5

@alloy alloy referenced this pull request in artsy/emission Jul 5, 2016

Merged

Added a Home container #192

schema/fair.js
GraphQLString,
GraphQLBoolean,
GraphQLNonNull,
} from 'graphql';
+const OrganizerType = new GraphQLObjectType({

This comment has been minimized.

This comment has been minimized.

@alloy

alloy Jul 5, 2016

Contributor

Could do, but unlike const naming in Ruby that won’t really change much on the outside as it’s not exported. I think that in the scope of the ‘fair’ file it’s clear what the ‘organiser’ organises. Let me know if you feel strong about this, I’m on the fence.

@alloy

alloy Jul 5, 2016

Contributor

Could do, but unlike const naming in Ruby that won’t really change much on the outside as it’s not exported. I think that in the scope of the ‘fair’ file it’s clear what the ‘organiser’ organises. Let me know if you feel strong about this, I’m on the fence.

This comment has been minimized.

@alloy

alloy Jul 5, 2016

Contributor

Done.

@alloy

alloy Jul 5, 2016

Contributor

Done.

+// To support a type, it should:
+// * specify that it implements the Node interface
+// * add the Node `__id` fields
+// * implement a `isType` function that from a payload determines if the payload is of that type

This comment has been minimized.

@mzikherman

mzikherman Jul 5, 2016

Contributor

👍

@mzikherman

mzikherman Jul 5, 2016

Contributor

👍

+const SupportedTypes = {
+ types: ['Article', 'Artist', 'Artwork', 'Partner', 'PartnerShow'],
+ // To prevent circular dependencies, when this file is loaded, the modules are lazily loaded.
+ typeModule: _.memoize(type => require(`./${_.snakeCase(type)}`).default),

This comment has been minimized.

@mzikherman

mzikherman Jul 5, 2016

Contributor

nice makes sense

@mzikherman

mzikherman Jul 5, 2016

Contributor

nice makes sense

@@ -265,6 +263,8 @@ const PartnerShow = {
return show;
});
},
+ // ObjectIdentification
+ isType: (obj) => obj.partner !== undefined && obj.display_on_partner_profile !== undefined,

This comment has been minimized.

@mzikherman

mzikherman Jul 5, 2016

Contributor

wonder if there's a better way to do these? /shrug

@mzikherman

mzikherman Jul 5, 2016

Contributor

wonder if there's a better way to do these? /shrug

This comment has been minimized.

@alloy

alloy Jul 5, 2016

Contributor

I agree, it feels… meh. The most fool-proof solution would be to add a type field to the Gravity response, but that also feels… meh ¯_(ツ)_/¯

Today I learned of GraphQLObjectType.isTypeOf, not sure what it’s for yet, but if similar in purpose I might at least use that in the future, instead of introducing my own API.

@alloy

alloy Jul 5, 2016

Contributor

I agree, it feels… meh. The most fool-proof solution would be to add a type field to the Gravity response, but that also feels… meh ¯_(ツ)_/¯

Today I learned of GraphQLObjectType.isTypeOf, not sure what it’s for yet, but if similar in purpose I might at least use that in the future, instead of introducing my own API.

schema/sale/index.js
@@ -34,17 +36,41 @@ export function auctionState({ start_at, end_at, live_start_at }) {
}
}
+const BidIncrement = new GraphQLObjectType({
+ name: 'BidIncrements',

This comment has been minimized.

@mzikherman

mzikherman Jul 5, 2016

Contributor

should this be singular now?

@mzikherman

mzikherman Jul 5, 2016

Contributor

should this be singular now?

This comment has been minimized.

@alloy

alloy Jul 5, 2016

Contributor

Oh yeah, good catch 👍

@alloy

alloy Jul 5, 2016

Contributor

Oh yeah, good catch 👍

This comment has been minimized.

@alloy

alloy Jul 5, 2016

Contributor

Done.

@alloy

alloy Jul 5, 2016

Contributor

Done.

+ },
+ };
+
+ _.keys(tests).forEach((typeName) => {

This comment has been minimized.

@mzikherman

mzikherman Jul 5, 2016

Contributor

nice tests!

@mzikherman

mzikherman Jul 5, 2016

Contributor

nice tests!

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Jul 5, 2016

Contributor

I changed nothing with regards to the NPM modules in the last commit, yet CI now fails on that 😭

Contributor

alloy commented Jul 5, 2016

I changed nothing with regards to the NPM modules in the last commit, yet CI now fails on that 😭

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Jul 5, 2016

Contributor

I’m not even sure why it tries to install fsevents 1.0.5, the shrinkwrap file has it pegged to 1.0.12

Contributor

alloy commented Jul 5, 2016

I’m not even sure why it tries to install fsevents 1.0.5, the shrinkwrap file has it pegged to 1.0.12

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Jul 5, 2016

Contributor

If I ssh into CI and run npm install it actually does install fsevents 1.0.12. Some stale dependency cache somewhere?

Contributor

alloy commented Jul 5, 2016

If I ssh into CI and run npm install it actually does install fsevents 1.0.12. Some stale dependency cache somewhere?

@mzikherman

This comment has been minimized.

Show comment
Hide comment
@mzikherman

mzikherman Jul 5, 2016

Contributor

Noo :( sorry for making you go back in for minor-ish changes

Contributor

mzikherman commented Jul 5, 2016

Noo :( sorry for making you go back in for minor-ish changes

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Jul 5, 2016

Contributor

Making the fsevents 1.0.12 dependency explicit does change something, but now it hits the following:

npm ERR! enoent ENOENT: no such file or directory, rename '/home/runner/metaphysics/node_modules/tar-pack/node_modules/isarray' -> '/home/runner/metaphysics/node_modules/tar-pack/node_modules/isarray'
npm ERR! enoent This is most likely not a problem with npm itself
npm ERR! enoent and is related to npm not being able to find a file.

Like… I dunno… it’s trying to rename a path to the same path? What’s up with that?

I give up, this seems very much a CI issue. I have no issues locally and also not when SSH-ing into CI. I’m going to revert the last commit and call it a day.

Noo :( sorry for making you go back in for minor-isa changes

Nah, that change should be totally irrelevant. I would say, if you feel good about the code, go ahead and merge it and give CI some time to chill, if that’s something y’all do in some cases.

Contributor

alloy commented Jul 5, 2016

Making the fsevents 1.0.12 dependency explicit does change something, but now it hits the following:

npm ERR! enoent ENOENT: no such file or directory, rename '/home/runner/metaphysics/node_modules/tar-pack/node_modules/isarray' -> '/home/runner/metaphysics/node_modules/tar-pack/node_modules/isarray'
npm ERR! enoent This is most likely not a problem with npm itself
npm ERR! enoent and is related to npm not being able to find a file.

Like… I dunno… it’s trying to rename a path to the same path? What’s up with that?

I give up, this seems very much a CI issue. I have no issues locally and also not when SSH-ing into CI. I’m going to revert the last commit and call it a day.

Noo :( sorry for making you go back in for minor-isa changes

Nah, that change should be totally irrelevant. I would say, if you feel good about the code, go ahead and merge it and give CI some time to chill, if that’s something y’all do in some cases.

@mzikherman

This comment has been minimized.

Show comment
Hide comment
@mzikherman

mzikherman Jul 5, 2016

Contributor

I'll try to rebuild it tonight (explore Semaphore's dependency cache- maybe can be force updated?), and if not I'll merge and babysit the staging deploy.

Either way you'll have it by (your) tmrw morning :)

Thanks for the awesome PR.

Contributor

mzikherman commented Jul 5, 2016

I'll try to rebuild it tonight (explore Semaphore's dependency cache- maybe can be force updated?), and if not I'll merge and babysit the staging deploy.

Either way you'll have it by (your) tmrw morning :)

Thanks for the awesome PR.

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Jul 5, 2016

Contributor

explore Semaphore's dependency cache- maybe can be force updated?

My naive attempt was based on this: https://semaphoreci.com/docs/caching-between-builds.html, but the .semaphore-cache was just empty :-/

Either way you'll have it by (your) tmrw morning :)

Thanks for the awesome PR.

💃🙌

Contributor

alloy commented Jul 5, 2016

explore Semaphore's dependency cache- maybe can be force updated?

My naive attempt was based on this: https://semaphoreci.com/docs/caching-between-builds.html, but the .semaphore-cache was just empty :-/

Either way you'll have it by (your) tmrw morning :)

Thanks for the awesome PR.

💃🙌

@mzikherman

This comment has been minimized.

Show comment
Hide comment
@mzikherman

mzikherman Jul 6, 2016

Contributor

I expired the cache and seems to have worked!

https://semaphoreci.com/artsy-it/metaphysics/settings/admin (need to be logged in as it@)

Contributor

mzikherman commented Jul 6, 2016

I expired the cache and seems to have worked!

https://semaphoreci.com/artsy-it/metaphysics/settings/admin (need to be logged in as it@)

@mzikherman mzikherman merged commit b538efb into master Jul 6, 2016

1 check passed

semaphoreci The build passed on Semaphore.
Details
@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Jul 6, 2016

Contributor

Yay!!

Contributor

alloy commented Jul 6, 2016

Yay!!

@alloy alloy deleted the relay branch Sep 8, 2017

@ermik

This comment has been minimized.

Show comment
Hide comment
@ermik

ermik May 12, 2018

What do you guys think of using UUIDv5 as __id-workaround across different services instead of (just) base64-encoding a string? (Let's just say I can enforce truly unique GUID generation across different data stores, so collision is solved.)

(Oh didn't mean to assign people, sorry.)

ermik commented May 12, 2018

What do you guys think of using UUIDv5 as __id-workaround across different services instead of (just) base64-encoding a string? (Let's just say I can enforce truly unique GUID generation across different data stores, so collision is solved.)

(Oh didn't mean to assign people, sorry.)

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy May 14, 2018

Contributor

@ermik It wouldn’t solve our issue, which is that the id field was already in use (and non-global) and we couldn’t change that without breaking some clients’ assumptions. Aside from that, I do like being able to encode whatever is needed to reach the same node again, which we also do with some complex filter fields that take many params.

Contributor

alloy commented May 14, 2018

@ermik It wouldn’t solve our issue, which is that the id field was already in use (and non-global) and we couldn’t change that without breaking some clients’ assumptions. Aside from that, I do like being able to encode whatever is needed to reach the same node again, which we also do with some complex filter fields that take many params.

@ermik

This comment has been minimized.

Show comment
Hide comment
@ermik

ermik May 15, 2018

@alloy sounds interesting. It's almost like a case of a funky collision turned into an interesting pattern. Thanks for the insight. I'm trying to grasp the ontology of your system to advance my code — learning from mistakes (and smarts) of others.

ermik commented May 15, 2018

@alloy sounds interesting. It's almost like a case of a funky collision turned into an interesting pattern. Thanks for the insight. I'm trying to grasp the ontology of your system to advance my code — learning from mistakes (and smarts) of others.

@ermik

This comment has been minimized.

Show comment
Hide comment
@ermik

ermik May 29, 2018

How do I use the object identification mechanism you have in this project on stitched schemas?

For example, I have two separate services both supporting relay and exposing Node root field, how do I stitch them together and resolve the node query in a metaphysics-like app? I'd really appreciate any guidance or tips on this. Thank you! ❤️

ermik commented May 29, 2018

How do I use the object identification mechanism you have in this project on stitched schemas?

For example, I have two separate services both supporting relay and exposing Node root field, how do I stitch them together and resolve the node query in a metaphysics-like app? I'd really appreciate any guidance or tips on this. Thank you! ❤️

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy May 30, 2018

Contributor

@ermik We have not been doing this yet. My idea was that the different services would encode the IDs themselves but prepend the IDs with their service name. Then the stitched schema could have a resolver that delegates to the responsible backend service by just matching the start of the IDs with base64 encoded versions of the services names (be sure to take into account how base64 encoding works, an example can be found here).

Contributor

alloy commented May 30, 2018

@ermik We have not been doing this yet. My idea was that the different services would encode the IDs themselves but prepend the IDs with their service name. Then the stitched schema could have a resolver that delegates to the responsible backend service by just matching the start of the IDs with base64 encoded versions of the services names (be sure to take into account how base64 encoding works, an example can be found here).

@ermik

This comment has been minimized.

Show comment
Hide comment
@ermik

ermik Jun 5, 2018

So, since I will need more answers from you guys 😉, I hope this will be interesting for you to see. I actually didn't get your response until just now and hacked it together the best i could.

First off, an I might PR this in the future, but I found that this repo is, as far as I understand is missing something like the following:

/**
 * update-master-schema.js
 * Grabs the stitched schema to allow Relay compiler and other tools
 * dependent on schema definition files to work.
 */
import fs from "fs";
import { printSchema } from "graphql/utilities";
import path from "path";
import { mergeSchemas } from "../server/graphql/lib/stitching/mergedSchemas";

let schema;
async function magic() {
  schema = await mergeSchemas();
  fs.writeFileSync(
    path.join(__dirname, "../server/graphql/master.graphql"),
    printSchema(schema)
  );
}

magic();

Then, mangling the beauty that is your code:

/*
 * megedSchemas.js
 * Prepares the edge layer to resolve graphql requests to the internal APIs by 
 * using schema merge, delegation, and transformation tooling 
 */

/* ... */

  const mergedSchema = _mergeSchemas({
    schemas: [
      lloydSchema,
      localSchema
      /**/
    ],
    resolvers: {
      Query: {
        // delegating directly, no subschemas or mergeInfo
        node: (parent, args, context, info) => {
          return info.mergeInfo.delegateToSchema({
            schema: lloydSchema,
            operation: "query",
            fieldName: "node",
            args: {
              id: args.id
            },
            context,
            info
          });
        }
      }
    }

/* ... */

Which works for a single service, but must not be hard (I assume) to make it work for multiple via the same mapping mechanism you use for loaders for example.

ermik commented Jun 5, 2018

So, since I will need more answers from you guys 😉, I hope this will be interesting for you to see. I actually didn't get your response until just now and hacked it together the best i could.

First off, an I might PR this in the future, but I found that this repo is, as far as I understand is missing something like the following:

/**
 * update-master-schema.js
 * Grabs the stitched schema to allow Relay compiler and other tools
 * dependent on schema definition files to work.
 */
import fs from "fs";
import { printSchema } from "graphql/utilities";
import path from "path";
import { mergeSchemas } from "../server/graphql/lib/stitching/mergedSchemas";

let schema;
async function magic() {
  schema = await mergeSchemas();
  fs.writeFileSync(
    path.join(__dirname, "../server/graphql/master.graphql"),
    printSchema(schema)
  );
}

magic();

Then, mangling the beauty that is your code:

/*
 * megedSchemas.js
 * Prepares the edge layer to resolve graphql requests to the internal APIs by 
 * using schema merge, delegation, and transformation tooling 
 */

/* ... */

  const mergedSchema = _mergeSchemas({
    schemas: [
      lloydSchema,
      localSchema
      /**/
    ],
    resolvers: {
      Query: {
        // delegating directly, no subschemas or mergeInfo
        node: (parent, args, context, info) => {
          return info.mergeInfo.delegateToSchema({
            schema: lloydSchema,
            operation: "query",
            fieldName: "node",
            args: {
              id: args.id
            },
            context,
            info
          });
        }
      }
    }

/* ... */

Which works for a single service, but must not be hard (I assume) to make it work for multiple via the same mapping mechanism you use for loaders for example.

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Jun 5, 2018

Contributor

Indeed we’re not persisting the stitched schema, we’ve started using tooling such as graphql-fetch-schema to fetch when the client needs it. Thanks for offering to PR, though!

As for the delegation, my thought was to do something like:

        node: (parent, args, context, info) => {
          let schema
          // Encode schema name prefix in such a way that it takes into account Base64 encoding boundaries.
          if (args.id.startsWith(base64("lloyd"))) {
            schema = lloydSchema
          }
          return info.mergeInfo.delegateToSchema({
            schema,
            operation: "query",
            fieldName: "node",
            args: {
              id: args.id
            },
            context,
            info
          });
        }
Contributor

alloy commented Jun 5, 2018

Indeed we’re not persisting the stitched schema, we’ve started using tooling such as graphql-fetch-schema to fetch when the client needs it. Thanks for offering to PR, though!

As for the delegation, my thought was to do something like:

        node: (parent, args, context, info) => {
          let schema
          // Encode schema name prefix in such a way that it takes into account Base64 encoding boundaries.
          if (args.id.startsWith(base64("lloyd"))) {
            schema = lloydSchema
          }
          return info.mergeInfo.delegateToSchema({
            schema,
            operation: "query",
            fieldName: "node",
            args: {
              id: args.id
            },
            context,
            info
          });
        }
@ermik

This comment has been minimized.

Show comment
Hide comment
@ermik

ermik Jun 5, 2018

There is a need for a contract that adheres to the graphql/relay spec. The node field itself holds the promise of identifying any object that implements the node interface by only it's ID. So when we look upstream, that same promise, for a given type, is the held by the particular service — never two at once (same as in case of eventual consistency needing a master of some sort). Any further linkage is well-served by service-to-service connections e.g. gRPC & Thrift which is how we can pick up the slack of resolving deeply nested/relational queries: get the user from "users" service, but also get his likes from the "likes" service can be resolved by the users service alone. (maybe an antipattern tho, don't know, but it seems to be very much like the illustration in Artsy tech blog) Correct me if I am mistaken, but afaik GraphQL is of no help here, as it doesn't resolve children until parent query comes back.

Based on these considerations I'd keep the decision of picking a schema it in a controlled format of a static map:

/** @flow */

import { fromGlobalId } from "graphql-relay";
import type { GraphQLSchema } from "graphql";

type ObjectType = string;
type Service = string;
type NodeMap = { [ObjectType]: Service };

const nodeResolvers: NodeMap = {
  ["Vendor"]: "lloyd",
  ["ElasticSearch"]: "local"
};

/**
 * @func schemaFromGlobalId retrieves delegate schema for a Relay Global ID
 * @param {string} id — Relay standard base64 encoded tuple of shape "type:id"
 * @desc instead of base64() cost if encoding servicename as part of type
 * we will pay expected base64() cost associated with Relay services; it will
 * also be paid at the destination, but that is also expected.
 */
const schemaFromGlobalId = (id: string): GraphQLSchema | void => {
  try {
    // fromGlobalId is unsafe, and generally this is flaky
    switch (nodeResolvers[fromGlobalId(id).type]) {
      case "lloyd":
        return lloydSchema;
      case "local":
        return localSchema;
      default:
        throw new Error("UnknownService");
    }
  } catch (e) {
    // sentry, etc.
    throw IdentificationFailureError(); // catch and recover thread elsewhere
  }
};

/* ... */
        node: (parent, args, context, info) => {
          const schema = schemaFromGlobalId(args.id);
          return info.mergeInfo.delegateToSchema({
            schema,
            operation: "query",
            fieldName: "node",
            args: {
              id: args.id
            },
            context,
            info
          });
        }

Introducing additional base64 data into an already clunky (imho) system of word followed by colon followed by actual id seems rough around the egdes to me.

Another issue with encoding the service name as part of the nodeGlobalID is that it would have to be an adjustment to either typename itself or the id of the entity.

  1. If you look at how graphql-relay package resolves types from ID it is clear to me that I cannot prepend anything to the type: it is defined as "everything until the first colon". This would mean encoding the service name as part of type itself.
  2. Prepending the service name to all service-local IDs at the graphql endpoint seems plausible, and provides an additonal check of "don't care about this request cus that's not my name" — but at the same time if we sent it there in the first place why don't we terminate the mess of resolving types during stitching at the point where stitching occurs then working cleanly further upstream.

ermik commented Jun 5, 2018

There is a need for a contract that adheres to the graphql/relay spec. The node field itself holds the promise of identifying any object that implements the node interface by only it's ID. So when we look upstream, that same promise, for a given type, is the held by the particular service — never two at once (same as in case of eventual consistency needing a master of some sort). Any further linkage is well-served by service-to-service connections e.g. gRPC & Thrift which is how we can pick up the slack of resolving deeply nested/relational queries: get the user from "users" service, but also get his likes from the "likes" service can be resolved by the users service alone. (maybe an antipattern tho, don't know, but it seems to be very much like the illustration in Artsy tech blog) Correct me if I am mistaken, but afaik GraphQL is of no help here, as it doesn't resolve children until parent query comes back.

Based on these considerations I'd keep the decision of picking a schema it in a controlled format of a static map:

/** @flow */

import { fromGlobalId } from "graphql-relay";
import type { GraphQLSchema } from "graphql";

type ObjectType = string;
type Service = string;
type NodeMap = { [ObjectType]: Service };

const nodeResolvers: NodeMap = {
  ["Vendor"]: "lloyd",
  ["ElasticSearch"]: "local"
};

/**
 * @func schemaFromGlobalId retrieves delegate schema for a Relay Global ID
 * @param {string} id — Relay standard base64 encoded tuple of shape "type:id"
 * @desc instead of base64() cost if encoding servicename as part of type
 * we will pay expected base64() cost associated with Relay services; it will
 * also be paid at the destination, but that is also expected.
 */
const schemaFromGlobalId = (id: string): GraphQLSchema | void => {
  try {
    // fromGlobalId is unsafe, and generally this is flaky
    switch (nodeResolvers[fromGlobalId(id).type]) {
      case "lloyd":
        return lloydSchema;
      case "local":
        return localSchema;
      default:
        throw new Error("UnknownService");
    }
  } catch (e) {
    // sentry, etc.
    throw IdentificationFailureError(); // catch and recover thread elsewhere
  }
};

/* ... */
        node: (parent, args, context, info) => {
          const schema = schemaFromGlobalId(args.id);
          return info.mergeInfo.delegateToSchema({
            schema,
            operation: "query",
            fieldName: "node",
            args: {
              id: args.id
            },
            context,
            info
          });
        }

Introducing additional base64 data into an already clunky (imho) system of word followed by colon followed by actual id seems rough around the egdes to me.

Another issue with encoding the service name as part of the nodeGlobalID is that it would have to be an adjustment to either typename itself or the id of the entity.

  1. If you look at how graphql-relay package resolves types from ID it is clear to me that I cannot prepend anything to the type: it is defined as "everything until the first colon". This would mean encoding the service name as part of type itself.
  2. Prepending the service name to all service-local IDs at the graphql endpoint seems plausible, and provides an additonal check of "don't care about this request cus that's not my name" — but at the same time if we sent it there in the first place why don't we terminate the mess of resolving types during stitching at the point where stitching occurs then working cleanly further upstream.
@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Jun 5, 2018

Contributor

For Relay, the only requirement is that a Node ID is globally unique and that the GraphQL server can resolve back to a unique entity from it. A strong suggestion is to make this ID opaque to the client.

One way to make the ID opaque but encode the required data, as is used by the graphql-relay package, is to Base64 encode type:id, but it could really be anything. You don’t need to use the functions provided by graphql-relay. My thought for our stitched schema is that the upstream services encode service:type:id; then metaphysics doesn’t even need to decode those IDs when the client requests them, it can just match the first part of the encoded ID with pre-cached Base64 encoded versions of the services.

Contributor

alloy commented Jun 5, 2018

For Relay, the only requirement is that a Node ID is globally unique and that the GraphQL server can resolve back to a unique entity from it. A strong suggestion is to make this ID opaque to the client.

One way to make the ID opaque but encode the required data, as is used by the graphql-relay package, is to Base64 encode type:id, but it could really be anything. You don’t need to use the functions provided by graphql-relay. My thought for our stitched schema is that the upstream services encode service:type:id; then metaphysics doesn’t even need to decode those IDs when the client requests them, it can just match the first part of the encoded ID with pre-cached Base64 encoded versions of the services.

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