Skip to content

Resource-mapper now considers content type when inserting index.html #1260

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

Closed
jaxoncreed wants to merge 1 commit into from
Closed

Resource-mapper now considers content type when inserting index.html #1260

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 1 addition & 1 deletion lib/ldp.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,7 @@ class LDP {
async get (options, searchIndex = true) {
let path, contentType, stats
try {
({ path, contentType } = await this.resourceMapper.mapUrlToFile({ url: options, searchIndex }))
({ path, contentType } = await this.resourceMapper.mapUrlToFile({ url: options, contentType: options.contentType, searchIndex }))

@RubenVerborgh RubenVerborgh Jul 11, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(See comment below first.)

Is options.contentType the value of the Accept header? Then it's not a content type, so can't be passed. But in any case, passing the content type to the resource mapper is only meaningful in the case of PUT or POST, when a new URL needs to be created; the mapper does not do content negotiation.

It seems to me that the desired logic should be implemented at this level, perhaps as follows:

  • Set searchIndex to false if the Accept header does not explicitly ask for text/html.
  • Then call mapUrlToFile.

@jaxoncreed jaxoncreed Jul 11, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah okay thanks. I'll do that

stats = await this.stat(path)
} catch (err) {
throw error(404, 'Can\'t find file requested: ' + options)
Expand Down
2 changes: 1 addition & 1 deletion lib/resource-mapper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ class ResourceMapper {
if (!isIndex) {
match = files.find(f => this._removeDollarExtension(f) === filename)
// Check if the index file exists
} else if (files.includes(this._indexFilename)) {
} else if (files.includes(this._indexFilename) && contentType && contentType.includes(this._indexContentType)) {

@RubenVerborgh RubenVerborgh Jul 11, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm. First of all, this can't be correct; the last condition will be true with text/htmlxyz whereas we probably want it to fail. So why the choice for includes here? Is contentType expected to be an Accept header? That would not be a correct use of the parameter, which is intended for a single content type. (Namely in the case of a PUT, where the client sends a Content-Type.)

However, I don't understand the intuition behind this line. What is is supposed to mean? The index file can only be served if the user agent "sends" a content type, that just so happens to be the content type of the index file?

@Ryuno-Ki Ryuno-Ki Jul 11, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Perhaps it is, because the contentType can have more values than „text/html” (IIRC a charset parameter or something). That's why an accept match isn't possible.
I had a „starts with” comparison in one of my projects, but a check against MIME types?
https://www.npmjs.com/package/@goa/content-type came after a short search for „RFC 7231”.

@RubenVerborgh RubenVerborgh Jul 11, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd be surprised if a GET could be reduced to a single content type (it cannot), but indeed, content types can have arguments.

match = this._indexFilename
}
}
Expand Down