-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
HIVE-27895: Remove dependency jodd-util #4888
Conversation
cc @sunchao @wangyum |
* limitations under the License. | ||
*/ | ||
|
||
// This class is ported from org.jodd:jodd-util:6.0.0 and remove unreferenced code |
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.
document where the code comes from, and also preserve the original license header
Sorry, I can not understand why we choose this way(copy source code) to fix CVE. https://github.com/oblac/jodd-util is still a active a repo, i think we can still use the dependecy to fix CVE. |
@zhangbutao I mostly stand on a downstream project perspective. Hive has a low release rate. The latest stable versions(except for alpha and beta) are
Once CVEs are reported as caused by Hive's transitive dependencies, such a release rate makes downstream projects like Spark awkward. Spark uses Hive 2.3.9 now. Hive 2.3.9 has many dependencies which have CVEs:
Hive only uses a few codes (~200 lines) of the |
* </ul> | ||
*/ | ||
public static String text(final CharSequence text) { |
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.
I just found it could be replaced with org.apache.commons.lang3.StringEscapeUtils#escapeHtml4
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.
Copy-pasting code from third party projects is not a good idea. There is no reason to do it especially since there is no real issue to address in this case. I don't think we should start doing such changes.
Moreover, the copy of JulianDate here class has license ambiguities and this is a definitely a reason for -1.
@zabetak the |
In most cases, I agree, but the size should be counted, I don't think it is worth pulling a dependency because of a few lines of code reference.
For master branch, there is no real issue, my real target is branch-2.3, just following the common "upstream first" practice to do change on the master branch and then do backport. As you oppose such change on master branch, is it acceptable if I only do such change for branch-2.3? @zabetak |
Kudos, SonarCloud Quality Gate passed!
|
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.
Hi @pan3793,
I do not think this is the right approach to fix a CVE. Yes, the releases are not as frequent as it should be. But we as a community try to change on this. Moreover to move a code internally to the repository it is just a very short-term gain. Yes, it is not so big, also not so complex. BUT the jodd-util responsibility for this move to Hive repo is not so good imho.
-1
@pan3793 Please check the respective section about how to treat third party works in ASF projects. |
@zabetak thanks for pointing it out. so copying is legal as long as we follow the guidance of ASF. Seems reducing dependencies is not a mission in the Hive project, close it. |
What changes were proposed in this pull request?
Remove dependency
jodd-util
.Why are the changes needed?
HIVE-25054(only present on 4.0.0) fixed the jodd CVE by upgrading it to a new version, when I'm looking to backport this to branch-2.3, I find Hive only uses a few code of this lib, so I think copy such code snippets and remove this dependency should be a better way.
Does this PR introduce any user-facing change?
No
Is the change a dependency upgrade?
No, but it removes a dependency.
How was this patch tested?
Pass CI