Skip to content

use of container representation should not change container headers#1643

Closed
bourgeoa wants to merge 4 commits into
mainfrom
issue#1635
Closed

use of container representation should not change container headers#1643
bourgeoa wants to merge 4 commits into
mainfrom
issue#1635

Conversation

@bourgeoa
Copy link
Copy Markdown
Member

@bourgeoa bourgeoa commented Dec 13, 2021 •
edited
Loading

resolves Issue #1387

@bourgeoa bourgeoa self-assigned this Dec 13, 2021
@bourgeoa bourgeoa added the bug label Dec 13, 2021
TallTed
TallTed reviewed Dec 13, 2021
View reviewed changes
Comment thread test/integration/http-test.js Outdated
@bourgeoa @TallTed
Update test/integration/http-test.js
0926c73 
Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
@bourgeoa
Copy link
Copy Markdown
Member Author

bourgeoa commented Dec 13, 2021

@TallTed Thanks

@bourgeoa bourgeoa changed the title use of container representation do not change container headers use of container representation should not change container headers Dec 20, 2021
@bourgeoa bourgeoa requested a review from jeff-zucker December 22, 2021 13:42
jeff-zucker
jeff-zucker reviewed Dec 22, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@jeff-zucker jeff-zucker left a comment

Choose a reason for hiding this comment

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

This makes sense for URLs in the form https://user.example.com/ but not for https://example.com/. The second one is not a pim:Storage, is it?

@jeff-zucker
Copy link
Copy Markdown
Member

jeff-zucker commented Dec 22, 2021

Also, wouldn't this violate the semantics of pim:Storage? AFAIK you can never have one Storage inside another Storage. If the server root is a Storage than all the user Storages are inside it.

@bourgeoa
Copy link
Copy Markdown
Member Author

bourgeoa commented Dec 22, 2021

This makes sense for URLs in the form https://user.example.com/ but not for https://example.com/. The second one is not a pim:Storage, is it?

Agreed I opened an issue. I did not had a solution to find the provider Uri.

Also, wouldn't this violate the semantics of pim:Storage? AFAIK you can never have one Storage inside another Storage. If the server root is a Storage than all the user Storages are inside it.

No because https://user.example.com/ is not a container of https://example.com

@bourgeoa
Copy link
Copy Markdown
Member Author

bourgeoa commented Dec 22, 2021

@jeff-zucker Anyway this PR is not about pim:storage, but only container representation. See files changed

@jeff-zucker
Copy link
Copy Markdown
Member

jeff-zucker commented Dec 22, 2021

@jeff-zucker Anyway this PR is not about pim:storage, but only container representation. See files changed

Then I am afraid I do not understand the PR at all. What do you mean by container representation? Doesn't this line mark the root folder as pim:Storage?

if (req.path === '/' && req.app.locals.host.serverUri.contains(req.hostname)) fileMetadata.isStorage = true

@bourgeoa
Copy link
Copy Markdown
Member Author

bourgeoa commented Dec 22, 2021

The resulting PR is only one line and a test

image

@bourgeoa
Copy link
Copy Markdown
Member Author

bourgeoa commented Dec 26, 2021

merged with PR #1646

@bourgeoa bourgeoa closed this Dec 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TallTed TallTed TallTed left review comments

@jeff-zucker jeff-zucker jeff-zucker left review comments

Assignees

@bourgeoa bourgeoa

Labels

bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@bourgeoa @jeff-zucker @TallTed