Add support for HTTP rewrites #1062
Conversation
| @@ -178,6 +178,12 @@ abstract class CliCommand(protected val cliOptions: CliBaseOptions) { | |||
| ?: settings.http?.proxy?.noProxy | |||
| } | |||
|
|
|||
| private val httpRewrites by lazy { | |||
There was a problem hiding this comment.
| private val httpRewrites by lazy { | |
| private val httpRewrites: Map<String, String>? by lazy { |
I know we are inconsistent in this regard, but I think it's better to have the types always defined for properties unless they are absolute obvious from the value. Even if they are private.
There was a problem hiding this comment.
That's fine with me; I'll change the rest of the properties in this class too
| this.userAgent = userAgent; | ||
| this.requestTimeout = requestTimeout; | ||
| this.testPort = testPort; | ||
| this.delegate = delegate; | ||
| this.rewrites = | ||
| rewrites.entrySet().stream() | ||
| .sorted(Comparator.comparingInt((entry) -> entry.getKey().length())) |
There was a problem hiding this comment.
Sorting by length means we'd pick https://foo.bar/ over https://foo.bar/baz when both match (because findRewrite picks the first match), which seems counterintuitive to me.
I'd expect the resolution to be specific -> generic, not the other way around.
There was a problem hiding this comment.
Good point, this was a bug
| ) | ||
| } | ||
| assertThat(ex.message) | ||
| .contains("Rewrite rule must have a lowercased hostname, but was 'www.FOO.com'") |
There was a problem hiding this comment.
Couldn't we lowercase the host instead of throw?
| .multiple() | ||
| .toMap() | ||
|
|
||
| private fun OptionCallTransformContext.validateRewrite(rewrite: URI) { |
There was a problem hiding this comment.
Where is this being used? I see you are calling IoUtils.validateRewriteRule which already does validation.
There was a problem hiding this comment.
Ah, this was leftover code. Removed.
| if (settings.http() != null) { | ||
| var httpClientBuilder = HttpClient.builder(); | ||
| if (settings.http().proxy() != null) { | ||
| var noProxy = settings.http().proxy().noProxy(); |
There was a problem hiding this comment.
Were we ignoring noProxy rules before? Is this a bugfix?
| @@ -1,5 +1,5 @@ | |||
| /* | |||
| * Copyright © 2024 Apple Inc. and the Pkl project authors. All rights reserved. | |||
| * Copyright © 2024-2025 Apple Inc. and the Pkl project authors. All rights reserved. | |||
There was a problem hiding this comment.
nit: You didn't change this file.
Follows the design of apple/pkl-evolution#17