Skip to content

Log unknown properties when using @ConfigurationProperties #9936

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

Closed
jkubrynski opened this issue Aug 2, 2017 · 16 comments
Closed

Log unknown properties when using @ConfigurationProperties #9936

jkubrynski opened this issue Aug 2, 2017 · 16 comments
Labels
status: duplicate A duplicate of another issue

Comments

@jkubrynski
Copy link
Contributor

Enhancement

With YAML properties it's too easy to corrupt properties, by just adding whitespace. eg:

# incorrect
spring:
  cloud:
    stream:
      input:
        consumer:
          maxAttempts: 5
          group: api
          
# correct
spring:
  cloud:
    stream:
      input:
        consumer:
          maxAttempts: 5
        group: api

If would be great if regardless the ignoreUnknownFields property, Spring Boot could load a WARN that unmatched property was found. I think that will preserve many stupid mistakes people are making. I can possibly do a PR if you agree it should be implemented.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 2, 2017
@wilkinsona
Copy link
Member

It's an interesting suggestion but I think it would lead to far too many warnings being generated when a binding target needs ignoreUnknownFields for whatever reason.

IMO, your problem is an argument in favour of using a properties file rather than YAML. Alternatively, your IDE (IDEA, Eclipse, or NetBeans) can already warn you when you use an unknown property.

We could, perhaps, consider debug logging if you still feel that would be useful.

@wilkinsona wilkinsona added the status: waiting-for-feedback We need additional information before we can continue label Aug 3, 2017
@jkubrynski
Copy link
Contributor Author

@wilkinsona The problem is that IDE cannot notify about the properties I've mentioned. The only way to solve it during runtime binding. I prefer YAML instead of properties as it is much easier to read and it has a nice structure. I'm not saying to enable it by default, but maybe it could be a parameter set on SpringBootApplication?

@wilkinsona
Copy link
Member

I'm not saying to enable it by default, but maybe it could be a parameter set on SpringBootApplication?

It feels much too specialised (and, given the lilkelihood of false positives, only really useful for debugging purposes) to warrant an attribute on SpringBootApplication. If we're going to do this at all, I'd rather use debug logging and enable it by configuring the logging level. Would that still address your need?

@wilkinsona wilkinsona added status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-feedback We need additional information before we can continue labels Aug 3, 2017
@jkubrynski
Copy link
Contributor Author

Most projects I've seen keep properties in the separate repository than the application. So you have different properties for production than for staging. I'm not sure if many people enable debug logging on production. INFO would be OK, because I can configure filters in ELK stack to figure out such issues.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Aug 3, 2017
@wilkinsona
Copy link
Member

I'm not sure if many people enable debug logging on production

It would be debug logging for one specific logger that only reported what you're asking for here.

INFO would be OK, because I can configure filters in ELK stack to figure out such issues.

Info doesn't help. It's output by default and I believe there would be too many false positives to be generally useful.

@wilkinsona wilkinsona added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Aug 3, 2017
@jkubrynski
Copy link
Contributor Author

I'll check in our applications how many properties are reported, and then I'll get back to you.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Aug 3, 2017
@wilkinsona wilkinsona added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Aug 3, 2017
@snicoll
Copy link
Member

snicoll commented Aug 3, 2017

For the record I agree with Andy. I guess it can't tell you because the target is a map and group is just a key? If I misunderstood, can you please clarify why the ide can't tell you.

Also some IDE shows keys of a map in a different font to let you know what type it is. All in all I do not think Spring Boot can/should attempt to fix those potential mistakes.

@jkubrynski
Copy link
Contributor Author

jkubrynski commented Aug 3, 2017

It's not a map -> it's a org.springframework.cloud.stream.config.BindingProperties class. There are two reasons why IDE won't help in such cases:

  1. Environment properties are usually kept in different repository than application - then you use for example Spring Cloud Config Server to serve them.
  2. Spring Cloud Stream properties are dynamic, so IDE won't resolve them. Leastways Intellij is not helping here.

So to sum up -> if you know how to solve above in different way than you using Spring Boot please let me know. But please take care about people who use your product on production and not only during conference talks.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Aug 3, 2017
@wilkinsona
Copy link
Member

wilkinsona commented Aug 3, 2017

But please take care about people who use your product on production and not only during conference talks

To suggest that we only care about people using Spring Boot during conference talks purely because we're not keen on something you've proposed is both rude and ridiculous. If we didn't care this issue would already have been closed rather than the team replying to you four times already today.

I'll check in our applications how many properties are reported, and then I'll get back to you.

Thank you. That would be a useful data point that might allow us to make some constructive progress. If it becomes apparent that there's some useful information that can be determined at runtime then we can think about how to surface that information. IMO, for the information to be useful it will have to be largely free of false-positives and I have a hunch that that is unlikely to be the case.

@wilkinsona wilkinsona added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Aug 3, 2017
@wilkinsona
Copy link
Member

I've opened #9945 to explore the possibility of improving the metadata generation (the first step towards the IDE warning about unknown properties).

@jkubrynski
Copy link
Contributor Author

@wilkinsona The metadata won't help in real-world use cases, because then you keep properties in different repository than the application.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Aug 3, 2017
@wilkinsona
Copy link
Member

@jkubrynski It may not help with your specific use case, but that does not mean that it will be of no help in all real-world use cases. Can we please try to focus on moving this issue forwards? I'm still interested in the data that you offered to collect.

@wilkinsona wilkinsona added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Aug 3, 2017
@jkubrynski
Copy link
Contributor Author

Of course, it's better to have such properties included in the metadata than not. However please keep in mind, that a lot of production systems keep properties in the separate repository than the application source code. The IDE plugin doesn't use metadata in non-java projects so it can be used only during development. Often even the operation teams manage those properties directly, using vi instead of the Intellij. So the very first moment to verify the properties is during the application startup - in such case the sooner the better. In my opinion, WARN is adequate to seriousness of the situation, but of course if I must choose between DEBUG and nothing I prefer DEBUG.

I've checked our applications and currently, there are no invalid properties. We include over 15 different Spring Boot & Spring Cloud starters. However, the problem I found is that after adding unknown property it's logged 3 times:

22:32:14.705 WARN [localhost-startStop-1] b.b.RelaxedDataBinder$RelaxedBeanWrapper : Unknown property found: Invalid property 'bindings[someName].producer.group' of bean class [org.springframework.cloud.stream.config.BindingProperties]: Bean property 'group' is not writable or has an invalid setter method. Did you mean 'group'?
22:32:43.751 WARN [main] b.b.RelaxedDataBinder$RelaxedBeanWrapper : Unknown property found: Invalid property 'bindings[someName].producer.group' of bean class [org.springframework.cloud.stream.config.BindingProperties]: Bean property 'group' is not writable or has an invalid setter method. Did you mean 'group'?
22:32:43.756 WARN [main] b.b.RelaxedDataBinder$RelaxedBeanWrapper : Unknown property found: Invalid property 'bindings[someName].producer.group' of bean class [org.springframework.cloud.stream.config.BindingProperties]: Bean property 'group' is not writable or has an invalid setter method. Did you mean 'group'?

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Aug 5, 2017
@wilkinsona
Copy link
Member

Superseded by #10030 which is our preferred way to tackle this problem.

@wilkinsona wilkinsona added status: duplicate A duplicate of another issue and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged labels Sep 29, 2017
@izeye
Copy link
Contributor

izeye commented Nov 3, 2017

@wilkinsona Meant to close?

@wilkinsona
Copy link
Member

Yes. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: duplicate A duplicate of another issue
Projects
None yet
Development

No branches or pull requests

5 participants