-
Notifications
You must be signed in to change notification settings - Fork 345
Use java.net.http.HttpClient instead of java.net.Http(s)URLConnection #217
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
Changes from all commits
9ac214a
dc78962
903f141
1ca9814
765b439
988481f
0022ea3
0b3734d
8f8ac86
b12db60
a240d4c
5b122a0
ed1f6ac
e16a8e0
9b696b8
09be2f4
8d78d28
fdf2026
cc25583
3b6e169
85103c6
06f94fd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| plugins { | ||
| pklAllProjects | ||
| pklJavaLibrary | ||
| pklPublishLibrary | ||
| } | ||
|
|
||
| publishing { | ||
| publications { | ||
| named<MavenPublication>("library") { | ||
| pom { | ||
| url.set("https://github.com/apple/pkl/tree/main/pkl-certs") | ||
| description.set(""" | ||
| Pkl's built-in CA certificates. | ||
| Used by Pkl CLIs and optionally supported by pkl-core.") | ||
| """.trimIndent()) | ||
| } | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -202,7 +202,7 @@ class CliPackageDownloaderTest { | |
|
|
||
| Failed to download package://bogus.domain/[email protected] because: | ||
| Exception when making request `GET https://bogus.domain/[email protected]`: | ||
| bogus.domain | ||
| Error connecting to host `bogus.domain`. | ||
|
|
||
| """ | ||
| .trimIndent() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ import java.nio.file.Files | |
| import java.nio.file.Path | ||
| import java.time.Duration | ||
| import java.util.regex.Pattern | ||
| import org.pkl.core.http.HttpClient | ||
| import org.pkl.core.module.ProjectDependenciesManager | ||
| import org.pkl.core.util.IoUtils | ||
|
|
||
|
|
@@ -113,15 +114,15 @@ data class CliBaseOptions( | |
| val testMode: Boolean = false, | ||
|
|
||
| /** | ||
| * [X.509 certificates](https://en.wikipedia.org/wiki/X.509) in PEM format. | ||
| * The CA certificates to trust. | ||
| * | ||
| * Elements can either be a [Path] or a [java.io.InputStream]. Input streams are closed when | ||
| * [CliCommand] is initialized. | ||
| * The given files must contain [X.509](https://en.wikipedia.org/wiki/X.509) certificates in PEM | ||
| * format. | ||
| * | ||
| * If not empty, this determines the CA root certs used for all HTTPS requests. Warning: this | ||
| * affects the whole Java runtime, not just the Pkl API! | ||
| * If [caCertificates] is the empty list, the certificate files in `~/.pkl/cacerts/` are used. If | ||
| * `~/.pkl/cacerts/` does not exist or is empty, Pkl's built-in CA certificates are used. | ||
| */ | ||
| val caCertificates: List<Any> = emptyList(), | ||
| val caCertificates: List<Path> = listOf(), | ||
| ) { | ||
|
|
||
| companion object { | ||
|
|
@@ -167,4 +168,26 @@ data class CliBaseOptions( | |
| projectDir?.resolve(ProjectDependenciesManager.PKL_PROJECT_FILENAME) | ||
| ?: normalizedWorkingDir.getProjectFile(rootDir) | ||
| } | ||
|
|
||
| /** [caCertificates] after normalization. */ | ||
| val normalizedCaCertificates: List<Path> = caCertificates.map(normalizedWorkingDir::resolve) | ||
|
|
||
| /** | ||
| * The HTTP client shared between CLI commands created with this [CliBaseOptions] instance. | ||
| * | ||
| * To release the resources held by the HTTP client in a timely manner, call its `close()` method. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't change I documented calling Let me know if you want me to change
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think We have a couple places where our builders accept Probably makes more sense to either accept the options on Same comment for the settings on
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sorry I don't understand what you're proposing. Can you elaborate?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sorry but I don't follow. Can you elaborate on what you're proposing? One option I considered was to pass
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, so, one suggestion is to add the builder methods on class EvaluatorBuilder {
public EvaluatorBuilder addCertificates(Path file) { /* etc */ }
public EvaluatorBuilder addCertificates(URI file) { /* etc */ }Another option is to accept an class EvaluatorBuilder {
public HttpClient.Builder setHttpClientBuilder(HttpClient.Builder builder) { /* etc */ }
}
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently we have
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm proposing that both here, and in evaluator builder, we provide knobs for After playing around with this locally, though, I don't feel strongly about this, and am okay with how you are doing it. |
||
| */ | ||
| val httpClient: HttpClient by lazy { | ||
| with(HttpClient.builder()) { | ||
| if (normalizedCaCertificates.isEmpty()) { | ||
| addDefaultCliCertificates() | ||
| } else { | ||
| for (file in normalizedCaCertificates) addCertificates(file) | ||
| } | ||
| // Lazy building significantly reduces execution time of commands that do minimal work. | ||
| // However, it means that HTTP client initialization errors won't surface until an HTTP | ||
| // request is made. | ||
| buildLazily() | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -18,24 +18,18 @@ package org.pkl.commons.cli | |||
| import java.nio.file.Path | ||||
| import java.util.regex.Pattern | ||||
| import org.pkl.core.* | ||||
| import org.pkl.core.http.HttpClient | ||||
| import org.pkl.core.module.ModuleKeyFactories | ||||
| import org.pkl.core.module.ModuleKeyFactory | ||||
| import org.pkl.core.module.ModulePathResolver | ||||
| import org.pkl.core.project.Project | ||||
| import org.pkl.core.resource.ResourceReader | ||||
| import org.pkl.core.resource.ResourceReaders | ||||
| import org.pkl.core.runtime.CertificateUtils | ||||
| import org.pkl.core.settings.PklSettings | ||||
| import org.pkl.core.util.IoUtils | ||||
|
|
||||
| /** Building block for CLI commands. Configured programmatically to allow for embedding. */ | ||||
| abstract class CliCommand(protected val cliOptions: CliBaseOptions) { | ||||
| init { | ||||
| if (cliOptions.caCertificates.isNotEmpty()) { | ||||
| CertificateUtils.setupAllX509CertificatesGlobally(cliOptions.caCertificates) | ||||
| } | ||||
| } | ||||
|
|
||||
| /** Runs this command. */ | ||||
| fun run() { | ||||
| if (cliOptions.testMode) { | ||||
|
|
@@ -158,6 +152,10 @@ abstract class CliCommand(protected val cliOptions: CliBaseOptions) { | |||
| ) | ||||
| } | ||||
|
|
||||
| // share HTTP client with other commands with the same cliOptions | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is true for the rest of the protected members of this class.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't see why. It's true for
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What I mean is that all of the protected members exist to be shared with other commands. It's a tad unncessary that this one is called out in particular as being shared.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It’s the same instance that’s shared, which is especially relevant for HttpClient and not generally the case for other protected members. |
||||
| protected val httpClient: HttpClient | ||||
| get() = cliOptions.httpClient | ||||
|
|
||||
| protected fun moduleKeyFactories(modulePathResolver: ModulePathResolver): List<ModuleKeyFactory> { | ||||
| return buildList { | ||||
| add(ModuleKeyFactories.standardLibrary) | ||||
|
|
@@ -195,6 +193,7 @@ abstract class CliCommand(protected val cliOptions: CliBaseOptions) { | |||
| .setStackFrameTransformer(stackFrameTransformer) | ||||
| .apply { project?.let { setProjectDependencies(it.dependencies) } } | ||||
| .setSecurityManager(securityManager) | ||||
| .setHttpClient(httpClient) | ||||
| .setExternalProperties(externalProperties) | ||||
| .setEnvironmentVariables(environmentVariables) | ||||
| .addModuleKeyFactories(moduleKeyFactories(modulePathResolver)) | ||||
|
|
||||
Uh oh!
There was an error while loading. Please reload this page.