Skip to content

delete file.acl when deleting file resource (atomic delete)#1415

Merged
michielbdejong merged 3 commits into
nodeSolidServer:masterfrom
bourgeoa:atomicDelete
Jul 6, 2020
Merged

delete file.acl when deleting file resource (atomic delete)#1415
michielbdejong merged 3 commits into
nodeSolidServer:masterfrom
bourgeoa:atomicDelete

Conversation

@bourgeoa

Copy link
Copy Markdown
Member

following emerging consensus and this is an update of pull #1180 and comments from @RubenVerborgh and @kjetilk

@bourgeoa bourgeoa closed this Mar 25, 2020
@bourgeoa bourgeoa reopened this Mar 25, 2020
@bourgeoa

Copy link
Copy Markdown
Member Author

Sorry for the problems encountered with git. The pull shall be OK now

@bourgeoa bourgeoa changed the title delete file with file.acl (atomic delete) delete file.acl when deleting file resource (atomic delete) Mar 25, 2020
@jaxoncreed jaxoncreed requested a review from RubenVerborgh April 1, 2020 12:39
@jaxoncreed

Copy link
Copy Markdown
Contributor

@RubenVerborgh

@RubenVerborgh RubenVerborgh left a comment

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'm missing something probably, but not sure this works, because folder deletion is attempted before the removal of its contents.

Comment thread lib/ldp.js
Comment thread lib/ldp.js
@bourgeoa

bourgeoa commented Apr 4, 2020

Copy link
Copy Markdown
Member Author

Everything is between line 381-445
I don't how to paste with line numbering

async delete (url) {
    // First check if the path points to a valid file
    let path, stats
    try {
      ({ path } = await this.resourceMapper.mapUrlToFile({ url }))
      stats = await this.stat(path)
    } catch (err) {
      throw error(404, "Can't find " + err)
    }

    // If so, delete the directory or file
    if (stats.isDirectory()) {
      return this.deleteContainer(path)
    } else {
      return this.deleteResource(path)
    }
  }

  async deleteContainer (directory) {
    if (directory[ directory.length - 1 ] !== '/') {
      directory += '/'
    }

    // Ensure the container exists
    let list
    try {
      list = await promisify(fs.readdir)(directory)
    } catch (err) {
      throw error(404, 'The container does not exist')
    }

    // Ensure the container is empty (we ignore .meta and .acl)
    if (list.some(file => !file.endsWith(this.suffixMeta) && !file.endsWith(this.suffixAcl))) {
      throw error(409, 'Container is not empty')
    }

    // Delete the directory recursively
    try {
      await promisify(rimraf)(directory)
    } catch (err) {
      throw error(err, 'Failed to delete the container')
    }
  }

  // delete resource with acl and meta links
  async deleteResource (path) {
    const linkPath = this.resourceMapper._removeDollarExtension(path)
    try {
      // first delete file, then links with write permission only (atomic delete)
      await withLock(path, { mustExist: false }, () => promisify(fs.unlink)(path))

      const aclPath = `${linkPath}${this.suffixAcl}`
      if (await promisify(fs.exists)(aclPath)) {
        await withLock(aclPath, { mustExist: false }, () => promisify(fs.unlink)(aclPath))
      }
      const metaPath = `${linkPath}${this.suffixMeta}`
      if (await promisify(fs.exists)(metaPath)) {
        await withLock(metaPath, { mustExist: false }, () => promisify(fs.unlink)(metaPath))
      }
    } catch (err) {
      debug.container('DELETE -- unlink() error: ' + err)
      throw error(err, 'Failed to delete resource')
    }
  }

@Ryuno-Ki

Ryuno-Ki commented Apr 4, 2020

Copy link
Copy Markdown

I don't how to paste with line numbering
That's described in https://github.blog/changelog/2019-10-01-multi-line-references-and-comments-beta/

When you then paste the link (https://github.com/solid/node-solid-server/blob/9e45888f223b470ac143ef091f6ecdcb6b3a55d5/lib/ldp.js#L381-L423), GitHub will turn it into:

https://github.com/solid/node-solid-server/blob/9e45888f223b470ac143ef091f6ecdcb6b3a55d5/lib/ldp.js#L381-L423

@bourgeoa

bourgeoa commented Apr 5, 2020

Copy link
Copy Markdown
Member Author

@RubenVerborgh @jaxoncreed
I renamed deleteResource() to deleteDocument()

One test fails and only on node.js/lts (the 3 others are OK), it is not related to delete, but POST (multipart)

1 failing
  1) HTTP APIs
       POST (multipart)
         should create as many files as the ones passed in multipart:
     Error: Either the size (remote/local) don't match or files are not stored
      at Test.<anonymous> (test/integration/http-test.js:853:27)
      at Test.assert (node_modules/supertest/lib/test.js:181:6)
      at Server.localAssert (node_modules/supertest/lib/test.js:131:12)
      at emitCloseNT (net.js:1653:8)
      at processTicksAndRejections (internal/process/task_queues.js:83:21)

@bourgeoa

Copy link
Copy Markdown
Member Author

@jaxoncreed @RubenVerborgh @kjetilk
I saw new version 5.2.4

This PR having been reviewed and amended I thought he could have been included. It is linked to PR #1410 which has been merged. This bug caused a lot of harm

@RubenVerborgh

Copy link
Copy Markdown
Contributor

I thought he could have been included.

There's things such as urgent security patches 🙂
I'm sure @jaxoncreed will put it in ASAP.

@michielbdejong michielbdejong left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

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.

5 participants