Skip to content

Conversation

@kingsword09
Copy link
Contributor

Which issue does this PR close?

Related to #6281.

Rationale for this change

This PR adds support for opendal::options::DeleteOptions conversion in the nodejs bindings. This is part of the migration to the new options API outlined in RFC-6213 (#6213).

What changes are included in this PR?

  • Added a complete mapping and conversion of opendal::options::DeleteOptions
  • behavior tests mirroring Rust's async_delete.rs test suite
  • Added the capabilities to support the options

Are there any user-facing changes?

Yes, users can now add options to their delete requests

@kingsword09 kingsword09 requested a review from suyanhanx as a code owner June 30, 2025 15:56
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. releases-note/feat The PR implements a new feature or has a title that begins with "feat" labels Jun 30, 2025
@Xuanwo Xuanwo requested a review from Copilot July 4, 2025 10:18
@Xuanwo
Copy link
Member

Xuanwo commented Jul 4, 2025

Hi, this PR LGTM to me. I will let Copilot in to see how it works.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds support for versioned delete options in the Node.js bindings and updates tests and capabilities accordingly.

  • Defines a N-API DeleteOptions struct with conversion to the Rust opendal::options::DeleteOptions.
  • Extends Operator methods (delete/delete_sync) to accept optional delete options.
  • Introduces delete_with_version capability and adds corresponding async/sync test suites.

Reviewed Changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
bindings/nodejs/src/options.rs Added N-API DeleteOptions struct and From impl
bindings/nodejs/src/lib.rs Updated delete/delete_sync to accept delete options
bindings/nodejs/src/capability.rs Added delete_with_version getter
bindings/nodejs/tests/suites/asyncDeleteOptions.suite.mjs New async delete options tests
bindings/nodejs/tests/suites/syncDeleteOptions.suite.mjs New sync delete options tests
bindings/nodejs/tests/suites/index.mjs Registered the new delete options test suites
Comments suppressed due to low confidence (5)

bindings/nodejs/tests/suites/syncDeleteOptions.suite.mjs:31

  • The suite title is incorrect for synchronous tests. Rename 'async delete options' to 'sync delete options' to reflect the correct context.
  describe.runIf(capability.write && capability.delete)('async delete options', () => {

bindings/nodejs/tests/suites/syncDeleteOptions.suite.mjs:88

  • This test does not assert the outcome of deleting with a non-existing version. Consider adding an assertion (e.g., assert.isTrue(op.existsSync(filename2))) to verify the file was not deleted.
      op.deleteSync(filename2, { version })

bindings/nodejs/tests/suites/asyncDeleteOptions.suite.mjs:88

  • There is no assertion confirming that the file remains after a delete with a non-matching version. Add something like assert.isTrue(await op.exists(filename2)) after this line.
      await op.delete(filename2, { version })

bindings/nodejs/tests/suites/syncDeleteOptions.suite.mjs:81

  • This references filename, which is undefined in this scope. It should use filename1 to retrieve the metadata.
      const meta = op.statSync(filename)

bindings/nodejs/tests/suites/asyncDeleteOptions.suite.mjs:81

  • filename is undefined here; you likely meant filename1 to get the metadata for the first file.
      const meta = await op.stat(filename)

@kingsword09 kingsword09 force-pushed the feat-nodejs-delete-opts branch from fd7c3ff to ecedccc Compare July 4, 2025 11:36
@kingsword09 kingsword09 requested a review from Xuanwo July 4, 2025 11:51
@kingsword09 kingsword09 force-pushed the feat-nodejs-delete-opts branch from ecedccc to cb4b551 Compare July 4, 2025 11:56
@Xuanwo
Copy link
Member

Xuanwo commented Jul 11, 2025

Thank you @kingsword09 for working on this!

@Xuanwo Xuanwo merged commit 8722caf into apache:main Jul 11, 2025
69 checks passed
@kingsword09 kingsword09 deleted the feat-nodejs-delete-opts branch July 11, 2025 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

releases-note/feat The PR implements a new feature or has a title that begins with "feat" size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants