Schema types fix #658

stevelacey · 2020-07-09T11:26:10Z

Summary

Specifying additional types per schema is broken, this fixes that and updates the tests to exercise the problem.

Type of change

Bug fix (non-breaking change which fixes an issue)

Checklist

Code style has been fixed via composer fix-style
Tests updated

mfn · 2020-07-09T16:01:06Z

That rips out the heart of the lazy type loader (see #405 ) and tests are breaking.

See also the discussion in #390 (comment) regarding the performance improvements

It's quite possible it works in your project (and probably would also in mine), but we wouldn't reap the benefits of lazy loading on larger schemas when not all types are necessary for a given request (think about a schema with 100 types and 100 queries but any given request only requires a fraction of them).

stevelacey · 2020-07-10T06:29:33Z

@mfn how about this? Seems to work and the tests pass.

mfn · 2020-07-12T04:36:07Z

Before taking a deeper look I decided to re-test the benchmarks from #390 (comment) and #390 (comment)

I can report back that, although the absolute numbers changed, the relative improvement is also still there 👍

For this to work I only applied the diff from this PR manually as this uses an older version and I wasn't keen yet do upgrade this.

lazyload_types=false => ~730ms
lazyload_types=true => ~530ms

I couldn't see any difference between the existing lazyload_types and from this PR 👍

@stevelacey since this PR supposedly fixes something 😏 can you please add a test showing this? A test I expect would not work without this change. Thanks!

stevelacey · 2020-07-12T08:59:00Z

@mfn awesome, good to know. I've updated the tests, they now fail on the previous commit with CustomExample type not found, which is what is fixed here 🤘

mfn · 2020-07-12T11:47:39Z

src/GraphQL.php

@@ -70,6 +70,9 @@ public function schema($schema = null): Schema
        $schemaQuery = $schema['query'] ?? [];
        $schemaMutation = $schema['mutation'] ?? [];
        $schemaSubscription = $schema['subscription'] ?? [];
+        $schemaTypes = $schema['types'] ?? [];
+
+        $this->addTypes($schemaTypes);


This one is interesting.

Every time a query is now run, it adds the schema types to the internal types list.

Previously this was only done once when constructing the GraphQL instance.

Multiple invocation within the same process of the same query will work correctly, as types are stored by their name.

Now, if multiple schemas with same named types are used... they will overwrite previous one but probably "just" work. I guess.

It's in fact similar to typesInstances with the difference that they're cleared before every query, so instances of different schemas are never mixed.

I still have to think about the impact here but if you've any insights here, please share them!

@mfn the thing is this whole mechanism seems to rely on php's request lifetime already, and I've not addressed that problem to reach my goal here – really we should do better, but it looks like a significant piece of work to get there I think. The GraphQL::type(...) helper would need to become schema-aware, and know to look within a separate pool of independently built/cached types depending on the current schema in use.

I figured the tests would exercise this problem and break for this reason, but they didn't, probably because even when >1 queries are executed within the same test, they're happening against the same schema, the same as in a request. Since a request can only be performed against one schema at a time, I figured this solution would probably do for now, but really I'd prefer this scenario was handled better.

One concern I had was regarding "always adding" and the memory consumption, i.e.:

this whole mechanism seems to rely on php's request lifetime already

I've stress tested with workers and horizon: single worker, dispatching about a million jobs which just perform GraphQL queries. Whilst a saw the memory going up over time:

it was very slowly (a few kb every minute)

it can't vouch that other (unrelated) code might have a leak -> I tested this in a real project

I never had issues in the past with workers so I can't even say if they naturally grow a bit over time or not, i.e. never did I look closely before.

So for now my feeling is: this seems to work!

mfn · 2020-07-12T11:53:47Z

Ah, one more thing I noticed: the readme from the beginning did give examples of multiple schemas, i.e. starts off with the 'default' schema etc.

So the best practice for defining queries/mutation is in fact per schema.

Interestingly, for the types it was never. We're basically promoting global types.

That's also emphasized by the default config we publish: there is even a types key there to begin with!

I wonder if this general sentiment should be changed, to focus on the whole "per schema" experience and only use global types for specific cases (something like the UploadType or probably any custom scalar types).

Any feeling about this?

stevelacey · 2020-07-12T12:48:18Z

Ah, one more thing I noticed: the readme from the beginning did give examples of multiple schemas, i.e. starts off with the 'default' schema etc.

So the best practice for defining queries/mutation is in fact per schema.

Interestingly, for the types it was never. We're basically promoting global types.

That's also emphasized by the default config we publish: there is even a types key there to begin with!

I wonder if this general sentiment should be changed, to focus on the whole "per schema" experience and only use global types for specific cases (something like the UploadType or probably any custom scalar types).

Any feeling about this?

Yeah, I agree, globally defined types shouldn't be the recommendation, in the project I've been working on it's resulted in 8 schema all with types half of them don't use. The most common reason for even having multiple schema is probably to differentiate public and private APIs, which likely have significant differences in their type offerings. In my case, only 1 of these 8 APIs is a public API, and it shouldn't receive the types used exclusively by the private APIs, nor populate the new public schema I'm adding with types that I've deprecated. I am able to move 20 unused input types out of the new schema with this change + move a few obscure types in to the private schema that exclusively use them.

Another thing worth noting is that sharing types between schema isn't hard even without a global types pool, encouraging users to define everything per schema and deprecate the feature entirely might be the way to go, it'll probably raise the quality of GraphQL APIs produced with this library – especially when you consider the tools like Graphidocs and GraphQL Playground that build documentation out of these type definitions.

mfn · 2021-03-30T15:00:37Z

@crissi in case you're still around 😅 since you're the original author of the improved type loader, do you think you've time for a review?

My concern from #658 (comment) isn't addressed yet, but I also found no good solution ATM.

The PR isn't yet finished in terms of tests, but I tested it successfully on a private project with per schema-types and also per-schema types mixed with global types etc. and everything worked out.

mfn · 2021-03-30T19:21:48Z

I've added some tests for these cases. They did show me that when I was running it against master branch, they broke. But they worked in this branch 🎉

crissi · 2021-03-30T19:37:57Z

@crissi in case you're still around 😅 since you're the original author of the improved type loader, do you think you've time for a review?

My concern from #658 (comment) isn't addressed yet, but I also found no good solution ATM.

The PR isn't yet finished in terms of tests, but I tested it successfully on a private project with per schema-types and also per-schema types mixed with global types etc. and everything worked out.

Still around:) I can try to test the branch tomorrow with some production code and see if it breaks anything or slows execution down significantly

mfn · 2021-03-30T19:40:02Z

@crissi that would be great!

The PR isn't yet finished in terms of tests,

PS: done from my side!

crissi · 2021-03-31T13:18:51Z

I ran on my project and my tests run succesfully and It does not seem to affect performance 👍

mfn · 2021-03-31T13:54:19Z

@crissi thanks, same experience here!

I'll probably maybe merge and release this later today.

mfn · 2021-03-31T22:18:07Z

Almost forgot: @stevelacey thanks for the PR!

stevelacey · 2021-04-01T02:47:36Z

Nice to see this finally merge! @Bradez

stevelacey force-pushed the patch-1 branch from 5abdbba to f081288 Compare July 9, 2020 11:36

stevelacey force-pushed the patch-1 branch 2 times, most recently from 7434d7e to 628ce80 Compare July 10, 2020 06:24

stevelacey force-pushed the patch-1 branch 2 times, most recently from 6a19f7f to c975ecb Compare July 10, 2020 09:50

stevelacey force-pushed the patch-1 branch from c975ecb to bb2ef74 Compare July 12, 2020 08:56

stevelacey force-pushed the patch-1 branch from bb2ef74 to febc653 Compare July 12, 2020 10:37

mfn reviewed Jul 12, 2020

View reviewed changes

mfn mentioned this pull request Aug 21, 2020

Dynamic, data-driven schemas and types based on route params #667

Closed

mfn mentioned this pull request Dec 6, 2020

Basic Automatic Persisted Queries (APQ) support #701

Merged

6 tasks

mfn mentioned this pull request Feb 15, 2021

Namespaced type #714

Closed

Schema types fix

fec4c53

mfn force-pushed the patch-1 branch from febc653 to fec4c53 Compare March 30, 2021 14:23

mfn self-assigned this Mar 30, 2021

mfn added 4 commits March 30, 2021 17:43

readme: promote per-schema types

028ba04

Update changelog

df6e84b

Add tests showing how per-schema types work

80906b3

composer phpstan-baseline

fec1c5a

mfn approved these changes Mar 30, 2021

View reviewed changes

mfn merged commit 56d0b7a into rebing:master Mar 31, 2021

stevelacey deleted the patch-1 branch April 1, 2021 02:46

mfn mentioned this pull request Apr 5, 2021

Schema specific fields #603

Closed

mfn mentioned this pull request May 15, 2021

Schema and type handling #778

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Schema types fix #658

Schema types fix #658

stevelacey commented Jul 9, 2020 •

edited

Loading

mfn commented Jul 9, 2020

stevelacey commented Jul 10, 2020

mfn commented Jul 12, 2020

stevelacey commented Jul 12, 2020

mfn Jul 12, 2020

stevelacey Jul 12, 2020

mfn Mar 30, 2021

mfn commented Jul 12, 2020

stevelacey commented Jul 12, 2020 •

edited

Loading

mfn commented Mar 30, 2021

mfn commented Mar 30, 2021 •

edited

Loading

crissi commented Mar 30, 2021

mfn commented Mar 30, 2021

crissi commented Mar 31, 2021

mfn commented Mar 31, 2021

mfn commented Mar 31, 2021

stevelacey commented Apr 1, 2021

Schema types fix #658

Schema types fix #658

Conversation

stevelacey commented Jul 9, 2020 • edited Loading

Summary

Type of change

Checklist

mfn commented Jul 9, 2020

stevelacey commented Jul 10, 2020

mfn commented Jul 12, 2020

stevelacey commented Jul 12, 2020

mfn Jul 12, 2020

Choose a reason for hiding this comment

stevelacey Jul 12, 2020

Choose a reason for hiding this comment

mfn Mar 30, 2021

Choose a reason for hiding this comment

mfn commented Jul 12, 2020

stevelacey commented Jul 12, 2020 • edited Loading

mfn commented Mar 30, 2021

mfn commented Mar 30, 2021 • edited Loading

crissi commented Mar 30, 2021

mfn commented Mar 30, 2021

crissi commented Mar 31, 2021

mfn commented Mar 31, 2021

mfn commented Mar 31, 2021

stevelacey commented Apr 1, 2021

stevelacey commented Jul 9, 2020 •

edited

Loading

stevelacey commented Jul 12, 2020 •

edited

Loading

mfn commented Mar 30, 2021 •

edited

Loading