Skip to content

Conversation

jaxoncreed
Copy link
Contributor

Fix for #1228

@jaxoncreed jaxoncreed requested a review from RubenVerborgh June 21, 2019 18:29
Copy link
Contributor

@RubenVerborgh RubenVerborgh left a comment

Choose a reason for hiding this comment

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

The relative path is too prone to errors (relative to what)? It bypasses the check that uncovered this error. ResourceMapper only does absolute paths for a good reason.

getAvailablePath should take an absolute container URI, create a new URI, then ask ResourceMapper to turn that into a file path, then check whether that path exists.

Extending ResourceMapper leaks internal knowledge, which caused bugs in the past.

@michielbdejong michielbdejong mentioned this pull request Jun 24, 2019
@jaxoncreed
Copy link
Contributor Author

@RubenVerborgh I took your suggestions into account and refactored

lib/ldp.js Outdated
getAvailablePath (host, containerURI, { slug = uuid.v1(), extension }) {
const path = slug + extension
async getAvailableUrl (hostname, containerURI, { slug = uuid.v1(), extension }) {
const fileName = slug + extension
Copy link
Contributor

@RubenVerborgh RubenVerborgh Jun 26, 2019

Choose a reason for hiding this comment

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

This is going to give encoding problems.

Here's what we will need to do. We need to do all operations fully in URL space (expect for the one "does the file exist already?" check, for which we convert to file space).

  1. Construct the full URL of the target resource.
  2. Map it to a filename with mapUrlToFile.
  3. Check if the file exists.
  4. If it exists, generate a new full URL (with uuid).
  5. Return the URL.

@jaxoncreed jaxoncreed dismissed RubenVerborgh’s stale review June 26, 2019 20:33

You've approved it

Copy link
Member

@michielbdejong michielbdejong left a comment

Choose a reason for hiding this comment

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

signing this off as discussed f2f with @RubenVerborgh

@jaxoncreed jaxoncreed merged commit 362959a into master Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants