Skip to content
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

SOLR-16562: Upgrade to Caffeine 3.1.2 #1198

Merged
merged 3 commits into from Dec 8, 2022
Merged

Conversation

risdenk
Copy link
Contributor

@risdenk risdenk commented Nov 29, 2022

https://issues.apache.org/jira/browse/SOLR-16562

This update of Caffeine includes infinite loop checking.

This also upgrades the following:
* SOLR-16579 - Jackson 2.14.1
* SOLR-16578 - errorprone 2.16
* and other minor dependencies based on transitive dependencies.

@risdenk
Copy link
Contributor Author

risdenk commented Nov 29, 2022

Relates to #1118 - the underlying issue w/ infinite loop is mutable Lucene/Solr query classes.

Copy link
Contributor

@epugh epugh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

amazing how many files an upgrade touches, however it all LGTM

@fsparv
Copy link

fsparv commented Nov 30, 2022

Can we fix the typo comment at the top to say woodstox-core 6.4 not 2.4? Also it looks like this happens to address CVE-2022-40152

@risdenk
Copy link
Contributor Author

risdenk commented Nov 30, 2022

Thanks for catching that @fsparv - updated the commit message. Bummer not all dependencies start with 2.x :P

@dsmiley
Copy link
Contributor

dsmiley commented Nov 30, 2022

Why is this upgrading more than one thing? Pure convenience? Caffeine has no dependencies.

@risdenk
Copy link
Contributor Author

risdenk commented Nov 30, 2022

Why is this upgrading more than one thing? Pure convenience? Caffeine has no dependencies.

https://mvnrepository.com/artifact/com.github.ben-manes.caffeine/caffeine/3.1.2

Caffeine definitely has dependencies. Upgrading Caffeine triggered these dependency updates.

@dsmiley
Copy link
Contributor

dsmiley commented Dec 1, 2022

RE Caffeine definitely has dependencies: My attempt to show this was looking at gradlew dependencies --configuration runtimeClasspath but I suppose this is flawed, like maybe if the same dependencies come through from multiple paths in the tree. Next time I'll just look at its pom and/or via mvnrepository.com. Thanks.

@risdenk
Copy link
Contributor Author

risdenk commented Dec 1, 2022

yea I'm not even sure I trust the mvnrepository.com output - https://github.com/ben-manes/caffeine/blob/master/gradle/dependencies.gradle shows lots of dependencies at least managed. https://github.com/ben-manes/caffeine/blob/master/caffeine/build.gradle is the limited list just for caffeine library. I wouldn't have guessed these dependencies updates would happen without the consistent versions highlighting it.

FWIW I'm not opposed to splitting the versions.props specific dependency changes (jackson, woodstox, errorprone) into other commits.

@dsmiley
Copy link
Contributor

dsmiley commented Dec 1, 2022

I have no problem doing a bunch of updates at once so long as it's clear in JIRA & CHANGES.txt that we did so

@ben-manes
Copy link

Caffeine has no required dependencies. It does have optional annotations that can be excluded (checker, errorprone) that are compile scoped. The managed dependencies are internal for testing, benchmarks, analysis and should not be in the pom.

The actual bug in solr hasn’t been fixed, so while this is good to fail fast, I am s little concerned that the root problem won’t be addressed.

@ben-manes
Copy link

ben-manes commented Dec 3, 2022

I believe that the managed dependencies in the pom are Gradle constraints added to protect the build itself from exploits. It’s noise that doesn’t impact users, but I’ll look into removing it from the pom as an implementation detail to avoid CI attacks like the codecov breach.

Edit: removed from the published pom for future releases

@ben-manes
Copy link

Solr worked around the same issue wrt managed dependencies being unintentionally exported. While this fixes the pom, the gradle module metadata still includes these and, as Gradle prefers that over the pom file, it takes precedence for consumers who use a Gradle build. To resolve that, I switched to not applying constraint to the published configuration, which removed the section from the metadata files.

pom({
// LUCENE-9561:
// Remove dependencyManagement section created by a combination of
// Palantir and the publishing plugin.
//
// https://github.com/palantir/gradle-consistent-versions/issues/550
withXml {
asNode().dependencyManagement.replaceNode {}
}
})

@risdenk
Copy link
Contributor Author

risdenk commented Dec 7, 2022

Thanks @ben-manes - I decided to separate these upgrades for Jackson and errorprone into separate commits to make it easier to see in solr/CHANGES.txt and not conflate the caffeine upgrade.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants