Skip to content

Configuration metadata processor ignores collection-based nested configuration non-primitive properties #9894

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
mmoayyed opened this issue Jul 28, 2017 · 5 comments
Assignees
Labels
status: declined A suggestion or change that we don't feel we should currently apply

Comments

@mmoayyed
Copy link
Contributor

Background

I first discussed this issue in the Gitter chatroom with @dsyer and I suspect the potential diagnosis was that it's a bug. So I'd like to document it here first and bring it to your attention, and certainly plan to offer a fix with a follow-up pull request if there is interest and need.

  • This is seen with Spring Boot 1.5.6 and Gradle 4.
  • A sample application to demonstrate what I think is a bug can be found here.

Problem Statement

The configuration metadata processor seems to be ignoring nested configuration properties and fields that are of type Collection and the inner type is an external class. When a properly configured build is run, the final generated metadata in the .json file does not contain any of the fields expected to be found in the inner class in some indexed way perhaps.

Example:

    @NestedConfigurationProperty
    private List<SomethingProperties> sth = new ArrayList<>();

    public List<SomethingProperties> getSth() {
        return sth;
    }

    public void setSth(final List<SomethingProperties> sth) {
        this.sth = sth;
    }

After the processor has run, under groups one can find a reference to sth in the generated JSON metadata yet the properties do not contain any references or fields that belong to SomethingProperties. The sample application above should demonstrate this well. I am not entirely familiar with this area myself, but having worked with @wilkinsona before a bit I don't think this situation is by design.

Thank you for your time.

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

snicoll commented Jul 28, 2017

Two things:

  • @NestedConfigurationProperty doesn't need to be used on a list. A list is non scalar so there is no need to indicate the processor to browse items.
  • Gradle 4 is not (officially) supported with Spring Boot 1.5.x (the fork of the sample you used indicates that already)

We don't generate properties for list. Ever. What would be the key? your.namespace.sth[].name?

I don't know what you are trying to do but when I remove the @NestedConfigurationProperty annotation, I get this:

{
  "hints": [],
  "groups": [
    {
      "sourceType": "org.example.additional.metadata.MyConfigProps",
      "name": "my",
      "type": "org.example.additional.metadata.MyConfigProps"
    }
  ],
  "properties": [
    {
      "sourceType": "org.example.additional.metadata.MyConfigProps",
      "defaultValue": 42,
      "name": "my.int-prop",
      "description": "int-prop is a test property for testing Spring Boot configuration annotation processor.\n\n Additional hits metedata should work, but it does not.",
      "type": "java.lang.Integer"
    },
    {
      "sourceType": "org.example.additional.metadata.MyConfigProps",
      "name": "my.sth",
      "type": "java.util.List<org.example.additional.metadata.SomethingProperties>"
    }
  ]
}

Which works as advertized. If you have an idea to improve the javadoc and/or the doc of @NestedConfigurationProperty we can use this issue to track that.

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Jul 28, 2017
@mmoayyed
Copy link
Contributor Author

mmoayyed commented Jul 28, 2017

Thanks for the feedback. Few questions for my own understanding:

  • When you say @NestedConfigurationProperty does not need to be on a List, I assume that applies to all manners of other Collection type objects as well?
  • In the same spirit, I assume that when you say properties are not generated for lists, they likewise are not generated for Collections as a whole in this particular context, correct?

To elaborate further, I am working on a CLI interface that allows one to query a metadata repository to understand what fields and properties are available for a given group, etc. My goal is that when the metadata repository is queried, the user/developer can learn that "oh, there is apparently a my.sth that is indexed, and if I index it correctly I can set a test field on it, which does blah because it's described in the docs to do so". My goal is to let the processor pick up stuff that is inside SomethingProperties which as you say it's not happening today. I realize it's tricky to come up with the right syntax for nested collections, etc. Thinking out loud, how about the following options:

{
  "hints": [],
  "groups": [
...
  ],
  "properties": [
...
    {
      "sourceType": "org.example.additional.metadata.MyConfigProps",
      "name": "my.sth",
      "type": "java.util.List<org.example.additional.metadata.SomethingProperties>",
      "indexed": [
      	{
      		"sourceType": "org.example.additional.metadata.SomethingProperties",
      		"type": "java.lang.String",
      		"name": "my.sth[index].test"
      	}
      ]
    }
  ]
}

For the "name": "my.sth[index].test", I might also suggest alternatives:

  • "name": "my.sth[0].test"
  • "name": "my.sth[].test" (As you did also note)
  • "name": "test"

@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 Jul 28, 2017
@snicoll
Copy link
Member

snicoll commented Jul 28, 2017

If you have questions, can you please ask them on Gitter/Stackoverflow? Generating metadata for nested POJOs is not at the agenda so I am afraid that you'll have to discover those properties yourself in such cases (that's what IDEs are doing).

I'll give some thoughts as how we can improve the javadoc of @NestedConfigurationPropery. If you have ideas, feel free to submit a PR or share them here.

@snicoll snicoll added type: documentation A documentation update and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged labels Jul 28, 2017
@snicoll snicoll added this to the 1.5.7 milestone Jul 28, 2017
@snicoll snicoll self-assigned this Jul 28, 2017
@mmoayyed
Copy link
Contributor Author

mmoayyed commented Jul 28, 2017

Sure thing, will do wrt Gitter.

However, there is a question that I think deserves to be asked here. I am not really interested in improving the documentation with a PR as much as I was interested in getting this to work "properly" with a PR. If understood you correctly, are you saying that sort of change would not be accepted and that I'd have to deal with it on my own?

Thanks again.

@snicoll
Copy link
Member

snicoll commented Jul 28, 2017

Yes, that's what I meant by " Generating metadata for nested POJOs is not at the agenda". We've thought about supporting it and the number of corner cases that such support would bring is too high to consider the feature. We have to stop somewhere and I believe that stopping at collections/maps is the right call.

To make that a bit more clear. Let me close this issue and I'll create another one for the documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

No branches or pull requests

3 participants