Avoid sha-1 resource load for remote download verification.#1547
Conversation
As per https://maven.apache.org/resolver/expected-checksums.html#non-standard-x-headers, many repository managers provide the sha-1 in a header already, so the extra request can be avoided. Fixes chains-project#1543
|
Hi @timw ! Thanks for your PRs. I have been a bit busy with other things, so I have not had time to look through them. Maybe @LogFlames can you take a look at this and the other two PRs if you have time? |
algomaster99
left a comment
algomaster99
left a comment
There was a problem hiding this comment.
Very clever optimization. Just also want to ensure we document this information in the code. Rest everything is good.
| // Fallback to verify downloaded artifact with SHA-1. Look first in standard repository manager | ||
| // headers. |
There was a problem hiding this comment.
| // Fallback to verify downloaded artifact with SHA-1. Look first in standard repository manager | |
| // headers. | |
| // Look first in standard repository manager headers as per | |
| // https://maven.apache.org/resolver/expected-checksums.html#non-standard-x-headers. | |
| // Fallback to verify downloaded artifact with SHA-1. |
| // headers. | ||
| var maybeVerificationChecksum = artifactResponse | ||
| .headers() | ||
| .firstValue("x-checksum-sha1") |
There was a problem hiding this comment.
| .firstValue("x-checksum-sha1") | |
| // curl -I https://repo1.maven.org/maven2/org/mvnpm/axios/1.15.1/axios-1.15.1.jar | |
| // x-checksum-sha1: 8161aecbad5fceb8ce4aca8b557be1a8b77b5cbe | |
| .firstValue("x-checksum-sha1") |
| .headers() | ||
| .firstValue("x-checksum-sha1") | ||
| .or(() -> artifactResponse.headers().firstValue("x-goog-meta-checksum-sha1")) | ||
| .or(() -> { |
There was a problem hiding this comment.
| .or(() -> { | |
| //Non-Maven Central or non-GCS hosted packages may not emit these headers | |
| // curl -I https://plugins.gradle.org/m2/org/jmailen/gradle/kotlinter-gradle/5.3.0/kotlinter-gradle-5.3.0.jar | |
| .or(() -> { |
|
@timw could you please accept my suggestions if you agree with them? |
|
Hi there! @timw @algomaster99 Nice optimization here. It would be nice to have it as a part of performance optimizations we need. We hit a performance bottleneck on some real world projects after implementing #1532. Definitely vote for this one +1 |
|
Move the PR here. |
|
@fz-rh the commit here is in |
|
Thank you! @algomaster99 |
|
Sounds good! Have not reviewed that PR yet but performance tests will definitely help in doing so. |
3 participants
As per https://maven.apache.org/resolver/expected-checksums.html#non-standard-x-headers, many repository managers provide the SHA-1 in a header already, so the extra request can be avoided.
Fixes #1543