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.

Sign up for GitHub

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

Jump to bottom

Use java.net.http.HttpClient instead of java.net.Http(s)URLConnection #217

Merged
merged 22 commits into from
Mar 6, 2024
Merged

Use java.net.http.HttpClient instead of java.net.Http(s)URLConnection #217

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
9ac214a
Use java.net.http.HttpClient instead of java.net.Http(s)URLConnection
translatenix Feb 20, 2024
dc78962
Support CA certificates in Executor API
translatenix Feb 21, 2024
903f141
Build CLI HTTP client lazily
translatenix Feb 21, 2024
1ca9814
Improve handling of HTTP client initialization errors
translatenix Feb 21, 2024
765b439
Improve SSL related error messages
translatenix Feb 21, 2024
988481f
Rename class
translatenix Feb 21, 2024
0022ea3
Rename some internals
translatenix Feb 22, 2024
0b3734d
Remove assert that is specific to JDK implementation
translatenix Feb 22, 2024
8f8ac86
Fix compile error on JDK 11
translatenix Feb 22, 2024
b12db60
Incorporate review feedback
translatenix Feb 22, 2024
a240d4c
Incorporate review feedback (more to follow later)
translatenix Feb 23, 2024
5b122a0
Incorporate more review feedback
translatenix Feb 23, 2024
ed1f6ac
Polish error message
translatenix Feb 25, 2024
e16a8e0
Make SSL error messages more robust
translatenix Feb 26, 2024
9b696b8
Improve error message on JDK 11
translatenix Feb 27, 2024
09be2f4
Publish pkl-certs JAR
translatenix Feb 27, 2024
8d78d28
Implement v2 of executor SPI
translatenix Feb 27, 2024
fdf2026
Reuse EmbeddedExecutor instances for all tests
translatenix Feb 27, 2024
cc25583
Incorporate review feedback
translatenix Feb 28, 2024
3b6e169
Change strategy for supporting certificates in pkl-executor
translatenix Feb 29, 2024
85103c6
Polish EmbeddedExecutorTest
translatenix Feb 29, 2024
06f94fd
Incorporate review feedback
translatenix Mar 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions bench/src/jmh/java/org/pkl/core/ListSort.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import org.openjdk.jmh.annotations.*;
import org.openjdk.jmh.util.TempFile;
import org.openjdk.jmh.util.TempFileManager;
import org.pkl.core.http.HttpClient;
import org.pkl.core.module.ModuleKeyFactories;
import org.pkl.core.repl.ReplRequest;
import org.pkl.core.repl.ReplResponse;
Expand All @@ -39,6 +40,7 @@ public class ListSort {
private static final ReplServer repl =
new ReplServer(
SecurityManagers.defaultManager,
HttpClient.dummyClient(),
Loggers.stdErr(),
List.of(ModuleKeyFactories.standardLibrary),
List.of(ResourceReaders.file()),
Expand Down
2 changes: 2 additions & 0 deletions docs/src/test/kotlin/DocSnippetTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import org.pkl.core.repl.ReplServer
import org.pkl.core.resource.ResourceReaders
import org.pkl.core.util.IoUtils
import org.antlr.v4.runtime.ParserRuleContext
import org.pkl.core.http.HttpClient
import java.nio.file.Files
import kotlin.io.path.isDirectory
import kotlin.io.path.isRegularFile
Expand Down Expand Up @@ -78,6 +79,7 @@ class DocSnippetTestsEngine : HierarchicalTestEngine<DocSnippetTestsEngine.Execu
override fun createExecutionContext(request: ExecutionRequest): ExecutionContext {
val replServer = ReplServer(
SecurityManagers.defaultManager,
HttpClient.dummyClient(),
Loggers.stdErr(),
listOf(
ModuleKeyFactories.standardLibrary,
Expand Down
19 changes: 19 additions & 0 deletions pkl-certs/pkl-certs.gradle.kts
View file
Open in desktop
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-cli and optionally supported by pkl-core.")
translatenix marked this conversation as resolved.
Show resolved Hide resolved
""".trimIndent())
}
}
}
}
0 .../commons/cli/commands/IncludedCARoots.pem → ...in/resources/org/pkl/certs/PklCARoots.pem
View file
Open in desktop
File renamed without changes.
2 changes: 1 addition & 1 deletion pkl-cli/pkl-cli.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ fun Exec.configureExecutable(isEnabled: Boolean, outputFile: File, extraArgs: Li
,"--no-fallback"
,"-H:IncludeResources=org/pkl/core/stdlib/.*\\.pkl"
,"-H:IncludeResources=org/jline/utils/.*"
,"-H:IncludeResources=org/pkl/commons/cli/commands/IncludedCARoots.pem"
,"-H:IncludeResources=org/pkl/certs/PklCARoots.pem"
//,"-H:IncludeResources=org/pkl/core/Release.properties"
,"-H:IncludeResourceBundles=org.pkl.core.errorMessages"
,"--macro:truffle"
Expand Down
2 changes: 1 addition & 1 deletion pkl-cli/src/main/kotlin/org/pkl/cli/CliPackageDownloader.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ class CliPackageDownloader(
if (moduleCacheDir == null) {
throw CliException("Cannot download packages because no cache directory is specified.")
}
val packageResolver = PackageResolver.getInstance(securityManager, moduleCacheDir)
val packageResolver = PackageResolver.getInstance(securityManager, httpClient, moduleCacheDir)
val errors = mutableMapOf<PackageUri, Throwable>()
for (pkg in packageUris) {
try {
Expand Down
1 change: 1 addition & 0 deletions pkl-cli/src/main/kotlin/org/pkl/cli/CliProjectPackager.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ class CliProjectPackager(
outputPath,
stackFrameTransformer,
securityManager,
httpClient,
skipPublishCheck,
consoleWriter
)
Expand Down
1 change: 1 addition & 0 deletions pkl-cli/src/main/kotlin/org/pkl/cli/CliProjectResolver.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ class CliProjectResolver(
SecurityManagers.defaultTrustLevels,
rootDir
),
httpClient,
moduleCacheDir
)
val dependencies = ProjectDependenciesResolver(project, packageResolver, errWriter).resolve()
Expand Down
1 change: 1 addition & 0 deletions pkl-cli/src/main/kotlin/org/pkl/cli/CliRepl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ internal class CliRepl(private val options: CliEvaluatorOptions) : CliCommand(op
SecurityManagers.defaultTrustLevels,
rootDir
),
httpClient,
Loggers.stdErr(),
listOf(
ModuleKeyFactories.standardLibrary,
Expand Down
2 changes: 1 addition & 1 deletion pkl-cli/src/main/kotlin/org/pkl/cli/CliServer.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import org.pkl.server.Server
class CliServer(options: CliBaseOptions) : CliCommand(options) {
override fun doRun() =
try {
val server = Server(MessageTransports.stream(System.`in`, System.out))
val server = Server(MessageTransports.stream(System.`in`, System.out), httpClient)
server.use { it.start() }
} catch (e: ProtocolException) {
throw CliException(e.message!!)
Expand Down
58 changes: 45 additions & 13 deletions pkl-cli/src/test/kotlin/org/pkl/cli/CliEvaluatorTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,19 +21,18 @@ import java.net.URI
import java.nio.file.Files
import java.nio.file.Path
import java.time.Duration
import kotlin.io.path.createDirectories
import kotlin.io.path.listDirectoryEntries
import kotlin.io.path.*
import org.assertj.core.api.Assertions.assertThat
import org.assertj.core.api.Assertions.assertThatCode
import org.junit.jupiter.api.AfterEach
import org.junit.jupiter.api.Test
import org.junit.jupiter.api.assertThrows
import org.junit.jupiter.api.io.TempDir
import org.junit.jupiter.params.ParameterizedTest
import org.junit.jupiter.params.provider.EnumSource
import org.pkl.commons.*
import org.pkl.commons.cli.CliBaseOptions
import org.pkl.commons.cli.CliException
import org.pkl.commons.cli.commands.BaseOptions
import org.pkl.commons.test.FileTestUtils
import org.pkl.commons.test.PackageServer
import org.pkl.core.OutputFormat
Expand Down Expand Up @@ -1158,8 +1157,46 @@ result = someLib.x
}

@Test
fun `not including the self signed certificate will result in a error`() {
fun `gives decent error message if certificate file contains random text`() {
val certsFile = tempDir.writeFile("random.pem", "RANDOM")
val err = assertThrows<CliException> { evalModuleThatImportsPackage(certsFile) }
assertThat(err)
.hasMessageContaining("Error parsing CA certificate file `${certsFile.pathString}`:")
.hasMessageContaining("No certificate data found")
.hasMessageNotContainingAny("java.", "sun.") // class names have been filtered out
}

@Test
fun `gives decent error message if certificate file is emtpy`(@TempDir tempDir: Path) {
val emptyCerts = tempDir.writeEmptyFile("empty.pem")
val err = assertThrows<CliException> { evalModuleThatImportsPackage(emptyCerts) }
assertThat(err).hasMessageContaining("CA certificate file `${emptyCerts.pathString}` is empty.")
}

@Test
fun `gives decent error message if certificate cannot be parsed`(@TempDir tempDir: Path) {
val invalidCerts = FileTestUtils.writeCertificateWithMissingLines(tempDir)
val err = assertThrows<CliException> { evalModuleThatImportsPackage(invalidCerts) }
assertThat(err)
// no assert for detail message because it differs between JDK implementations
.hasMessageContaining("Error parsing CA certificate file `${invalidCerts.pathString}`:")
.hasMessageNotContainingAny("java.", "sun.") // class names have been filtered out
}

@Test
fun `gives decent error message if CLI doesn't have the required CA certificate`() {
PackageServer.ensureStarted()
// provide SOME certs to prevent CliEvaluator from falling back to ~/.pkl/cacerts
val builtInCerts = FileTestUtils.writePklBuiltInCertificates(tempDir)
val err = assertThrows<CliException> { evalModuleThatImportsPackage(builtInCerts) }
assertThat(err)
// on some JDK11's this doesn't cause SSLHandshakeException but some other SSLException
// .hasMessageContaining("Error during SSL handshake with host `localhost`:")
.hasMessageContaining("unable to find valid certification path to requested target")
.hasMessageNotContainingAny("java.", "sun.") // class names have been filtered out
}

private fun evalModuleThatImportsPackage(certsFile: Path) {
val moduleUri =
writePklFile(
"test.pkl",
Expand All @@ -1168,22 +1205,17 @@ result = someLib.x

res = Swallow
"""
.trimIndent()
)
val buffer = StringWriter()

val options =
CliEvaluatorOptions(
CliBaseOptions(
sourceModules = listOf(moduleUri),
workingDir = tempDir,
moduleCacheDir = tempDir,
noCache = true,
// ensure we override any previously set root cert to the default buundle.
caCertificates = listOf(BaseOptions.Companion.includedCARootCerts())
caCertificates = listOf(certsFile),
noCache = true
),
)
val err = assertThrows<CliException> { CliEvaluator(options, consoleWriter = buffer).run() }
assertThat(err.message).contains("unable to find valid certification path to requested target")
CliEvaluator(options).run()
}

private fun writePklFile(fileName: String, contents: String = defaultContents): URI {
Expand Down
2 changes: 1 addition & 1 deletion pkl-cli/src/test/kotlin/org/pkl/cli/CliPackageDownloaderTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ class CliPackageDownloaderTest {

Failed to download package://bogus.domain/notAPackage@1.0.0 because:
Exception when making request `GET https://bogus.domain/notAPackage@1.0.0`:
bogus.domain
Error connecting to host `bogus.domain`.

"""
.trimIndent()
Expand Down
13 changes: 8 additions & 5 deletions pkl-cli/src/test/kotlin/org/pkl/cli/CliProjectPackagerTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ import org.pkl.commons.readString
import org.pkl.commons.test.FileTestUtils
import org.pkl.commons.test.PackageServer
import org.pkl.commons.writeString
import org.pkl.core.runtime.CertificateUtils

class CliProjectPackagerTest {
@Test
Expand Down Expand Up @@ -868,7 +867,6 @@ class CliProjectPackagerTest {
@Test
fun `publish checks`(@TempDir tempDir: Path) {
PackageServer.ensureStarted()
CertificateUtils.setupAllX509CertificatesGlobally(listOf(FileTestUtils.selfSignedCertificate))
tempDir.writeFile("project/main.pkl", "res = 1")
tempDir.writeFile(
"project/PklProject",
Expand All @@ -888,7 +886,10 @@ class CliProjectPackagerTest {
val e =
assertThrows<CliException> {
CliProjectPackager(
CliBaseOptions(workingDir = tempDir),
CliBaseOptions(
workingDir = tempDir,
caCertificates = listOf(FileTestUtils.selfSignedCertificate)
),
listOf(tempDir.resolve("project")),
CliTestOptions(),
".out/%{name}@%{version}",
Expand All @@ -912,7 +913,6 @@ class CliProjectPackagerTest {
@Test
fun `publish check when package is not yet published`(@TempDir tempDir: Path) {
PackageServer.ensureStarted()
CertificateUtils.setupAllX509CertificatesGlobally(listOf(FileTestUtils.selfSignedCertificate))
tempDir.writeFile("project/main.pkl", "res = 1")
tempDir.writeFile(
"project/PklProject",
Expand All @@ -930,7 +930,10 @@ class CliProjectPackagerTest {
)
val out = StringWriter()
CliProjectPackager(
CliBaseOptions(workingDir = tempDir),
CliBaseOptions(
workingDir = tempDir,
caCertificates = listOf(FileTestUtils.selfSignedCertificate)
),
listOf(tempDir.resolve("project")),
CliTestOptions(),
".out/%{name}@%{version}",
Expand Down
2 changes: 2 additions & 0 deletions pkl-cli/src/test/kotlin/org/pkl/cli/repl/ReplMessagesTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import org.pkl.commons.toPath
import org.pkl.core.Loggers
import org.pkl.core.SecurityManagers
import org.pkl.core.StackFrameTransformers
import org.pkl.core.http.HttpClient
import org.pkl.core.module.ModuleKeyFactories
import org.pkl.core.repl.ReplRequest
import org.pkl.core.repl.ReplResponse
Expand All @@ -30,6 +31,7 @@ class ReplMessagesTest {
private val server =
ReplServer(
SecurityManagers.defaultManager,
HttpClient.dummyClient(),
Loggers.stdErr(),
listOf(ModuleKeyFactories.standardLibrary),
listOf(),
Expand Down
1 change: 1 addition & 0 deletions pkl-commons-cli/pkl-commons-cli.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ dependencies {

implementation(projects.pklCommons)
testImplementation(projects.pklCommonsTest)
runtimeOnly(projects.pklCerts)
}

publishing {
Expand Down
35 changes: 29 additions & 6 deletions pkl-commons-cli/src/main/kotlin/org/pkl/commons/cli/CliBaseOptions.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
bioball marked this conversation as resolved.
Show resolved Hide resolved
* `~/.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 {
Expand Down Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's java.lang.AutoCloseable, so I think any recommendation should favour try-with-resources patterns, no?

Copy link
Contributor Author

@translatenix translatenix Feb 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I didn't change CliBaseOptions to implement AutoCloseable. Usually, an options object shouldn't need to be closed. Adding CliBaseOptions.httpClient felt a bit smelly, but it seemed like a price worth paying for being able to share HttpClient resources between commands. I couldn't find another way to achieve this without a major redesign.

I documented calling CliBaseOptions.httpClient.close() for the edge cases where it might matter. That's also why I made httpClient public.

Let me know if you want me to change CliBaseOptions to implement AutoCloseable. Kotlin doesn't have try-with-resources, but it does have a Closeable.use extension method.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think CliBaseOptions implementing AutoCloseable is the right call.

We have a couple places where our builders accept Closeable, but we also try to avoid it whenever we can.

Probably makes more sense to either accept the options on HttpClient.Builder directly, or accept HttpClient.Builder as an option as a whole.

Same comment for the settings on EvaluatorBuilder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably makes more sense to either accept the options on HttpClient.Builder directly, or accept HttpClient.Builder as an option as a whole.

Sorry I don't understand what you're proposing. Can you elaborate?
I considered passing HttpClient to CliCommand, but then CliBaseOptions.caCertificates is no longer meaningful. (It's HttpClient that needs to be configured with the certificates.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably makes more sense to either accept the options on HttpClient.Builder directly, or accept HttpClient.Builder as an option as a whole.

Sorry but I don't follow. Can you elaborate on what you're proposing?

One option I considered was to pass HttpClient to constructors of CliCommand subclasses. But this doesn't really work because it makes CliBaseOptions.caCertificates irrelevant. (It's the HttpClient that needs to be configured with certificates.)

Copy link
Contributor

@bioball bioball Feb 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, so, one suggestion is to add the builder methods on HttpClient.Builder to EvaluatorBuilder:

class EvaluatorBuilder {
  public EvaluatorBuilder addCertificates(Path file) { /* etc */ }

  public EvaluatorBuilder addCertificates(URI file) { /* etc */ }

Another option is to accept an HttpClient.Builder as an option in EvaluatorBuilder:

class EvaluatorBuilder {
  public HttpClient.Builder setHttpClientBuilder(HttpClient.Builder builder) { /* etc */ }
}

Copy link
Contributor Author

@translatenix translatenix Feb 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Currently we have EvaluatorBuilder.setHttpClient(). Your proposals might make EvaluatorBuilder more convenient to use if the default HTTP client isn't good enough. But I don't see how any of this relates to the CliBaseOptions.httpClient discussion, and it's not clear to me if and what change you want there.

Copy link
Contributor

@bioball bioball Feb 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The 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 addCertificates, and possibly any other options on HttpClient.Builder.

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()
}
}
}
13 changes: 6 additions & 7 deletions pkl-commons-cli/src/main/kotlin/org/pkl/commons/cli/CliCommand.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -158,6 +152,10 @@ abstract class CliCommand(protected val cliOptions: CliBaseOptions) {
)
}

// share HTTP client with other commands with the same cliOptions
Copy link
Contributor

Choose a reason for hiding this comment

The 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
// share HTTP client with other commands with the same cliOptions

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

I don't see why. It's true for httpClient because it delegates to cliOptions.httpClient.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

@translatenix translatenix Feb 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The 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)
Expand Down Expand Up @@ -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))
Expand Down
Loading