Introduce config properties

[marceloverdijk]

The DGS framework takes a hard stance on how to configure itself:

• Hardcoded /graphql path
• Hardcoded /schema.json path and whether it is enabled or not
• Hardcoded /graphiql path and whether it is enabled or not
• Hardcoded classpath*:schema/**/*.graphql* to load the schema files from

It would be nice if DGS would offer some configuration options like:

dgs.graphql.path = /graphql
dgs.graphql.schema-location = classpath*:schema/**/*.graphql*
dgs.graphql.schema-json.enabled = true
dgs.graphql.schema-json.path = /schema.json
dgs.graphql.graphiql.enabled = true
dgs.graphql.graphiql.path = /graphiql

Rationale is to be able to change paths, but also to disable certain functionality.
E.g. GraphiQL is very useful during development but it might be desired or required to disable this in production because of security requirements or customized GraphiQL pages served differently (same for /schema.json).
For the schema-location, some might want to use a different location like classpath:graphql/schema.graphqls to reduce classpath scanning.

Probably there are more hardcoded things that could be moved to @ConfigrationProperties (websockets?)

[paulbakker]

Using @ConfigurationProperties in general is something we want to do.
Adding extra properties for paths etc. makes sense too, there isn’t really a reason these things aren’t configurable other than we didn’t have a need for it (but that doesn’t mean others won’t).

[onobc]

Preface Thoughts

• The "introduce config props" can be a nebulous bag of features as each configuration "knob" has to have something done w/ it to make it work. I do agree with and understand the motivation of the ticket though.
• We all know this, but it is re-stating out loud: Premature configuration comes w/ a cost as the configuration variants have to be maintained and bug fixed. The bugs will come in. :). I do believe that the "configuration knobs" in this ticket make sense and somebody may want to use them, but unless someone is actually asking for them, we should probably punt on the non-trivial ones (graphiql path is in this camp).

Summary

So interestingly enough, only 1 of the example configuration "knobs" in the description would introduce a @ConfigurationProperties entry. Here is why:

Some config props end up not being used in code (hitting the ConfigurationProperties accessors) but are rather used in SpEL paths in annotations such as @ConditionalOnProperty(name = ["my.props.feature.enabled"]) or @RequestMapping("\${my.props.path}"). It is a common practice to not define these in @ConfigurationProperties but rather as additional metadata info.

• Here is one example in Spring Boot.
• Here is one example in Spring Cloud Consul.

First Glance

I took a quick stab at implementing the config "knobs" in this sample branch Here is a summary:

• dgs.graphql.path - solved by @RequestMapping
• dgs.graphql.schema-json.enabled - solved by @ConditionalOnProperty
• dgs.graphql.schema-json.path - solved by @RequestMapping
• dgs.graphql.graphiql.enabled - solved by @ConditionalOnProperty
• dgs.graphql.graphiql.path : I did not touch this one as it seems not as straightforward as the other asks and I think it should be in a follow on ticket of its own.
• dgs.graphql.schema-location - solved by @ConfigurationProperties (multiple entries as it does main, test, meta-inf resolution)

Kapt and annotation processing

Also, I am not super familiar w/ Kotlin, but I do see this information around config props and kapt. Looks like Kapt will need to be configured in etc..

Next steps

I propose that we do the following:

• solve the 1st 4 items in the list above w/o config props (as in my sample branch)
• add the 4 props into the additional metadata info
• solve the schema-location with the newly added config props
• add/update tests
• figure out what to do w/ kapt
• create a follow-on ticket for configuring the graphiql path
• update docs / add section in guide for config props (example)

Another approach could be to split out the non config props work such as:

• Ticket 1 (non-config props):
  • solve the 1st 4 items in the list above w/o config props (as in my sample branch)
  • add the 4 props into the additional metadata info
  • add/update tests
  • update docs / add section in guide for config props (example)
• Ticket 2 (config props intro):
  • solve the schema-location with the newly added config props
  • figure out what to do w/ Kapt
  • add/update tests
  • update newly added section in guide for config props (example)
• Ticket 3:
  • create a follow-on ticket for configuring the graphiql path

Ok, enough of my rambling manifesto. I am interested and already invested in this ticket and can follow through whatever approach we decide on. Just lmk.

[paulbakker]

Thanks for the detailed description and branch @bono007!

@berngp Can you have a look at this as well?

[marceloverdijk]

Nice analysis @bono007

I wasn't aware of the additional metadata info and I guess that also helps with auto-completion of property files in e.g. IntelliJ?

In the past I've added all properties to @ConfigurationProperties classes - even the ones not used in code - for that reason, auto-completion.

I think it absolutely makes sense to split up the work in the tickets you mentioned.
Ticket 1 can be implemented fairly easy without taking on board maintenance overhead.

Being able to easily enable/disable the Graph_i_QL and the schema json endpoints would be a big win for me personally and the main reason I created this issue.

[onobc]

About 15 minutes after adding my comment, my co-worker (who must be watching the repo closely) pinged me and pointed out that we will want to configure the path to graphiql within the next month in our applications. So, great foresight @marceloverdijk :)

@paulbakker I am going to go ahead and follow through w/ the non-config props work in my branch (add tests and docs) as I have a nice hacking window carved out this am. If we end up shifting direction after @berngp reviews the above then I will gladly pivot, update, etc..

[marceloverdijk]

@bono007 LOL 👍

[berngp]

@bono007 thanks for the arguments you made and the PR!

Already approved the PR. For completeness I included some thoughts that came to mind when reviewing the PR.

1. Instead of defining a default value when accessing the property I prefer to define an Environment Post-Processor that loads a defaults from a default property file. The 'admin/env' actuator will tell you the source of such vale. If we define a default when we access instead, understanding the source of this value is obscured. All said I'm happy to do this as a separate PR.

2. The metadata file would be a good to have, but again happy to do this as a separate pr. Ref here.

[onobc]

@berngp you are welcome and thank you for the review and feedback. I like your idea on 1) above, and in a follow on PR sounds good. On 2), yep, agreed and I intend on doing that in the scope of the current MR but just ran out of time this morning. I will add that later this evening though.

[paulbakker]

This can be done separately from the first PR, but we'll need some more work for GraphiQL. E.g. if you change the /graphql path, GraphiQL won't work anymore, because the setting isn't carried over to the graphiql configuration.

[onobc]

This can be done separately from the first PR, but we'll need some more work for GraphiQL. E.g. if you change the /graphql path, GraphiQL won't work anymore, because the setting isn't carried over to the graphiql configuration.

In that case, I would be in favor of pulling the config "knob" for the graphql path out of this PR and into the start of a follow on PR that would add the config for both graphql path and graphiql path. I don't mind doing it - just lmk.

[paulbakker]

This can be done separately from the first PR, but we'll need some more work for GraphiQL. E.g. if you change the /graphql path, GraphiQL won't work anymore, because the setting isn't carried over to the graphiql configuration.

In that case, I would be in favor of pulling the config "knob" for the graphql path out of this PR and into the start of a follow on PR that would add the config for both graphql path and graphiql path. I don't mind doing it - just lmk.

That sounds good to me!

[paulbakker]

Everything else looks good to me; I have also tested it in a test app and it seems to work well.

@bono007 Can you confirm Kapt is required for the config properties to be picked up? I'm ok adding it, but only if necessary.

[marceloverdijk]

I have created #141 for ticket "2" in the earlier discussion so it will not be forgotten and can be tracked.