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::WriteOptions 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::WriteOptions
  • behavior tests mirroring Rust's async_write.rs test suite
  • Added the capabilities to support the options

Are there any user-facing changes?

Yes, users can now add options to their write requests

@kingsword09 kingsword09 requested a review from suyanhanx as a code owner June 21, 2025 04:17
@dosubot dosubot bot added the size:XL This PR changes 500-999 lines, ignoring generated files. label Jun 21, 2025
@kingsword09 kingsword09 marked this pull request as draft June 21, 2025 04:17
@dosubot dosubot bot added the releases-note/feat The PR implements a new feature or has a title that begins with "feat" label Jun 21, 2025
@kingsword09
Copy link
Contributor Author

kingsword09 commented Jun 21, 2025

The following attributes do not appear to be implemented yet and are inconsistent with the test case results, which will cause the test to fail:

  • if_match (It's possible that I'm not using AWS's official S3 service but a third-party adapted S3 service, and this third-party service has this issue?)
  • if_not_exists

const meta = await op.write(filenameA, contentA, { ifMatch: etagA })
assert.instanceOf(meta, Metadata)

await expect(op.write(filenameA, contentA, { ifMatch: etagB })).rejects.toThrowError('ConditionNotMatch')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ConditionNotMatch error is not thrown here, but the Metadata is returned.

image

@kingsword09
Copy link
Contributor Author

The same error occurs in the Rust tests within the core:

image

@Xuanwo
Copy link
Member

Xuanwo commented Jun 25, 2025

  • It's possible that I'm not using AWS's official S3 service but a third-party adapted S3 service, and this third-party service has this issue?

Yep, not all s3 compatible services have the same feature.

@Xuanwo
Copy link
Member

Xuanwo commented Jul 11, 2025

Thank you @kingsword09 for working on this, I think we can resolve the conflicts and get ready for merging.

@kingsword09 kingsword09 force-pushed the feat-nodejs-write-opts branch from 5d87cb4 to ea9613f Compare July 11, 2025 08:06
@kingsword09
Copy link
Contributor Author

Thank you @kingsword09 for working on this, I think we can resolve the conflicts and get ready for merging.

Merged!

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thank you @kingsword09 for working on this, really great!

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jul 11, 2025
@Xuanwo Xuanwo merged commit 1b47d42 into apache:main Jul 11, 2025
69 checks passed
@kingsword09 kingsword09 deleted the feat-nodejs-write-opts branch July 11, 2025 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm This PR has been approved by a maintainer releases-note/feat The PR implements a new feature or has a title that begins with "feat" size:XL This PR changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants