Log type conflics when inferring graphql types #3905

pieh · 2018-02-08T00:37:35Z

This adds logging discarded/omitted fields that have conflicting types.

Example of output:

success update schema — 0.152 s
warning There are conflicting field types in your data. GraphQL schema will omit those fields.
SitePage.context.previous:
 - boolean: false
 - object: { fields: [Object], frontmatter: [Object] }
SitePage.context.next:
 - boolean: false
 - object: { fields: [Object], frontmatter: [Object] }
MarkdownRemark.frontmatter.scalarTest:
 - boolean: true (File "src\pages\my-second-post\index.md")
 - number: 5 (File "src\pages\hi-folks\index.md")
 - string: 'string' (File "src\pages\hello-world\index.md")
MarkdownRemark.frontmatter.scalarObjectTest:
 - object: { prop: true, a3: true } (File "src\pages\hello-world\index.md")
 - string: 'string' (File "src\pages\my-second-post\index.md")
MarkdownRemark.frontmatter.objectArrayTest:
 - array<number>: [ 5 ] (File "src\pages\hello-world\index.md")
 - object: { prop: true } (File "src\pages\hi-folks\index.md")
MarkdownRemark.frontmatter.mixedArrayInSingleNodeTest:
 - array<number|string>: [ 5, ..., 'string', ... ] (File "src\pages\hello-world\index.md")
MarkdownRemark.frontmatter.mixedArrayInDifferentNodesTest:
 - array<number>: [ 5 ] (File "src\pages\hello-world\index.md")
 - array<string>: [ 'string' ] (File "src\pages\hi-folks\index.md")
MarkdownRemark.frontmatter.objectsInArray[].scalarTest:
 - boolean: true (File "src\pages\hello-world\index.md")
 - string: 'test' (File "src\pages\my-second-post\index.md")
success extract queries from components — 0.080 s

Above was generated using this demo - https://github.com/pieh/gatsby-conflicting-types-demo with prepared conflicts in frontmatter:

page 1 (hello-world - https://github.com/pieh/gatsby-conflicting-types-demo/blob/master/src/pages/hello-world/index.md ):

scalarTest: 'string'
scalarObjectTest:
  prop: true
  a3: true
objectArrayTest:
  - 5
mixedArrayInSingleNodeTest:
  - 5
  - 8
  - 9
  - 'string'
  - 'anotherString'
mixedArrayInDifferentNodesTest:
  - 5
objectsInArray:
  - scalarTest: true
    correct: true

page 2 (hi-folks - https://github.com/pieh/gatsby-conflicting-types-demo/blob/master/src/pages/hi-folks/index.md ):

scalarTest: 5
scalarObjectTest:
  prop: true
  a2: true
objectArrayTest:
  prop: true
mixedArrayInDifferentNodesTest:
  - 'string'

page 3 (my second post - https://github.com/pieh/gatsby-conflicting-types-demo/blob/master/src/pages/my-second-post/index.md ):

scalarTest: true
scalarObjectTest: 'string'
objectsInArray:
  - scalarTest: "test"
    correct: true

Closes #3856

Btw: funny side gain for finding conflicts in gatsby-starter-blog in passed context ( if anyone would ever want to query that :) ). Would need to use null instead of false here - https://github.com/gatsbyjs/gatsby-starter-blog/blob/master/gatsby-node.js#L39-L40

gatsbybot · 2018-02-08T00:43:03Z

Deploy preview for gatsbygram ready!

Built with commit 97a9953

https://deploy-preview-3905--gatsbygram.netlify.com

pieh · 2018-02-08T00:50:52Z

Will check those failing tests tomorrow

KyleAMathews · 2018-02-08T00:53:36Z

Woah nice!!

A couple of thoughts which you've probably thought of already but I'll throw out to document them:

it'd be cool to print out the actual conflicting data
When possible, print out the source of the data e.g. frontmatter on a markdown file — print out the file path to the file with the conflicting data. Or in case of remote data — this is harder (we might want to establish conventions for source plugins to create error strings e.g. IDs of a node) but we could say e.g. for Contentful that the data came from an object of ID X and in Space Y.

pieh · 2018-02-08T02:51:46Z

it'd be cool to print out the actual conflicting data

Had that in my initial version, just removed it thinking it would be too spammy with array values (in long arrays scenario). If I would reduce arrays to single items of given type then it would be more readable but probably also somewhat confusing

When possible, print out the source of the data e.g. frontmatter on a markdown file — print out the file path to the file with the conflicting data. Or in case of remote data — this is harder (we might want to establish conventions for source plugins to create error strings e.g. IDs of a node) but we could say e.g. for Contentful that the data came from an object of ID X and in Space Y.

I'd rather avoid adding special cases in core to show local markdown file even if it would be straight forward to do (getting parent File node and showing path to it). I would think that in general this would try finding nearest ancestor node (or self) that have origin (in whatever manner it is decided to be set).

I will experiment with some conventions tomorrow and hopefully plugin authors will voice their opinion on this topic. It doesn't need to be implemented in this PR - this can be done in follow up IMO.

Another topic - what's Your opinion about adding links in console output - I could add paragraph about type conflicts here https://www.gatsbyjs.org/docs/querying-with-graphql/#where-does-gatsbys-graphql-schema-come-from and add link to it in warning message but then we'd need to be sure this link will always be correct. We do this in couple of places (f.e. debugging html builds or queries not run in non page-or-layout components), but I don't know if that's the right way to add more context to warnings/errors.

KyleAMathews · 2018-02-08T03:01:18Z

what's your opinion about adding links in console output

I added the other links so I'm a big fan :-)

Ideally we link to a single page and chose the title wisely so the link is stable. If it's a single page, we can easily add / tweak content over time.

KyleAMathews · 2018-02-09T02:23:54Z

Deploy preview for using-glamor failed.

Built with commit f390267

https://app.netlify.com/sites/using-glamor/deploys/5a7d091e0b79b765d683cd48

pieh · 2018-02-10T14:24:08Z

Just note - I added data origin logging but it's bugged currently (it reports wrong sources). I will check if this is something I can fix with my current approach or will have to change it to something else.

pieh · 2018-02-15T20:57:48Z

I ended up rewriting extractFieldExamples completely as this was just very painful and hacky to get original value to display (instead of merged value) and track selector path.

It now will analyze values and report conflicts before doing any actual merging. It does have some performance hit (using freeCodeCamp guides website - it was consistently about +20% in 5 tests I run):

nodes count
 - SitePage - 2769
 - SitePlugin - 16
 - Site - 1
 - Directory - 2761
 - File - 2767
 - MarkdownRemark - 2764
current: 267.151ms
dev: 315.725ms

but it is offseted by fact that it now stores example value and extraction in not run 4 times for same nodes (for fields, for input fields, for sort field enums and for group enums). I will recheck that with example provided in #4055 later.

Examples will fail to build because I added origin field in node.internal and as it wasn't published it won't work ( 300961e ) and I still consider this RFC. For now I'm not worried much about it. I'd like to discuss what's actually good way to pass that information from plugin first.

gatsbybot · 2018-04-05T16:11:40Z

Deploy preview for using-drupal ready!

Built with commit 97a9953

https://deploy-preview-3905--using-drupal.netlify.com

pieh · 2018-04-05T16:36:13Z

Rebased this PR so it's mergable again

I've run gatsby build on master and this PR 5 times for gatsbyjs.org and it does decrease time spent on building schema (average times):

	master	this PR
building schema	1.591s	0.726s
update schema	1.408s	0.527s
total	2.999s	1.253s

…or fields, for input fields, for sort field enums and for group enums) Signed-off-by: Michal Piechowiak <[email protected]>

… it to findRootNodeAncestor Signed-off-by: Michal Piechowiak <[email protected]>

Signed-off-by: Michal Piechowiak <[email protected]>

…t type conflicts when creating graphql schema Signed-off-by: Michal Piechowiak <[email protected]>

…h and detect type conflicts early (to not merge values if there is conflict) Signed-off-by: Michal Piechowiak <[email protected]>

pieh · 2018-04-10T17:29:44Z

Anyone care to review?

KyleAMathews

Looks good!

I didn't see any tests for the conflicts? Or perhaps I just missed them.

Also does the output look similar still to your original PR comment? I think that could be clearer still.

KyleAMathews · 2018-04-11T01:03:02Z

packages/gatsby/src/redux/actions.js

@@ -505,6 +505,10 @@ const typeOwners = {}
 * @param {string} node.internal.contentDigest the digest for the content
 * of this node. Helps Gatsby avoid doing extra work on data that hasn't
 * changed.
+ * @param {string} node.internal.nodeDescription An optional field. Human


Maybe just name the field description? node is already implied.

Sure, will change that soon

KyleAMathews · 2018-04-11T01:12:59Z

packages/gatsby/src/schema/data-tree-utils.js

+  return { type, nodes }
+}
+
+const getExampleValue = arg => {


Any reason this isn't named getExampleValues?

I can change that, but we get single value (object) from array of nodes, so I though single value would be more accurate.

Yeah, I think of the object as a bag of values 🤷‍♂️

Looks like this is all that's left for this to get in?

pieh · 2018-04-11T13:02:05Z

Yes, tests for this functionality is missing right now, will add them

pieh · 2018-04-11T14:10:14Z

Output in PR description is what currently looks like - i feel this a little bit more readible in actual terminal:

Just did testing with more spaced out information:

might be worth adding some more colouring to seprate things, but I worry this would cause problems with readability in custom styled terminals people seems to like to use

Signed-off-by: Michal Piechowiak <[email protected]>

KyleAMathews · 2018-04-11T19:26:51Z

Cool yeah — and generally people will only have one conflicting type so it should be less overwhelming for folks.

We should double-check the official starters don't have any conflicting types before this gets merged too :-)

Signed-off-by: Michal Piechowiak <[email protected]>

pieh · 2018-04-11T21:20:15Z

I will run this through official starters, but what about gatsbyjs.org: 😄

I was just thinking now how to handle possible conflicts in SitePlugin nodes - not sure if anyone is using those in graphql, but pluginOptions could generate some conflicts if plugins will have options with same names and different types - should we silence SitePlugin conflicts?

Signed-off-by: Michal Piechowiak <[email protected]>

pieh · 2018-04-12T14:29:48Z

Updating starters right now and as I suspected - this might be problematic with SitePlugin:

warning There are conflicting field types in your data. GraphQL schema will omit those fields.
SitePlugin.packageJson.author:
 - type: object
   value: { name: 'Kyle Mathews', email: '[email protected]' }
 - type: string
   value: 'Kyle Mathews <[email protected]>'

I think silencing SitePlugin conflicts seems reasonable

Signed-off-by: Michal Piechowiak <[email protected]>

…rently examples are not stored because type is not set - this is just future proofing Signed-off-by: Michal Piechowiak <[email protected]>

Signed-off-by: Michal Piechowiak <[email protected]>

KyleAMathews · 2018-04-14T23:43:05Z

Sweeet! Let's publish this!

pieh · 2018-04-15T00:06:39Z

We should also merge this ( gatsby-starter-blog small adjustments to prevent same conflict as in this screenshot - #3905 (comment) ) - gatsbyjs/gatsby-starter-blog#97 not sure why travis fail at git checkout there

rdela · 2018-08-28T01:19:05Z

Would need to use null instead of false here - gatsbyjs/gatsby-starter-blog:gatsby-node.js@master#L39-L40

man thank heavens for this comment @pieh that was driving me crazy til I found this!
rdela/rdela.com@9914f7f#diff-fda05457e393bada716f508859bfc604L74

* add nodeDescription field to node internal object Signed-off-by: Michal Piechowiak <[email protected]> * store extracted example value to not run it 4 times for same nodes (for fields, for input fields, for sort field enums and for group enums) Signed-off-by: Michal Piechowiak <[email protected]> * introduce TypeConflictReporter utility class that will store and print type conflicts when creating graphql schema Signed-off-by: Michal Piechowiak <[email protected]> * enhance findRootNode function to accept predicate function and rename it to findRootNodeAncestor Signed-off-by: Michal Piechowiak <[email protected]> * rewrite extractFieldExamples to be able to keep track of selector path and detect type conflicts early (to not merge values if there is conflict) Signed-off-by: Michal Piechowiak <[email protected]> * use type conflict reporter when extracting example value Signed-off-by: Michal Piechowiak <[email protected]> * add nested arrays in array of objects to tests Signed-off-by: Michal Piechowiak <[email protected]> * rename nodeDescription to description Signed-off-by: Michal Piechowiak <[email protected]> * add tests for conflict reporting Signed-off-by: Michal Piechowiak <[email protected]> * space out conflict info to be more readable Signed-off-by: Michal Piechowiak <[email protected]> * add description for possible conflicts in page context Signed-off-by: Michal Piechowiak <[email protected]> * fix schema type conflict in gatsbyjs.org Signed-off-by: Michal Piechowiak <[email protected]> * Silence SitePlugin type conflicts Signed-off-by: Michal Piechowiak <[email protected]> * just in case clear type examples store in data-tree-utils tests - currently examples are not stored because type is not set - this is just future proofing Signed-off-by: Michal Piechowiak <[email protected]> * rename getExampleValue to getExampleValues Signed-off-by: Michal Piechowiak <[email protected]>

ghost assigned pieh Feb 8, 2018

ghost added the review label Feb 8, 2018

pieh mentioned this pull request Feb 8, 2018

Sparsely populated fields in sources can be omitted from the graphql schema #3913

Closed

pieh force-pushed the infer_conflict_type_log branch from fddc563 to 302682d Compare February 9, 2018 02:21

pieh force-pushed the infer_conflict_type_log branch 2 times, most recently from 4a77478 to d36997c Compare February 10, 2018 01:18

pieh mentioned this pull request Feb 10, 2018

When inferring types for the graphql schema, log when we discard a field due to conflicting types #3856

Closed

pieh force-pushed the infer_conflict_type_log branch 3 times, most recently from 620721a to eab0355 Compare February 15, 2018 19:55

pieh force-pushed the infer_conflict_type_log branch 2 times, most recently from ee447d2 to 8045728 Compare February 20, 2018 10:53

pieh mentioned this pull request Feb 27, 2018

How to identify why GraphQL fields are missing? #4271

Closed

pieh force-pushed the infer_conflict_type_log branch from 8045728 to 6580cfa Compare April 5, 2018 16:03

pieh force-pushed the infer_conflict_type_log branch from 6580cfa to ec6e929 Compare April 5, 2018 20:40

pieh added 5 commits April 10, 2018 17:39

store extracted example value to not run it 4 times for same nodes (f…

bf6f16b

…or fields, for input fields, for sort field enums and for group enums) Signed-off-by: Michal Piechowiak <[email protected]>

enhance findRootNode function to accept predicate function and rename…

a5853e7

… it to findRootNodeAncestor Signed-off-by: Michal Piechowiak <[email protected]>

add nodeDescription field to node internal object

e6aa7eb

Signed-off-by: Michal Piechowiak <[email protected]>

introduce TypeConflictReporter utility class that will store and prin…

e7a4711

…t type conflicts when creating graphql schema Signed-off-by: Michal Piechowiak <[email protected]>

rewrite extractFieldExamples to be able to keep track of selector pat…

151840b

…h and detect type conflicts early (to not merge values if there is conflict) Signed-off-by: Michal Piechowiak <[email protected]>

pieh force-pushed the infer_conflict_type_log branch from ec6e929 to 17ae80a Compare April 10, 2018 15:40

KyleAMathews suggested changes Apr 11, 2018

View reviewed changes

rename nodeDescription to description

f19be9f

Signed-off-by: Michal Piechowiak <[email protected]>

pieh added 3 commits April 11, 2018 22:02

add tests for conflict reporting

620a68c

Signed-off-by: Michal Piechowiak <[email protected]>

space out conflict info to be more readable

9bc77d5

Signed-off-by: Michal Piechowiak <[email protected]>

add description for possible conflicts in page context

71e7569

Signed-off-by: Michal Piechowiak <[email protected]>

fix schema type conflict in gatsbyjs.org

802b2ec

Signed-off-by: Michal Piechowiak <[email protected]>

pieh mentioned this pull request Apr 12, 2018

fix schema type conflicts gatsbyjs/gatsby-starter-blog#97

Merged

Silence SitePlugin type conflicts

97fd886

Signed-off-by: Michal Piechowiak <[email protected]>

m-allanson removed the review label Apr 13, 2018

m-allanson mentioned this pull request Apr 13, 2018

Output warning the graphql inference process drops fields due to type mis-matches #2405

Closed

pieh added 2 commits April 15, 2018 01:24

just in case clear type examples store in data-tree-utils tests - cur…

a6d2311

…rently examples are not stored because type is not set - this is just future proofing Signed-off-by: Michal Piechowiak <[email protected]>

rename getExampleValue to getExampleValues

97a9953

Signed-off-by: Michal Piechowiak <[email protected]>

pieh changed the title ~~[rfc] Log type conflics when inferring graphql types~~ Log type conflics when inferring graphql types Apr 14, 2018

KyleAMathews approved these changes Apr 14, 2018

View reviewed changes

KyleAMathews merged commit 5b1dc1c into gatsbyjs:master Apr 14, 2018

This was referenced May 31, 2018

Don't infer types for plugin provided GQL types #5497

Merged

schema: fix not preferring floats over ints in some situations #5654

Merged

henricazottes mentioned this pull request Mar 6, 2020

How to display source file for conflicting fields types? #22016

Closed

tejzpr mentioned this pull request Feb 7, 2024

[Snyk] Fix for 17 vulnerabilities tejzpr/gatsby#63

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Log type conflics when inferring graphql types #3905

Log type conflics when inferring graphql types #3905

pieh commented Feb 8, 2018 •

edited

Loading

gatsbybot commented Feb 8, 2018 •

edited

Loading

pieh commented Feb 8, 2018

KyleAMathews commented Feb 8, 2018

pieh commented Feb 8, 2018 •

edited

Loading

KyleAMathews commented Feb 8, 2018

KyleAMathews commented Feb 9, 2018 •

edited

Loading

pieh commented Feb 10, 2018

pieh commented Feb 15, 2018 •

edited

Loading

gatsbybot commented Apr 5, 2018 •

edited

Loading

pieh commented Apr 5, 2018

pieh commented Apr 10, 2018

KyleAMathews left a comment

KyleAMathews Apr 11, 2018

pieh Apr 11, 2018

KyleAMathews Apr 11, 2018

pieh Apr 11, 2018

KyleAMathews Apr 11, 2018

KyleAMathews Apr 14, 2018

pieh commented Apr 11, 2018

pieh commented Apr 11, 2018

KyleAMathews commented Apr 11, 2018

pieh commented Apr 11, 2018

pieh commented Apr 12, 2018

KyleAMathews commented Apr 14, 2018

pieh commented Apr 15, 2018

rdela commented Aug 28, 2018 •

edited

Loading

Log type conflics when inferring graphql types #3905

Log type conflics when inferring graphql types #3905

Conversation

pieh commented Feb 8, 2018 • edited Loading

gatsbybot commented Feb 8, 2018 • edited Loading

pieh commented Feb 8, 2018

KyleAMathews commented Feb 8, 2018

pieh commented Feb 8, 2018 • edited Loading

KyleAMathews commented Feb 8, 2018

KyleAMathews commented Feb 9, 2018 • edited Loading

pieh commented Feb 10, 2018

pieh commented Feb 15, 2018 • edited Loading

gatsbybot commented Apr 5, 2018 • edited Loading

pieh commented Apr 5, 2018

pieh commented Apr 10, 2018

KyleAMathews left a comment

Choose a reason for hiding this comment

KyleAMathews Apr 11, 2018

Choose a reason for hiding this comment

pieh Apr 11, 2018

Choose a reason for hiding this comment

KyleAMathews Apr 11, 2018

Choose a reason for hiding this comment

pieh Apr 11, 2018

Choose a reason for hiding this comment

KyleAMathews Apr 11, 2018

Choose a reason for hiding this comment

KyleAMathews Apr 14, 2018

Choose a reason for hiding this comment

pieh commented Apr 11, 2018

pieh commented Apr 11, 2018

KyleAMathews commented Apr 11, 2018

pieh commented Apr 11, 2018

pieh commented Apr 12, 2018

KyleAMathews commented Apr 14, 2018

pieh commented Apr 15, 2018

rdela commented Aug 28, 2018 • edited Loading

pieh commented Feb 8, 2018 •

edited

Loading

gatsbybot commented Feb 8, 2018 •

edited

Loading

pieh commented Feb 8, 2018 •

edited

Loading

KyleAMathews commented Feb 9, 2018 •

edited

Loading

pieh commented Feb 15, 2018 •

edited

Loading

gatsbybot commented Apr 5, 2018 •

edited

Loading

rdela commented Aug 28, 2018 •

edited

Loading