Improve static content handling#5553
Open
solonovamax wants to merge 22 commits into
Open
Conversation
Signed-off-by: solonovamax <solonovamax@12oclockpoint.com>
Improves the performance of static content slightly, by using arrays instead of lists. Signed-off-by: solonovamax <solonovamax@12oclockpoint.com>
Fixes: - Vary header was not added for static paths (KTOR-8326) Additions: - order best compression fit by file size - does not apply to resources - Reject static file request sooner if file does not exist. Previously only rejected if no other compressed file could be found, however it's better to reject sooner. Signed-off-by: solonovamax <solonovamax@12oclockpoint.com>
Signed-off-by: solonovamax <solonovamax@12oclockpoint.com>
The old behaviour was unintuitive anyways Signed-off-by: solonovamax <solonovamax@12oclockpoint.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ZSTD and DEFLATE compressed encodings; changes precompressed handling to use a nullable Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
I also have some a change which stores the contents of the default file (& all precompressed versions) in a byte array in memory, and then watch the files for changes. if you'd like, I can add these changes to the PR. |
Contributor
Author
|
Also, I tried running |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
ktor-server/ktor-server-core/jvm/src/io/ktor/server/http/content/PreCompressed.kt (2)
93-93: Simplify the awkward paired null-check on the return.Both
smallestFile/smallestType(and thePathpair) are always set together inside the loop, so only one null check is needed; the current(x ?: return null) to (y ?: return null)reads as if they can be independently null and also duplicates the return.♻️ Proposed simplification
- return (smallestFile ?: return null) to (smallestType ?: return null) + val file = smallestFile ?: return null + return file to smallestType!!Or, equivalently:
- return (smallestFile ?: return null) to (smallestType ?: return null) + return smallestFile?.let { it to smallestType!! }Also applies to: 126-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ktor-server/ktor-server-core/jvm/src/io/ktor/server/http/content/PreCompressed.kt` at line 93, The return currently does two redundant null-checks: "(smallestFile ?: return null) to (smallestType ?: return null)"; since smallestFile and smallestType (and the Path pair) are always set together, replace this with a single null check (e.g., if (smallestFile == null) return null) and then return the pair (smallestFile to smallestType), referencing the existing local names smallestFile, smallestType and the Path pair; make the same simplification for the second occurrence where the pattern repeats around the other return.
69-77: Move thecompressedTypes.isEmpty()fast path before allocating the encoding set.All three
bestCompressionFitoverloads buildacceptedEncodings(and for thecalloverload, runacceptEncoding.mapTo(HashSet(...))) before checking whether there's anything to match against. WhencompressedTypesis empty (the common default), this allocates a set for nothing.♻️ Proposed reorder (applies analogously to all three overloads)
internal fun bestCompressionFit( file: File, acceptEncoding: List<HeaderValue>, compressedTypes: Array<CompressedFileType> ): Pair<File, CompressedFileType>? { - val acceptedEncodings = acceptEncoding.mapTo(LinkedHashSet(acceptEncoding.size)) { it.value } - - // Find the smallest file in the accepted encodings - var smallestType: CompressedFileType? = null - var smallestFile: File? = null - var smallestSize: Long = Long.MAX_VALUE - - if (compressedTypes.isEmpty()) - return null + if (compressedTypes.isEmpty()) return null + + val acceptedEncodings = acceptEncoding.mapTo(LinkedHashSet(acceptEncoding.size)) { it.value } + + // Find the smallest file in the accepted encodings + var smallestType: CompressedFileType? = null + var smallestFile: File? = null + var smallestSize: Long = Long.MAX_VALUEAlso applies to: 102-110, 143-147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ktor-server/ktor-server-core/jvm/src/io/ktor/server/http/content/PreCompressed.kt` around lines 69 - 77, The code currently constructs acceptedEncodings (via acceptEncoding.mapTo(LinkedHashSet(...)) or mapTo(HashSet(...))) before checking compressedTypes.isEmpty(), causing an unnecessary allocation when there are no compressed types; move the fast-path check compressedTypes.isEmpty() to the top of each bestCompressionFit overload (the ones that build acceptedEncodings) so you return null immediately if compressedTypes is empty, then build acceptedEncodings only when compressedTypes is non-empty; apply the same reorder in the other two overloads referenced around the blocks at the other occurrences (the sections noted at 102-110 and 143-147) and update any related local vars (smallestType/smallestFile/smallestSize) initialization to remain after the early return.ktor-server/ktor-server-core/jvm/src/io/ktor/server/http/content/StaticContentResolution.kt (1)
51-68: Remove unnecessary@OptIn(InternalAPI::class).The body of
resolveResourceURLdoes not reference any@InternalAPI-annotated symbol:normalisedPathis file-private,classLoader.getResourcesis JDK, andresourceCacheis just aConcurrentHashMap. The opt-in has no effect here.♻️ Proposed cleanup
-@OptIn(InternalAPI::class) internal fun Application.resolveResourceURL( path: String, resourcePackage: String? = null, classLoader: ClassLoader = environment.classLoader ): URL? {As per coding guidelines: "Keep
@OptIn(...)scope minimal".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ktor-server/ktor-server-core/jvm/src/io/ktor/server/http/content/StaticContentResolution.kt` around lines 51 - 68, Remove the unnecessary `@OptIn`(InternalAPI::class) from the top of the resolveResourceURL function: open the function resolveResourceURL and delete the leading `@OptIn`(InternalAPI::class) annotation, since the body only uses normalisedPath, classLoader.getResources and resourceCache (none are `@InternalAPI`), leaving the function signature and implementation unchanged.ktor-server/ktor-server-core/jvm/src/io/ktor/server/http/content/StaticContent.kt (3)
554-554: Extract the repeatedstaticContentEncodedTypes?.toTypedArray() ?: emptyArray().The same list→array coercion is duplicated in five deprecated helpers (
default,file,files,resource,resources,defaultResource). Pulling it behind an internal extension reduces the noise and makes a later removal of the legacyList<CompressedFileType>attribute trivial.♻️ Sketch
private val Route.staticContentEncodedTypesArray: Array<CompressedFileType> get() = staticContentEncodedTypes?.toTypedArray() ?: emptyArray()Then replace each call site with
val compressedTypes = staticContentEncodedTypesArray.Also applies to: 578-578, 601-601, 642-642, 662-662, 682-682
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ktor-server/ktor-server-core/jvm/src/io/ktor/server/http/content/StaticContent.kt` at line 554, Create a private extension property on Route (e.g., staticContentEncodedTypesArray) that returns staticContentEncodedTypes?.toTypedArray() ?: emptyArray() and replace the duplicated expression in each deprecated helper (default, file, files, resource, resources, defaultResource) by using this property (e.g., val compressedTypes = staticContentEncodedTypesArray) so all five call sites use the shared accessor.
454-467:flattenExcludeFunctions— small style nit.The
whenexpression returns from each branch, which is fine, but the explicitreturninside the lambda parameter (exclude@{ it -> ... return@exclude true }) is redundant and hides that the last expression is the result. Alsoitcan be dropped since it's the default lambda parameter name.♻️ Proposed cleanup
-private fun <Resource : Any> List<(Resource) -> Boolean>.flattenExcludeFunctions(): (Resource) -> Boolean { - when { - isEmpty() -> return { false } - size == 1 -> return this.first() - else -> return exclude@{ it -> - for (function in this) { - if (function(it)) - return@exclude true - } - - return@exclude false - } - } -} +private fun <Resource : Any> List<(Resource) -> Boolean>.flattenExcludeFunctions(): (Resource) -> Boolean = + when (size) { + 0 -> { _ -> false } + 1 -> first() + else -> { resource -> any { it(resource) } } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ktor-server/ktor-server-core/jvm/src/io/ktor/server/http/content/StaticContent.kt` around lines 454 - 467, flattenExcludeFunctions currently uses explicit returns inside the when branches and a labeled lambda with an explicit return@exclude and explicit parameter name; simplify by returning the lambda as the last expression in the else branch without using return@exclude and omit the explicit "it" parameter (use the implicit it), so the for-loop should set a boolean result or use any { } and the final expression of the lambda should be the boolean value; target the function flattenExcludeFunctions and its else branch (the exclude@ lambda) for this cleanup.
958-958:File.separatorused for classpath-style resource paths works only incidentally.
relativePath()joins path segments withFile.separator, and at line 958 you build"$relativePath${File.separator}$index". These are then fed toresolveResourceURL/resolveResource, which normalize vianormalisedPath()(splitting on both/and\). So on Windows the request path ends up as e.g.nested\index.txt, which the normalizer rescues by splitting on\before rejoining with/.Functionally correct, but conflating OS file separators with classpath separators for resources is misleading for future readers and any code path that inspects the raw string before normalization. Consider using
/for the resource joins, and reservingFile.separatorfor the actualFile/Pathvariants.♻️ Sketch
- val indexResource = "$relativePath${File.separator}$index" + val indexResource = "$relativePath/$index"And in
respondStaticResource, consider buildingrelativePathwith/directly when the handler's resource is a URL, rather than reusing the file-orientedApplicationCall.relativePath().Also applies to: 994-1004
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ktor-server/ktor-server-core/jvm/src/io/ktor/server/http/content/StaticContent.kt` at line 958, The code builds classpath resource paths using File.separator (e.g., val indexResource = "$relativePath${File.separator}$index"), which mixes OS file separators into resource/URL paths; change those joins to use '/' instead of File.separator and ensure any construction of relativePath for URL-backed resources in respondStaticResource uses '/'-joined segments (not ApplicationCall.relativePath() which uses File.separator); update spots that feed into resolveResourceURL / resolveResource / normalisedPath() so resource path strings are composed with '/' consistently.ktor-server/ktor-server-tests/jvm/test/io/ktor/server/plugins/StaticContentTest.kt (2)
618-651: Missing regression coverage forstaticFileSystem+ multiplepreCompressed+ partial variants.The
testStaticFilesPreCompressedtest (File-based) covers the "only.gzexists" scenario withgzOnly.txt, but there is no equivalent forstaticFileSystem/staticZipwherebestCompressionFitusesPath.fileSize()instead ofFile.length(). Given the NoSuchFileException pitfall flagged inPreCompressed.kt, a test that sendsAccept-Encoding: br, gzipagainst a path where only one variant exists would have caught the issue.Consider adding, once the
PreCompressed.ktordering is fixed:`@Test` fun testStaticPathPreCompressedPartialVariants() = testApplication { // fixture has index.html.gz but no index.html.br routing { staticFileSystem("static", "jvm/test-resources/public") { preCompressed(CompressedFileType.BROTLI, CompressedFileType.GZIP) } } val response = client.get("static") { header(HttpHeaders.AcceptEncoding, "br, gzip") } assertEquals(HttpStatusCode.OK, response.status) assertEquals("gzip", response.headers[HttpHeaders.ContentEncoding]) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ktor-server/ktor-server-tests/jvm/test/io/ktor/server/plugins/StaticContentTest.kt` around lines 618 - 651, The test suite is missing a regression test for staticFileSystem/staticZip when multiple preCompressed variants exist but only one file variant is present (exposing the Path.fileSize() vs File.length() NoSuchFileException bug in PreCompressed.kt); add a new test (e.g., testStaticPathPreCompressedPartialVariants) that mounts staticFileSystem("static", "jvm/test-resources/public") and calls preCompressed(CompressedFileType.BROTLI, CompressedFileType.GZIP), then performs a GET to "static" with header Accept-Encoding: br, gzip and asserts 200 OK and Content-Encoding is "gzip"; this will exercise bestCompressionFit and ensure the code handles missing .br variant when only .gz exists and uses Path.fileSize()/File.length() correctly.
915-938: Temp directories are not cleaned up between tests.
Files.createTempDirectory("testServeEncodedFile")is called intestServeEncodedFileBr,testServeEncodedFileGz,testSuppressCompressionIfAlreadyCompressed,testCompressedTypesOrder,testPreCompressedConfiguresImperatively, andtestPreCompressedConfiguresNestedwithout a finally/afterTest that removes them. On repeated runs (especially on CI), these accumulate under the system temp dir. Not a correctness issue, but a test-hygiene nit.♻️ Sketch
val tempDir = Files.createTempDirectory("testServeEncodedFile").toFile() try { // ... existing test body } finally { tempDir.deleteRecursively() }Also applies to: 940-964
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ktor-server/ktor-server-tests/jvm/test/io/ktor/server/plugins/StaticContentTest.kt` around lines 915 - 938, The tests create temporary directories with Files.createTempDirectory("testServeEncodedFile") (e.g., in testServeEncodedFileBr, testServeEncodedFileGz, testSuppressCompressionIfAlreadyCompressed, testCompressedTypesOrder, testPreCompressedConfiguresImperatively, testPreCompressedConfiguresNested) but never delete them; wrap the test body that uses tempDir in a try/finally so the finally calls tempDir.deleteRecursively() (or equivalent cleanup) to remove the temp directory, ensuring the Files.createTempDirectory result is always cleaned up after each test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@ktor-server/ktor-server-core/jvm/src/io/ktor/server/http/content/PreCompressed.kt`:
- Around line 52-57: The headers building logic in PreCompressed (inside
Headers.build which currently calls appendFiltered(...), then
append(HttpHeaders.Vary, HttpHeaders.AcceptEncoding)) must merge with any
existing Vary header instead of unconditionally appending to avoid duplicate
Vary entries; update the code that constructs headers for PreCompressed to check
original.headers[HttpHeaders.Vary] and set Vary to either the merged value
(existing plus HttpHeaders.AcceptEncoding) or just HttpHeaders.AcceptEncoding,
while keeping the append of Content-Encoding using compressedType.encoding and
still filtering out Content-Length via appendFiltered.
- Around line 116-123: The code calls compressedPath.fileSize() before checking
compressedPath.isRegularFile(), which throws NoSuchFileException for missing
compressed variants; change the logic in the loop in PreCompressed (the branch
handling Path variants) to first check compressedPath.isRegularFile() and only
then call compressedPath.fileSize(), updating smallestType, smallestPath and
smallestSize afterwards (mirror the safe ordering used in the File overload), or
alternatively guard the fileSize() call with a try/catch that skips missing
files so missing compressed variants are ignored.
---
Nitpick comments:
In
`@ktor-server/ktor-server-core/jvm/src/io/ktor/server/http/content/PreCompressed.kt`:
- Line 93: The return currently does two redundant null-checks: "(smallestFile
?: return null) to (smallestType ?: return null)"; since smallestFile and
smallestType (and the Path pair) are always set together, replace this with a
single null check (e.g., if (smallestFile == null) return null) and then return
the pair (smallestFile to smallestType), referencing the existing local names
smallestFile, smallestType and the Path pair; make the same simplification for
the second occurrence where the pattern repeats around the other return.
- Around line 69-77: The code currently constructs acceptedEncodings (via
acceptEncoding.mapTo(LinkedHashSet(...)) or mapTo(HashSet(...))) before checking
compressedTypes.isEmpty(), causing an unnecessary allocation when there are no
compressed types; move the fast-path check compressedTypes.isEmpty() to the top
of each bestCompressionFit overload (the ones that build acceptedEncodings) so
you return null immediately if compressedTypes is empty, then build
acceptedEncodings only when compressedTypes is non-empty; apply the same reorder
in the other two overloads referenced around the blocks at the other occurrences
(the sections noted at 102-110 and 143-147) and update any related local vars
(smallestType/smallestFile/smallestSize) initialization to remain after the
early return.
In
`@ktor-server/ktor-server-core/jvm/src/io/ktor/server/http/content/StaticContent.kt`:
- Line 554: Create a private extension property on Route (e.g.,
staticContentEncodedTypesArray) that returns
staticContentEncodedTypes?.toTypedArray() ?: emptyArray() and replace the
duplicated expression in each deprecated helper (default, file, files, resource,
resources, defaultResource) by using this property (e.g., val compressedTypes =
staticContentEncodedTypesArray) so all five call sites use the shared accessor.
- Around line 454-467: flattenExcludeFunctions currently uses explicit returns
inside the when branches and a labeled lambda with an explicit return@exclude
and explicit parameter name; simplify by returning the lambda as the last
expression in the else branch without using return@exclude and omit the explicit
"it" parameter (use the implicit it), so the for-loop should set a boolean
result or use any { } and the final expression of the lambda should be the
boolean value; target the function flattenExcludeFunctions and its else branch
(the exclude@ lambda) for this cleanup.
- Line 958: The code builds classpath resource paths using File.separator (e.g.,
val indexResource = "$relativePath${File.separator}$index"), which mixes OS file
separators into resource/URL paths; change those joins to use '/' instead of
File.separator and ensure any construction of relativePath for URL-backed
resources in respondStaticResource uses '/'-joined segments (not
ApplicationCall.relativePath() which uses File.separator); update spots that
feed into resolveResourceURL / resolveResource / normalisedPath() so resource
path strings are composed with '/' consistently.
In
`@ktor-server/ktor-server-core/jvm/src/io/ktor/server/http/content/StaticContentResolution.kt`:
- Around line 51-68: Remove the unnecessary `@OptIn`(InternalAPI::class) from the
top of the resolveResourceURL function: open the function resolveResourceURL and
delete the leading `@OptIn`(InternalAPI::class) annotation, since the body only
uses normalisedPath, classLoader.getResources and resourceCache (none are
`@InternalAPI`), leaving the function signature and implementation unchanged.
In
`@ktor-server/ktor-server-tests/jvm/test/io/ktor/server/plugins/StaticContentTest.kt`:
- Around line 618-651: The test suite is missing a regression test for
staticFileSystem/staticZip when multiple preCompressed variants exist but only
one file variant is present (exposing the Path.fileSize() vs File.length()
NoSuchFileException bug in PreCompressed.kt); add a new test (e.g.,
testStaticPathPreCompressedPartialVariants) that mounts
staticFileSystem("static", "jvm/test-resources/public") and calls
preCompressed(CompressedFileType.BROTLI, CompressedFileType.GZIP), then performs
a GET to "static" with header Accept-Encoding: br, gzip and asserts 200 OK and
Content-Encoding is "gzip"; this will exercise bestCompressionFit and ensure the
code handles missing .br variant when only .gz exists and uses
Path.fileSize()/File.length() correctly.
- Around line 915-938: The tests create temporary directories with
Files.createTempDirectory("testServeEncodedFile") (e.g., in
testServeEncodedFileBr, testServeEncodedFileGz,
testSuppressCompressionIfAlreadyCompressed, testCompressedTypesOrder,
testPreCompressedConfiguresImperatively, testPreCompressedConfiguresNested) but
never delete them; wrap the test body that uses tempDir in a try/finally so the
finally calls tempDir.deleteRecursively() (or equivalent cleanup) to remove the
temp directory, ensuring the Files.createTempDirectory result is always cleaned
up after each test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c5686fa9-2f56-4d86-9b95-221751f4014a
📒 Files selected for processing (7)
ktor-server/ktor-server-core/jvm/src/io/ktor/server/http/content/PreCompressed.ktktor-server/ktor-server-core/jvm/src/io/ktor/server/http/content/StaticContent.ktktor-server/ktor-server-core/jvm/src/io/ktor/server/http/content/StaticContentResolution.ktktor-server/ktor-server-tests/jvm/test-resources/public/has-fallback.secret.txtktor-server/ktor-server-tests/jvm/test-resources/public/has-fallback.txtktor-server/ktor-server-tests/jvm/test-resources/public/ignored/index.txtktor-server/ktor-server-tests/jvm/test/io/ktor/server/plugins/StaticContentTest.kt
Signed-off-by: solonovamax <solonovamax@12oclockpoint.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@ktor-server/ktor-server-core/jvm/src/io/ktor/server/http/content/PreCompressed.kt`:
- Around line 287-317: The code currently serves a pre-compressed classpath
sibling even when the uncompressed base resource is missing; fix by resolving
the base classpath resource first and only consider bestCompressionFit if the
base exists. Concretely, call application.resolveResource(path =
requestedResource, resourcePackage = packageName, mimeResolve = contentType)
up-front (same call used in the first branch), store its result (e.g.
baseContent) and if baseContent == null do not fall through to using
bestCompressionFit; otherwise continue with the existing logic and allow
PreCompressedResponse(bestCompressionFit...) only when baseContent != null;
update any uses of modifier, cacheControl(...), provideVersions(...) to
reference baseContent/url when deciding which variant to respond with.
- Around line 232-233: The current guard in PreCompressed.kt uses
requestedPath.exists() which lets directories through; change it to check for a
regular file like the File overload does. Replace the exists() check on
requestedPath with an isRegularFile() check (so the guard reads something
equivalent to "if (!requestedPath.isRegularFile()) return") to prevent
directories or symlinked directories from being treated as files and to mirror
respondStaticFile's !requestedFile.isFile behavior.
- Around line 165-172: The replacement logic in the resolveResource lambda uses
the wrong escaping and path separator: switch Regex.escapeReplacement(...) to
Regex.escape(...) when building the Regex so the filename is safely treated as a
pattern, and replace File.separator usages with '/' (URL paths always use
forward slashes) when calling substringAfterLast on compressed and resource;
update the Regex call to use Regex.escape(compressed.substringAfterLast('/'))
and use resource.substringAfterLast('/') when constructing requestPath so
requestPath replacement works cross-platform.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 73347e33-be3e-4f4f-9fda-59b110fef9c4
📒 Files selected for processing (1)
ktor-server/ktor-server-core/jvm/src/io/ktor/server/http/content/PreCompressed.kt
Signed-off-by: solonovamax <solonovamax@12oclockpoint.com>
Changes: - Merges a couple of existing tests - Ensures behaviour is tested for all static content types Signed-off-by: solonovamax <solonovamax@12oclockpoint.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
ktor-server/ktor-server-tests/jvm/test/io/ktor/server/plugins/StaticContentTest.kt (4)
1392-1419: Minor: shadowing the outeretaginside the helper hurts readability.Line 1404 introduces
val etag = response.headers[...]insidetestCustomEtagAndLastModified, which shadows the outeretagdeclared at line 1366. The behavior is correct (the helper compares againstexpectedEtag), but renaming the local (e.g.actualEtag/headerEtag) would make the intent clearer.♻️ Optional rename
- val etag = response.headers[HttpHeaders.ETag] ?: fail("no ETag") - assertEquals(expectedEtag.quote(), etag) + val actualEtag = response.headers[HttpHeaders.ETag] ?: fail("no ETag") + assertEquals(expectedEtag.quote(), actualEtag)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ktor-server/ktor-server-tests/jvm/test/io/ktor/server/plugins/StaticContentTest.kt` around lines 1392 - 1419, The helper function testCustomEtagAndLastModified shadows the outer etag by declaring val etag = response.headers[...] — rename this local variable (e.g., actualEtag or headerEtag) so it no longer shadows the outer etag; update its usages in testCustomEtagAndLastModified (the assertEquals call comparing to expectedEtag and the fail message) to use the new variable name.
509-545:testModifier: relying on URL path tail for resource ETag is fine but slightly fragile.For
staticResources,url.path.substringAfterLast('/')works for bothfile:andjar:file:...!/public/...URLs because both end with the resource name, but it’s easy to overlook that it must continue to hold across classloader/URL implementations. A short comment noting the assumption would help future maintainers; otherwise the test is correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ktor-server/ktor-server-tests/jvm/test/io/ktor/server/plugins/StaticContentTest.kt` around lines 509 - 545, Add a brief inline comment in the testModifier function next to the staticResources modify lambda (the call that uses url.path.substringAfterLast('/')) documenting the assumption that resource URLs from both file: and jar:file: classloader forms end with the resource name; mention that this logic depends on URL.path containing the filename for both file and jar URLs and should be revisited if URL implementations change. This clarifies why url.path.substringAfterLast('/') is used and helps future maintainers understand the fragility.
42-73: Vary-header test asserts only the precompressed path; consider also asserting the uncompressed response has noContent-Encoding.The helper correctly verifies the new contract (no
Varywhen not serving a precompressed variant,Vary: Accept-Encodingwhen one is). For extra safety against regressions whereContent-Encodingcould leak onto the identity response, an additional assertion in the no-Accept-Encodingbranch would be cheap to add.♻️ Optional tightening
suspend fun testVaryHeader(path: String) { client.get(path).let { response -> assertNull(response.headers[HttpHeaders.Vary]) + assertNull(response.headers[HttpHeaders.ContentEncoding]) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ktor-server/ktor-server-tests/jvm/test/io/ktor/server/plugins/StaticContentTest.kt` around lines 42 - 73, In testResourcesVaryHeaderWithPreCompressed, extend the helper testVaryHeader to also assert that the response returned by client.get(path) without an Accept-Encoding header does not include a Content-Encoding header (i.e., assertNull(response.headers[HttpHeaders.ContentEncoding] or equivalent), while keeping the existing assertion that the Vary header is null; leave the existing branch that appends HttpHeaders.AcceptEncoding and asserts Vary: Accept-Encoding unchanged.
384-450: StrengthentestPreCompressedby assertingContent-Encodingis absent whenexpectEncodedResponse=false.When the client advertises an encoding for which no precompressed variant exists (e.g.
gzOnly.txtwithbr), the test asserts the body but never asserts thatContent-Encodingis unset. Adding that assertion would prevent silent regressions where an unrelated encoding leaks onto the identity response.♻️ Optional tightening
assertEquals(HttpStatusCode.OK, response.status) assertEquals(content, response.bodyAsText().trim()) - if (type != null && expectEncodedResponse) + if (type != null && expectEncodedResponse) { assertEquals(type.encoding, response.headers[HttpHeaders.ContentEncoding]) + } else { + assertNull(response.headers[HttpHeaders.ContentEncoding]) + } assertEquals(ContentType.Text.Plain, response.contentType()!!.withoutParameters())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ktor-server/ktor-server-tests/jvm/test/io/ktor/server/plugins/StaticContentTest.kt` around lines 384 - 450, The test helper testPreCompressed currently only checks Content-Encoding when expectEncodedResponse is true; update testPreCompressed (the suspend function named testPreCompressed) to also assert that response.headers[HttpHeaders.ContentEncoding] is null when expectEncodedResponse == false (i.e., when the client requested an encoding but no precompressed file exists), so add a branch after the existing encoding assertion that verifies absence of the Content-Encoding header when expectEncodedResponse is false to prevent leakage of unrelated encodings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@ktor-server/ktor-server-tests/jvm/test/io/ktor/server/plugins/StaticContentTest.kt`:
- Around line 1392-1419: The helper function testCustomEtagAndLastModified
shadows the outer etag by declaring val etag = response.headers[...] — rename
this local variable (e.g., actualEtag or headerEtag) so it no longer shadows the
outer etag; update its usages in testCustomEtagAndLastModified (the assertEquals
call comparing to expectedEtag and the fail message) to use the new variable
name.
- Around line 509-545: Add a brief inline comment in the testModifier function
next to the staticResources modify lambda (the call that uses
url.path.substringAfterLast('/')) documenting the assumption that resource URLs
from both file: and jar:file: classloader forms end with the resource name;
mention that this logic depends on URL.path containing the filename for both
file and jar URLs and should be revisited if URL implementations change. This
clarifies why url.path.substringAfterLast('/') is used and helps future
maintainers understand the fragility.
- Around line 42-73: In testResourcesVaryHeaderWithPreCompressed, extend the
helper testVaryHeader to also assert that the response returned by
client.get(path) without an Accept-Encoding header does not include a
Content-Encoding header (i.e.,
assertNull(response.headers[HttpHeaders.ContentEncoding] or equivalent), while
keeping the existing assertion that the Vary header is null; leave the existing
branch that appends HttpHeaders.AcceptEncoding and asserts Vary: Accept-Encoding
unchanged.
- Around line 384-450: The test helper testPreCompressed currently only checks
Content-Encoding when expectEncodedResponse is true; update testPreCompressed
(the suspend function named testPreCompressed) to also assert that
response.headers[HttpHeaders.ContentEncoding] is null when expectEncodedResponse
== false (i.e., when the client requested an encoding but no precompressed file
exists), so add a branch after the existing encoding assertion that verifies
absence of the Content-Encoding header when expectEncodedResponse is false to
prevent leakage of unrelated encodings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f247cd5e-b2ea-46a4-8350-463c5e01400b
📒 Files selected for processing (2)
ktor-server/ktor-server-tests/jvm/test-resources/public/index.html.brktor-server/ktor-server-tests/jvm/test/io/ktor/server/plugins/StaticContentTest.kt
✅ Files skipped from review due to trivial changes (1)
- ktor-server/ktor-server-tests/jvm/test-resources/public/index.html.br
Signed-off-by: solonovamax <solonovamax@12oclockpoint.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ktor-server/ktor-server-tests/jvm/test/io/ktor/server/plugins/StaticContentTest.kt (1)
414-431: StrengthentestPreCompressedhelper: assert noContent-Encodingwhen fallback to plain is expected.When
expectEncodedResponse = false(lines 437, 444, 451), the helper verifies the body content but doesn't verify thatContent-Encodingwas actually omitted. A regression that incorrectly attaches an encoding header (e.g., serving.gzasgzip-encoded but with plain body, or echoing back the requested encoding) wouldn't be caught.This is the precise behavior the PR is changing for "no precompressed candidate matches the Accept-Encoding" — worth pinning down in the test.
♻️ Proposed refactor
assertEquals(HttpStatusCode.OK, response.status) assertEquals(content, response.bodyAsText().trim()) - if (type != null && expectEncodedResponse) { - assertEquals(type.encoding, response.headers[HttpHeaders.ContentEncoding]) + if (expectEncodedResponse && type != null) { + assertEquals(type.encoding, response.headers[HttpHeaders.ContentEncoding]) + } else { + assertNull(response.headers[HttpHeaders.ContentEncoding]) } assertEquals(ContentType.Text.Plain, response.contentType()!!.withoutParameters())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ktor-server/ktor-server-tests/jvm/test/io/ktor/server/plugins/StaticContentTest.kt` around lines 414 - 431, The helper testPreCompressed currently omits asserting that Content-Encoding is absent when expectEncodedResponse is false; update testPreCompressed (the suspend function testPreCompressed) to explicitly assert that response.headers[HttpHeaders.ContentEncoding] is null or empty when expectEncodedResponse == false so regressions that incorrectly set a Content-Encoding header are caught; keep the existing checks for status, body content, and Content-Type and only add the negative Content-Encoding assertion in the branch where expectEncodedResponse is false.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@ktor-server/ktor-server-tests/jvm/test/io/ktor/server/plugins/StaticContentTest.kt`:
- Around line 414-431: The helper testPreCompressed currently omits asserting
that Content-Encoding is absent when expectEncodedResponse is false; update
testPreCompressed (the suspend function testPreCompressed) to explicitly assert
that response.headers[HttpHeaders.ContentEncoding] is null or empty when
expectEncodedResponse == false so regressions that incorrectly set a
Content-Encoding header are caught; keep the existing checks for status, body
content, and Content-Type and only add the negative Content-Encoding assertion
in the branch where expectEncodedResponse is false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e9143480-78b0-400c-9094-d5ecbb6e9277
📒 Files selected for processing (4)
ktor-server/ktor-server-core/api/ktor-server-core.apiktor-server/ktor-server-core/jvm/src/io/ktor/server/http/content/PreCompressed.ktktor-server/ktor-server-core/jvm/src/io/ktor/server/http/content/StaticContent.ktktor-server/ktor-server-tests/jvm/test/io/ktor/server/plugins/StaticContentTest.kt
✅ Files skipped from review due to trivial changes (1)
- ktor-server/ktor-server-core/jvm/src/io/ktor/server/http/content/StaticContent.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- ktor-server/ktor-server-core/jvm/src/io/ktor/server/http/content/PreCompressed.kt
Signed-off-by: solonovamax <solonovamax@12oclockpoint.com>
Signed-off-by: solonovamax <solonovamax@12oclockpoint.com>
Signed-off-by: solonovamax <solonovamax@12oclockpoint.com>
It's was deprecated in JDK 21, and is now ignored. It will eventually be removed, and this will become a NoClassDefFoundError. It already produces this error, and apparently has already been removed on some linux distros. See: - JDK-8303188 - JDK-8303175 - JDK-8303471 - JDK-8303413 - openjdk/jdk#12746 (PR which deprecates it for removal) - openjdk/jdk@ae29054 - openjdk/jdk#12795 (PR which removes the functionality) - KTOR-1647 Signed-off-by: solonovamax <solonovamax@12oclockpoint.com>
Signed-off-by: solonovamax <solonovamax@12oclockpoint.com>
Signed-off-by: solonovamax <solonovamax@12oclockpoint.com>
Adds caching for the default file system when using staticFileSystem() The default file is cached in memory, and the files are watched so they can be refreshed if they are updated. Signed-off-by: solonovamax <solonovamax@12oclockpoint.com>
Signed-off-by: solonovamax <solonovamax@12oclockpoint.com>
Signed-off-by: solonovamax <solonovamax@12oclockpoint.com>
Signed-off-by: solonovamax <solonovamax@12oclockpoint.com>
Signed-off-by: solonovamax <solonovamax@12oclockpoint.com>
Signed-off-by: solonovamax <solonovamax@12oclockpoint.com>
Signed-off-by: solonovamax <solonovamax@12oclockpoint.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Subsystem
ktor-server module
Motivation
Fixes:
Also a couple of things that I felt were lacking with the static file handling.
Solution
Changes:
test.txt.brexisted andtest.txtdid not and a request was made fortest.txtwith support for brotli compression, the request would succeed. imo, this behaviour does not make sense, so I changed it to instead only succeed iftest.txtalso exists.the reason for changing this is because this would result in clients with different
Acceptheaders having different results when requesting a resource. some would see the resource, some wouldn't.StaticContentTestFile/Path/resources), to ensure that behaviour is always tested on all three.Varyheader is applied correctly. previously, it would not get added to static content ifstaticFileSystemwas used.SensitivityWatchEventModifier.staticFileSystemstaticFileSystemwhich takesPathfor the base path & index, instead ofString. note: these overloads are ambiguous, so this change is not appropriate for a patch update.my current implementation does not check exclusion rules for the default file. I felt this was reasonable, as it would be odd that something were configured as the default but had an exclusion rule that filtered it out.
would you like for me to also always check the exclusion rule for the default file?
some remaining things I want to do before this is merged but haven't gotten the chance to:
alla lot of the tests currently use the old wayPathvariantFilevariantURL/resource variantstaticFileSystemforPathstaticPaths, which just directly callsstaticFileSystem. (I personally prefer the namestaticPathsto `staticFileSystem