Introduce config properties about dgs-framework HOT 13 CLOSED

Thanks for the detailed description and branch @bono007!

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

from dgs-framework.

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.

from dgs-framework.

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..

from dgs-framework.

@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.

from dgs-framework.

@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.

from dgs-framework.

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).

from dgs-framework.

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.

from dgs-framework.

@bono007 LOL 👍

from dgs-framework.

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.

from dgs-framework.

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.

from dgs-framework.

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!

from dgs-framework.

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.

from dgs-framework.

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

from dgs-framework.