Non string values making it to isClientID breaking isomorphic relay implementation

[jeromecovington]

I am working on injecting prepared data via isomorphic-relay...see this issue for further details.

However, some properties of the graphql response have a value of null, and they seem to be getting passed off to isClientID as empty objects, and raising Uncaught TypeError: dataID.substring is not a function

Should JSON be sanitized before being passed onto isClientID, or should the function return if the argument passed to it is not a string?

[josephsavona]

Relay treats null records as null and doesn't attempt to traverse into them or create IDs for them. Given that, if you're seeing empty objects passed to isClientID then the problem could be in isomorphic-relay or how your app is configured. Can you share a commit preview or gist of your isomorphic-relay setup? If that's tricky, where is isClientID called with the invalid value? (Stack trace would help).

[jeromecovington]

I don't think it is how my app is configured, as other graphql responses can be both prepared and injected without difficulty, it is one type of response only (again with some null values) that ends up raising the error from isClientID.

Here is a stack trace:

Uncaught TypeError: dataID.substring is not a function
isClientID @ app.js:124159
getPath @ app.js:130435
_writeLink @ app.js:131496
visitField @ app.js:131243
visit @ app.js:130972
(anonymous function) @ app.js:130985
traverseChildren @ app.js:130994
traverse @ app.js:130984
visitFragment @ app.js:131212
visit @ app.js:130974
(anonymous function) @ app.js:130985
traverseChildren @ app.js:130994
traverse @ app.js:130984
(anonymous function) @ app.js:131455
_writePluralLink @ app.js:131437
visitField @ app.js:131241
visit @ app.js:130972
(anonymous function) @ app.js:130985
traverseChildren @ app.js:130994
traverse @ app.js:130984
(anonymous function) @ app.js:131455
_writePluralLink @ app.js:131437
visitField @ app.js:131241
visit @ app.js:130972
(anonymous function) @ app.js:130985
traverseChildren @ app.js:130994
traverse @ app.js:130984
_writeLink @ app.js:131507
visitField @ app.js:131243
visit @ app.js:130972
(anonymous function) @ app.js:130985
traverseChildren @ app.js:130994
traverse @ app.js:130984
visitFragment @ app.js:131212
visit @ app.js:130974
(anonymous function) @ app.js:130985
traverseChildren @ app.js:130994
traverse @ app.js:130984
visitRoot @ app.js:131197
visit @ app.js:130976
writePayload @ app.js:131120
(anonymous function) @ app.js:138437
writeRelayQueryPayload @ app.js:138433
instrumentedCallback @ app.js:128672
handleQueryPayload @ app.js:133422
instrumentedCallback @ app.js:128672
(anonymous function) @ app.js:77609
injectPreparedData @ app.js:77603
componentWillMount @ app.js:14233

[jeromecovington]

"Relay treats null records as null and doesn't attempt to traverse into them or create IDs for them."

Does relay also treat {} records the same way (not traversing, creating IDs, etc.)? Because somewhere along the way, the null records are getting changed to {}.

[josephsavona]

Relay treats empty objects as records, and synthesizes a client ID for them if id is not set. We haven't heard of this error with regular Relay, hence my suspicion that this has to do with isomorphic-relay's injectPreparedData method.

Can you paste (or link to a gist) with the query text and payload that is failing? Which record fails?

[jeromecovington]

Here is an example of the kind of response from graphql that ends up being correlated to the TypeError in isClientID, as well as a log of the values coming through as the dataID argument. This is before it goes into injectPreparedData. As far as exactly which record fails...I'm not sure how best to pinpoint that at the moment.

[jeromecovington]

Perhaps this is more helpful, the query + result that is passed to injectPreparedData after having passed through prepareData.

[josephsavona]

In the second gist there are multiple records that have id: {} - which means that either invalid data is being returned by the server - IDs should be strings - or prepareData is somehow transforming it incorrectly. I noticed that in the first paste the results do not have these malformed IDs, which also suggests that it is the transform step. Can you confirm that the IDs are correct prior to the transform step? It might be worth digging into isomorphic- relay to see where the ID objects are being generated (or maybe file an issue on that repo and link it here?)

Cc @denvned

[denvned]

or prepareData is somehow transforming it incorrectly.

@josephsavona, prepareData just intercepts calls to RelayStoreData#handleQueryPayload and convert the query to a plain object using toGraphQL.Query from Relay (as you suggested here):
https://github.com/denvned/isomorphic-relay/blob/ac2ce85d48be592f7d735c1849b762f10b3ad72d/src/prepareData.js#L15-L16

[denvned]

Also prepareData does not transform the result. So, invalid data returned by the GraphQL server seems more probable. It might also be transformed by a custom network layer (@jeromecovington do you use any?).

@jeromecovington what is the GraphQL schema for Image?

[jeromecovington]

@denvned - The only customization to our network layer allows for sending the request to graphql via get rather than post (for caching reasons). Although I did notice this issue prior to that customization.

The schema for Image is here.

[denvned]

@jeromecovington Looks like your GraphQL server at least sometimes returns an empty object {} as id of Image, although its type according to your schema should be scalar ID.

[jeromecovington]

@denvned Yeah there may be issues with how Image is being handled in the data transform layer. Let me sort that out and then we'll see. I do appreciate you and @josephsavona taking the time to help me step through what may have ultimately turned out to be an internal data issue!

[jeromecovington]

@denvned I am (seeing)[https://gist.github.com/jeromecovington/4a2058b5f8c18b96a9ef] the id field of Image to be optional (not GraphQLNonNull), should we not be able to query against type Image without asking for or needing an id? In this query I am not asking for the id, yet am still bumping up against the TypeError in isClientID.

[denvned]

@jeromecovington We can query against Image without asking for id even if it was GraphQLNonNull. I can't tell why Relay asks it in your case. Anyway the main problem here is not in Relay, but in your GraphQL server.

[jeromecovington]

Hm. Although if I issue this request in graphiql, in the response I do not get any null or {} values for id on photosTout (or type Image), in fact I do not even ask for the id and so get no key/value for that at all...this is the identical query issued in the container which is leading to the TypeError. Should I be able to trust that the response in graphiql is the exact response that relay sees?

{
  allBreakers(termSlug: "example-term", taxSlug: "example-taxonomy") {
    hero {
      breakerType,
      ctaTitle,
      ctaURL,
      seasonSlug,
      title,
      promotedItem {
        ... on PromotedContent {
          title,
          photosTout {
            title,
            url
          },
          url,
          cneID,
          channel {
            slug,
            url
          }
        }
        ... on Video {
          id,
          title,
          photosTout {
            title,
            url
          }
        }
      }
    }
    feed {
      breakerType,
      ctaTitle,
      ctaURL,
      seasonSlug,
      title,
      promotedItem {
        ... on PromotedContent {
          title,
          photosTout {
            title,
            url
          },
          url,
          cneID,
          channel {
            slug,
            url
          }
        }
        ... on Video {
          id,
          title,
          photosTout {
            title,
            url
          }
        }
      }
    }
  }
}

[josephsavona]

Relay automatically queries for id if this field exists on a type. If your type does not always have an id, make sure that this is reflected in its type: it should not implement the Node interface, id should have type of nullable ID, and it should return null for id on records that don't have an id.

[wincent]

Going to close this due to inactivity, as I don't think there is anything actionable remaining here. Thanks for filing the report!