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: GraphiQL may render broken and cannot be fully disabled #1183

Merged
merged 3 commits into from Oct 21, 2022

Conversation

foo4u
Copy link
Contributor

@foo4u foo4u commented Aug 5, 2022

Pull request checklist

  • Please read our contributor guide
  • Consider creating a discussion on the discussion forum
    first
  • Make sure the PR doesn't introduce backward compatibility issues
  • Make sure to have sufficient test cases: tests cases already exist to cover not enabling GraphiQL for both MVC and WebFlux. However, the auto configuration for automatic resource discovery is not enabled on such tests. If you'd like this PR to be updated with static resource loading enabled I can do that.

Pull Request type

  • Bugfix

Changes in this PR

Fix a few issues with GraphiQL:

Alternatives considered

DGS_GRAPHQL_PATH is sometimes unset because there's a race condition between Spring's built in static resource handler and DGS' GraphiQL handlers in both Spring MVC and WebFlux. The alternative option to changing the path for graphiql would be to force applications to configure spring.web.resources.add-mappings to false; it defaults to true.

PR #1167 from another contributor, also says it fixes #1135. However, it looks like a different potential issue related non-ASCII data in queries, rather than a fix for this bug.

@foo4u
Copy link
Contributor Author

foo4u commented Sep 5, 2022

@srinivasankavitha any way to get 👀 on this?

@@ -48,6 +48,7 @@ class DgsWebfluxConfigurationProperties(
* Configuration properties for the GraphiQL endpoint.
*/
data class DgsGraphiQLConfigurationProperties(
@DefaultValue("true") var enabled: Boolean = true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this used?

Copy link
Contributor Author

@foo4u foo4u Sep 6, 2022

Choose a reason for hiding this comment

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

It's used by Spring's metadata generator. The property isn't used directly by DGS b/c it's referred to in full dot notation as a @Conditional. This is mainly useful for IDE autocompletion.

@ConditionalOnProperty(name = ["dgs.graphql.graphiql.enabled"], havingValue = "true", matchIfMissing = true)
open fun graphiQlConfigurer(configProps: DgsWebfluxConfigurationProperties): GraphiQlConfigurer {
return GraphiQlConfigurer(configProps)
}

@@ -35,7 +35,7 @@ class GraphiQlConfigurer(private val configProps: DgsWebfluxConfigurationPropert
val graphiQLTitle = configProps.graphiql.title
registry
.addResourceHandler(configProps.graphiql.path + "/**")
.addResourceLocations("classpath:/static/graphiql/")
Copy link
Contributor

Choose a reason for hiding this comment

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

For my understanding, could you explain why moving the location out of the static directory addresses the race condition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DGS_GRAPHQL_PATH is sometimes unset because there's a race condition between Spring's built in static resource handler and DGS' GraphiQL handlers in both Spring MVC and WebFlux. The alternative option to changing the path for graphiql would be to force applications to configure spring.web.resources.add-mappings to false; it defaults to true.

So, Spring attempts to serve all assets under resources/static as static files. This means the resources are served as-is from disk / JAR, and not processed by the DGS handler. Depending on bean registration order, the DGS handler may be registered after Spring's static resource handler.

From my understanding about Spring, it's best NOT to store templates that require server side rendering in /resources/[static|public].

@foo4u foo4u force-pushed the srossillo.fix-graphiql-issues branch from cee2503 to 2208027 Compare September 30, 2022 12:41
@foo4u
Copy link
Contributor Author

foo4u commented Sep 30, 2022

Hi @srinivasankavitha , I answered your questions and rebased this on master. It should be ready. 🙏 Thank you!

@foo4u foo4u force-pushed the srossillo.fix-graphiql-issues branch from 2208027 to 90c41b2 Compare October 9, 2022 19:07
@foo4u foo4u force-pushed the srossillo.fix-graphiql-issues branch from 90c41b2 to f338788 Compare October 11, 2022 15:53
@foo4u foo4u force-pushed the srossillo.fix-graphiql-issues branch from f338788 to 6741dc3 Compare October 19, 2022 18:24
@srinivasankavitha srinivasankavitha merged commit 9c439d3 into Netflix:master Oct 21, 2022
@foo4u foo4u deleted the srossillo.fix-graphiql-issues branch October 26, 2022 15:56
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.

bug: Sometimes DGS_GRAPHQL_PATH is not set and it breaks the GraphiQL forcing me to redeploy the app
2 participants