Current comparator implementation produces not consistent order for special case, when a codebase contains multiple classes that shares same "namespace" and (optionally) same properties.
Consider this example:
package com.example;
@ConfigurationProperties("app.section")
public class Foo {
private int val;
}
@ConfigurationProperties("app.section")
public class Bar {
private boolean flag;
private int val;
}
As both classes share same section (and it works in runtime just fine), this leads to multiple outcomes in groups
sections, as well as in properties
section:
"groups": [
{
"name": "app.section", // order can be swapped!
"type": "com.example.Foo",
"sourceType": "com.example.Foo"
},
{
"name": "app.section",
"type": "com.example.Bar",
"sourceType": "com.example.Bar""
}
],
Same is applied for val
in this exampe, as name is same, but type/sourceType is different.
I understand that this is extreme example, but it is valid in terms of spring support, so I think that adding sort stage for type can solve this problem:
private final Comparator<ItemMetadata> propertyComparator = Comparator.<ItemMetadata, Boolean>comparing(ItemMetadata::isDeprecated)
.thenComparing(ItemMetadata::getName)
.thenComparing(ItemMetadata::getType);
private final Comparator<ItemMetadata> groupComparator = Comparator.<ItemMetadata, String>comparing(ItemMetadata::getName).thenComparing(ItemMetadata::getType);
Comment From: snicoll
@zeldigas thanks for the report. Curious what problem this causes for you concretely?
Comment From: zeldigas
@snicoll We are managing a set of libraries that we build in reproducible fashion using https://zlika.github.io/reproducible-build-maven-plugin/. One of the library has described setup - multiple beans sharing single section. When we changed build agents, we'd bump into this problem. With this issue fixed, we'd be able to get properly reproducible artifact.
Comment From: zeldigas
If you are fine with the proposed change, I'm ready to work on PR. Just let me know what branch to send (I think it'd make sense to fix it not just in master, but in earlier versions as well)
Comment From: philwebb
@zeldigas Thanks for the offer the PR. Submitting against master/main should be fine, we can rebase it onto any older branch when we merge. It would be helpful if there was a test to go along with the change. Thanks!
Comment From: zeldigas
Deal. I'm going to work on PR this weekend.
Comment From: zeldigas
PR is opened with small correction to original proposed fix - instead of Type, i'm using SourceType as it looks like type
describes type of fields for properties, that will not provide good ordering compared to sourceType
that identifies location.
Comment From: snicoll
Closing in favor of PR #26230
Comment From: zeldigas
@snicoll could you please clarify, what's the purpose of closing issue if PR is not merged yet?
Comment From: snicoll
The issue is now superseded by the PR, as indicated by the label I've added.
Comment From: zeldigas
OK, looks like I need to get familiar with the process better, sorry.