-
Notifications
You must be signed in to change notification settings - Fork 317
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
analyzer: Stop using a SortedSet
in ProjectAnalyzerResult
#6244
Conversation
|
||
import com.fasterxml.jackson.databind.util.StdConverter | ||
|
||
class SortedSetConverter<T : Comparable<T>> : StdConverter<Set<T>, Set<T>>() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, this is a generic implementation that works on Comparable<T>
, which has the downside that the classes still need to implement Comparable
, and the current implementations are confusing as they're not in line with equals()
.
So maybe we should prefer to have dedicated converters per class that we want to sort as these could hard-code the property to sort on then, like
class SortedPackageSetConverter : StdConverter<Set<Package>, Set<Package>>() {
override fun convert(value: Set<Package>): Set<Package> =
value.toSortedSet(compareBy { it.id })
}
That would allow us to remove the Comparable
implementations in exchange for several more converted classes. But as there are only ~10 implementations that we need to care about as they influence serialization, that's maybe still a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So maybe we should prefer to have dedicated converters per class [...] That would allow us to remove the
Comparable
implementations in exchange for several more converted classes.
For the record, that's what I did now.
2dcaffc
to
0258766
Compare
0258766
to
34962d6
Compare
FYI @mnonnenmacher, a Which means that this change may uncover previously unnoticed cases where packages with the same id but different other properties are being added to the result. |
34962d6
to
4b3aa53
Compare
Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
4b3aa53
to
68aa67c
Compare
As now all properties are taken into account, this aligns `compareTo() == 0` with `equals() == true`. Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
There is no need to have these sorted in memory. Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
It is fine if the issues are not created in the order of sorted package ids. Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
The `ProjectAnalyzerResult` is only serialized as part of analyzer functional tests when comparing to expected results. So in memory, there is no need for the `packages` to be a `SortedSet`. Change the code to only sort `packages` on serialization via a Jackson converter to have a defined order when comparing to expected results. This lays the fundation for getting rid of `Comparable` implementations in various classes. These implementations were only added for sorted output during serialization, but they break the `Set` contract as `compareTo() == 0` is not aligned with `equals() == true`, also see [1]. [1]: https://docs.oracle.com/javase/8/docs/api/java/util/SortedSet.html Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
68aa67c
to
179fbc3
Compare
Continuing the story from [1], only sort authors on serialization. [1]: #6244 Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
The
ProjectAnalyzerResult
is only serialized as part of analyzer functional tests when comparing to expected results. So in memory, there is no need for thepackages
to be aSortedSet
. Change the code to only sortpackages
on serialization via a Jackson converter to have a defined order when comparing to expected results.Signed-off-by: Sebastian Schuberth sschuberth@gmail.com
Part of: #6235