silverstripe / silverstripe-graphql

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

"On demand" schema generation doesn't occur if schema has changed #459

Closed maxime-rainville closed 2 years ago

maxime-rainville commented 2 years ago

Problem

If there's a pre-existing schema built and your schema definition changes, on-demand build won't notice and rebuild itself.

Steps to reproduce

Expected results: Query should complain that the boom field doesn't exist. Actual results: Query succeeds.

---
Name: myppojectgraphql
After: '#graphqlconfig'
---
SilverStripe\Control\Director:
  rules:
    'graphql': '%$SilverStripe\GraphQL\Controller.default'

SilverStripe\GraphQL\Schema\Schema:
  schemas:
    default:
      models:
        Dummy:
          operations: '*'
          fields:
            id: true
            boom: true
{
  readDummies {
    nodes {
      id
      boom
    }
  }
}
GuySartorelli commented 2 years ago

I think fixing this would probably require a level of introspection that makes the pre-built schema almost pointless. Isn't the point of building the schema so that we don't need to parse, validate, etc the schema definition for every request?

I think this is probably a matter of documenting it. The solution imo is for developers to invalidate (e.g. via that clear task from graphql Dev) the schema when they update the schema definition if they are relying solely on on-demand builds.

That said I don't have a whole lot of knowledge around this module so it's possible I'm off base here.

kinglozzer commented 2 years ago

I thought this functionality was intended to automatically build the schema when it’s missing, not rebuild it on changes?

GuySartorelli commented 2 years ago

@maxime-rainville Do you still think this is a problem? Or can we accept that this is a necessary limitation of on-demand schema building which we've covered in the docs about building and deploying schema?

unclecheese commented 2 years ago

Yeah, let's not think of the on-demand schema generation as a feature -- it's a safety net. It's for edge cases like preventing CI from blowing up. As @GuySartorelli has pointed out, making this a feature is antithetical to the whole premise of the code generation approach.

There was a hackday a while back where we experimented with file watching (e.g. _graphql/*.yaml, composer.lock) to stale the schema and allow on-the-fly generation to be more of a first-class citizen, but it's just fraught with limitations.

GuySartorelli commented 2 years ago

Going to close this for now. @maxime-rainville if you do have concerns about this feel free to re-open and we can discuss. But the concensus seems to be that this is working as designed.