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.

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

Provide a way to set fallback file extensions for FileService #5806

Merged
merged 2 commits into from
Jul 16, 2024

Conversation

ikhoon
Copy link
Contributor

@ikhoon ikhoon commented Jul 9, 2024

Motivation:

I prefer clean URL patterns without a trailing slash so /app/projects is preferred over /app/projects/. If I built /app/projects with a Javascript framework, /app/projects/index.html or /app/projects.html may be exported by the framework which is a common feature.

In FileService, /app/projects/index.html can be served by /app/projects/ path, but cannot be found by /app/projects. A trailing slash / can be converted into /index.html or an auto index page. As some fallback logics are already implemented, I didn't want to add a new fallback option for a trailing slash.

Alternatively, I propose an option that appends an extension if there is no file for the request path. For example, a request sent to /app/projects also finds /app/projects.[ext] as a fallback.

Related links:

Modifications:

  • Allow configuring fallbackFileExtensions() via FileServiceBuilder
  • Find a file with fallback extensions if missing.

Result:

  • You can now set fallback file extensions to look up files in FileService.
FileService
  .builder(rootDir)
  .fallbackFileExtensions("html", "txt")
  ...

Motivation:

I personally prefer clean URL patterns without a trailing slash.
`/app/projects` is prefered over `/app/projects/`.  If I built
`/app/projects` with a Javascript framework, `/app/projects/index.html`
or `/app/projects.html` may be exported by the framework.

In `FileService`, `/app/projects/index.html` can be served by
`/app/projects/` path, but cannot be found by `/app/projects`. Handling
trailing slash is tricky and difficult to modify.

Alternatively, I propose an option that appends an extension if there is
no file for the request path. For example, a request sent to
`/app/projects` also finds `/app/projects.[ext]` as a fallback.

Relate links:

- line#4542
- line#1655
- https://ktor.io/docs/server-static-content.html#extensions

Modifications:

- Allow configuring `fileExtensions()` via `FileServiceBuilder`
- Find a file with fallback extensions if missing.

Result:

You can now set fallback file extensions to look up files in
`FileService`.

```java
FileService
  .builder(rootDir)
  .fileExtensions("html", "txt")
  ...
```
@ikhoon ikhoon added this to the 1.30.0 milestone Jul 9, 2024
@trustin
Copy link
Member

trustin commented Jul 9, 2024

I feel like the proposed property name (fileExtensions()) doesn't convey what it actually does. Maybe fallbackFileExtensions()?

Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

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

Left a question, but overall looks good 👍

final String extension = extensionIterator.next();
return findFile(ctx, path + '.' + extension, supportedEncodings, decompress).thenCompose(file -> {
if (file != null) {
return UnmodifiableFuture.completedFuture(file);
Copy link
Contributor

@jrhee17 jrhee17 Jul 10, 2024

Choose a reason for hiding this comment

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

Question) I'm not sure if we should be redirecting to the location with the extension like done in the other location, or we can just return the file as-is.

e.g.

HttpFile.ofRedirect(path + '.' + extension);

I think either way should work, but asking just in case I'm missing anything.

I prefer clean URL patterns without a trailing slash so /app/projects is preferred over /app/projects/.

Also, if it isn't necessary to redirect for the index.html case, then I thought maybe this is solvable in a simpler way (by just adding an option to either redirect or return the file as-is)

Copy link
Contributor

Choose a reason for hiding this comment

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

Talked internally, and it seems like other web servers are also employing the redirect logic. I think the current approach is probably fine then 👍

} else {
return HttpFile.nonExistent();
// Try appending file extensions if it was a file access and file extensions are configured.
return findFileWithExtensions(ctx, fallbackExtensions.iterator(), decodedMappedPath,
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: I understood the order of 1) check extensions 2) and then check index.html is important since the objective is to avoid a trailing slash

Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

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

👍 👍 👍

Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

👍 👍 👍

@ikhoon ikhoon merged commit c208353 into line:main Jul 16, 2024
14 of 15 checks passed
@ikhoon ikhoon deleted the file-service-extension branch July 16, 2024 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow configuring implicit file extensions in FileService
4 participants