Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX Attempt to rebuild the schema if a schema file is missing #521

Merged

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Mar 7, 2023

Issue #500

Schema building code is copied from https://github.com/silverstripe/silverstripe-graphql/blob/4/src/Dev/Build.php#L81 though I've removed all the logging.

I did have some concerns that this may lead to make it easy to allow bad users to trigger schema rebuilds by sending graphql requests the deliberately contain things that aren't in the schema, though there appears to be validation done before it reaches that point. For instances this is the standard graphql request send when doing a request for elemental, :

{"operationName":"ReadBlocksForArea","variables":{"id":1},"query":"query ReadBlocksForArea($id: ID!) {\n readOneElementalArea(filter: {id: {eq: $id}}, versioning: {mode: DRAFT}) {\n elements {\n id\n title\n blockSchema\n obsoleteClassName\n isLiveVersion\n isPublished\n version\n canCreate\n canPublish\n canUnpublish\n canDelete\n __typename\n }\n __typename\n }\n}\n"}

I've tried adding "X"'s to various bits of this request, and tested using Firefox's "Edit and resend" in the network panel, though none of them trigger a debug break point I placed in the new code that rebuilds the schema AbstractTypeRegistry::get().

These are the responses I got from the server.

  • "operationName":"XReadBlocksForAreaX" -- standard response i.e. "ReadBlocksForArea" key isn't important
  • "XqueryX":"query ReadBlocksForArea( -- { message: 'This endpoint requires a "query" parameter', code: 400, file: "/var/www/vendor/silverstripe/framework/src/Control/RequestHandler.php", … }
  • "query":"XqueryX ReadBlocksForArea( -- { message: 'Syntax Error: Unexpected Name "XqueryX"', code: 0, file: "/var/www/vendor/webonyx/graphql-php/src/Language/Parser.php", … }
  • "query":"query XReadBlocksForAreaX( -- standard response i.e. "ReadBlocksForArea" after query isn't important
  • XreadOneElementalAreaX(filter -- { message: 'Cannot query field "XreadOneElementalAreaX" on type "Query". Did you mean "readOneElementalArea"?', code: 0, file: "/var/www/vendor/webonyx/graphql-php/src/Validator/Rules/FieldsOnCorrectType.php", … }
  • readOneXElementalAreaX( -- { message: 'Cannot query field "readOneXElementalAreaX" on type "Query". Did you mean "readOneElementalArea"?', code: 0, file: "/var/www/vendor/webonyx/graphql-php/src/Validator/Rules/FieldsOnCorrectType.php", … }

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine, works locally. I can't find any way to trip this up or cause it to do anything unexpected. We only ever get to this part of the code if somewhere in the schema it says there's a type, so it only happens in these scenarios:

  1. The schema is incorrectly configured (should be caught during dev so no impact on production)
  2. The schema is incomplete (e.g. files removed or schema build interrupted somehow - this seems to be the scenario of the original report)

Just one change to make to be sure we're respecting peoples' configuration.

try {
return static::fromCache($typename);
} catch (Exception $e) {
// Try to rebuild the whole schema as fallback.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to make sure we respect SilverStripe\GraphQL\Controller::autobuildEnabled()

Suggested change
// Try to rebuild the whole schema as fallback.
if (!Controller::has_curr() || !(Controller::curr() instanceof GraphQLController) || !Controller::curr()->autobuildEnabled()) {
throw $e;
}
// Try to rebuild the whole schema as fallback.

The class names here assume the new use statements:

use SilverStripe\Control\Controller;
use SilverStripe\GraphQL\Controller as GraphQLController;

Note that the Controller::has_curr() here is probably redundant - but please include it. I'd rather be safe than sorry here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

@emteknetnz emteknetnz force-pushed the pulls/4.1/extra-fallback branch from 8353fc1 to c2c9b1a Compare March 9, 2023 22:30
@GuySartorelli GuySartorelli merged commit fadb1b2 into silverstripe:4.1 Mar 9, 2023
@GuySartorelli GuySartorelli deleted the pulls/4.1/extra-fallback branch March 9, 2023 23:13
@GuySartorelli
Copy link
Member

Released as 4.1.1 and 4.2.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants