silverstripe / silverstripe-graphql

Serves Silverstripe data as GraphQL representations
BSD 3-Clause "New" or "Revised" License
52 stars 61 forks source link

Graphql 3.x-dev compatibility with upcoming CWP 2.8 #376

Closed emteknetnz closed 3 years ago

emteknetnz commented 3 years ago

Installing kitchen-sink 2.x-dev with graphql 3.x-dev

File manager:

image

Installing kitchen-sink 2.7.1 and manually using graphql 3.x-dev instead of 3.4.1

image

Some debugging notes:

- Manager:408
- UnionScaffolder:80 - $this->type has CWPBasePage
- $_name = SilverStripeSiteTreeWithDescendants (for both exc's)
- UnionScaffolder:36
- InheritanceScaffolder:118
- :127 $tree[2] = 'CWP\CWP\PageTypes\BasePage' ($class)
- StaticSchema:109
- :112 $customTypeName = $this->mappedTypeName($class); - null
- :118 $typeName = 'CWPBasePage'
- $parts = explode('\\', $class)
    array (
        0 => 'CWP',
        1 => 'CWP',
        2 => 'PageTypes',
        3 => 'BasePage',
    )
    $typeName = sizeof($parts) > 1 // (true)
      ? $parts[0] . end($parts) // <<< CWPBasePage
      : $parts[0];

Possibly this commit? https://github.com/silverstripe/silverstripe-graphql/commit/25f654c9fd33327b6107158868a636a6090d4d13#diff-b60efc42f855dbb4c758a6216ded97c98c345b5606e1820014afc5e174ebf473

emteknetnz commented 3 years ago

I'm not sure if this is the greatest solution, though it does stop asset-admin from complaining about graphql types

myproject.yml

SilverStripe\GraphQL\Manager:
  schemas:
    admin:
      typeNames:
        CWP\CWP\PageTypes\BasePage: Page

Trying to use CWP\CWP\PageTypes\BasePage: BasePage results in the error "Type 'BasePage' is not a registered GraphQL type"

My understanding is that Page will usually inherit BasePage on CWP projects, so this seems near enough

This config is really just to get graphql to not complain. I don't think there are any real world use case where graphql is used to read BasePage's specifically

We'd add this config to the cwp/cwp module where BasePage.php resides

maxime-rainville commented 3 years ago

Maybe this is an OK fix, but I would rather try to understand what change in-between CWP 2.7 an CWP 2.8 that this error propped up.

emteknetnz commented 3 years ago

I don't think anything changed in CWP. It looks like the change happened in graphql. I've provided a summary of my understanding of the graphql3 changes here. I've linked to a commit in the description of this issue where I think things changed, though I don't exactly what in there caused the CWP incompatibility.

unclecheese commented 3 years ago

OK, have found the issue. It's the forward compat commit as @emteknetnz identified. The issue is that, to maintain compliance with graphql 4, you can now do nodes { fieldName } as a functional equivalent to edges { node { fieldName } }. The latter is a verbose standard used for cursor-based pagination, which we're never going to have. Many GraphQL APIs support both options as we do in GraphQL 4. The way things are built in Graphql 3 means that this results in a type getting created twice -- one for each implementation of the pagination.

Fix is pretty straightforward. We just need to persist the result of getConnectionType before returning the connection. Or we could make getEdgeType take an argument for Type $connectionType. But that's a public method, so we'd need to make it optional to not break the API, in which case we'd have to fall back on creating it, which seems like way too much complexity.

I reckon in toType, we just persist the connection type to local state at that point, and then update getConnectionType to prefer that state. Half a day's effort at most.

unclecheese commented 3 years ago

https://github.com/silverstripe/silverstripe-graphql/pull/378

unclecheese commented 3 years ago

https://github.com/silverstripe/silverstripe-cms/pull/2651

emteknetnz commented 3 years ago

Great thanks for being responsive getting this fixed Aaron, I'll look to test and merge these shortly

emteknetnz commented 3 years ago

@unclecheese - Worked fine when running kitchen sink 2.x-dev, however I got this when running kitchen sink 2.7.1 with a composer file as below. Scenario where this becomes relevant is non-recipe cwp projects with a graphql:^3 requirement (e.g. asset-admin 1.7.1 requites graphql:^3)

As is, this kind of implies that we still need the conflict solution to prevent older versions from upgrading to graphql 3.5

        "cwp/cwp-installer": "2.7.1",
        "cwp/agency-extensions": "^2", (all other kitchen-sink requirements are ^carets)
...
        "silverstripe/graphql": "dev-pulls/3/beta-blocker as 3.4.1",
        "silverstripe/cms": "dev-pulls/4/beta-blocker as 4.7.1",

When navigating to the file manager:

image

emteknetnz commented 3 years ago

@unclecheese Just chatted with Max, we've decided we don't need to fix this for 2.7.1, instead we'll still use the conflict solution to manage older versions. Your fix solves this for 2.x-dev / 2.8.0 so we're good to go there.

So seems like there's nothing further you need to do here. Cheers for your help!

emteknetnz commented 3 years ago

Just to confirm that we do not need the yml solution first proposed in a comment above. Aaron's fix is a better solution so we'll go with that instead.