Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(nodejs): add WriteOptions for write methods #4785

Merged
merged 4 commits into from
Jun 24, 2024
Merged

feat(nodejs): add WriteOptions for write methods #4785

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e61eb95
feat(nodejs): add `OpWriteOptions` properties for write methods
bxb100 Jun 23, 2024
5f1ab79
chore
bxb100 Jun 23, 2024
2311bce
chore
bxb100 Jun 24, 2024
fb83098
refactor: remove trait and rename
bxb100 Jun 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions bindings/nodejs/.prettierignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,6 @@ generated.js
generated.d.ts
.yarn
pnpm-lock.yaml
.devbox
Xuanwo marked this conversation as resolved.
Show resolved Hide resolved
devbox.json
devbox.lock
41 changes: 37 additions & 4 deletions bindings/nodejs/generated.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,35 @@ export interface ListOptions {
limit?: number
recursive?: boolean
}
export interface OpWriteOptions {
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for you effort. But OpXxx is not an existing concept at the binding side. How about using WriteOptions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMHO, the OpWriteOptions concept corresponds to OpWrite, so I think it's better to use one object not OpWriteOptions, OpWriterOptions

the name starts with Op also because of this, but WriteOptions is ok for me

Copy link
Member

Choose a reason for hiding this comment

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

IMHO, the OpWriteOptions concept corresponds to OpWrite, so I think it's better to use one object not OpWriteOptions, OpWriterOptions

Your understanding is correct. But there is a gap between rust core and nodejs binding.

Rust Core users have a concept called OpXxx like OpWrite and OpRead. But we never expose those APIs to nodejs binding. So from the nodejs users view, they don't know about OpXxx thus they can't understand what's OpXxx represents for.

So I think it's better to use WriteOptions without the Op prefix.

And for WriteOptions and WriterOptions, it's possible for the future to introduce different options inside write and writer options. So I want to seperate them for now instead of another breaking changes in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, that's a comprehensive explanation, I will fix it ASAP

/**
* Append bytes into path.
*
* ### Notes
*
* - It always appends content to the end of the file.
* - It will create file if the path not exists.
*/
append?: boolean
/**
* Set the chunk of op.
*
* If chunk is set, the data will be chunked by the underlying writer.
*
* ## NOTE
*
* Service could have their own minimum chunk size while perform write
* operations like multipart uploads. So the chunk size may be larger than
* the given buffer size.
*/
chunk?: bigint
/** Set the [Content-Type](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Type) of op. */
contentType?: string
/** Set the [Content-Disposition](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Disposition) of op. */
contentDisposition?: string
/** Set the [Cache-Control](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control) of op. */
cacheControl?: string
}
/** PresignedRequest is a presigned request return by `presign`. */
export interface PresignedRequest {
/** HTTP method of this request. */
Expand Down Expand Up @@ -265,21 +294,23 @@ export class Operator {
* await op.write("path/to/file", Buffer.from("hello world"));
* // or
* await op.write("path/to/file", "hello world");
* // or
* await op.write("path/to/file", Buffer.from("hello world"), { contentType: "text/plain" });
* ```
*/
write(path: string, content: Buffer | string): Promise<void>
write(path: string, content: Buffer | string, options?: OpWriteOptions | undefined | null): Promise<void>
/**
* Write multiple bytes into path.
*
* It could be used to write large file in a streaming way.
*/
writer(path: string): Promise<Writer>
writer(path: string, options?: OpWriteOptions | undefined | null): Promise<Writer>
/**
* Write multiple bytes into path synchronously.
*
* It could be used to write large file in a streaming way.
*/
writerSync(path: string): BlockingWriter
writerSync(path: string, options?: OpWriteOptions | undefined | null): BlockingWriter
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we need WriterOptions too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as I mentioned above, I think WriteOptions is enough

/**
* Write bytes into path synchronously.
*
Expand All @@ -288,9 +319,11 @@ export class Operator {
* op.writeSync("path/to/file", Buffer.from("hello world"));
* // or
* op.writeSync("path/to/file", "hello world");
* // or
* op.writeSync("path/to/file", Buffer.from("hello world"), { contentType: "text/plain" });
* ```
*/
writeSync(path: string, content: Buffer | string): void
writeSync(path: string, content: Buffer | string, options?: OpWriteOptions | undefined | null): void
/**
* Append bytes into path.
*
Expand Down
202 changes: 182 additions & 20 deletions bindings/nodejs/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,9 @@
#[macro_use]
extern crate napi_derive;

mod capability;

use std::collections::HashMap;
use std::fmt::Display;
use std::future::Future;
use std::io::Read;
use std::str::FromStr;
use std::time::Duration;
Expand All @@ -30,6 +29,11 @@ use futures::AsyncReadExt;
use futures::TryStreamExt;
use napi::bindgen_prelude::*;

use opendal::operator_functions::{FunctionWrite, FunctionWriter};
Copy link
Member

Choose a reason for hiding this comment

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

It's better not to play with those APIs. What do you think about adding bunch of if-else directly?

This PR also makes me thinking about exporting WriteOptions from the rust core side. We can have a discussion there.

Copy link
Contributor Author

@bxb100 bxb100 Jun 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

you mean this?

         let mut writer = self.0.write_with(&path, c);
         if let Some(options) = options {
-            writer = writer.with(options);
+            if let Some(append) = options.append {
+                writer = writer.append(append);
+            }
+            if let Some(chunk) = options.chunk {
+                writer = writer.chunk(chunk.get_u64().1 as usize);
+            }
+            if let Some(ref content_type) = options.content_type {
+                writer = writer.content_type(content_type);
+            }
+            if let Some(ref content_disposition) = options.content_disposition {
+                writer = writer.content_disposition(content_disposition);
+            }
+            if let Some(ref cache_control) = options.cache_control {
+                writer = writer.cache_control(cache_control);
+            }
         }
         writer.await.map_err(format_napi_error)
     }

Copy link
Member

Choose a reason for hiding this comment

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

Yes!

Copy link
Member

@Xuanwo Xuanwo Jun 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We can polish here in the future if rust core exposes WriteOptions directly.

use opendal::operator_futures::{FutureWrite, FutureWriter};

mod capability;

#[napi]
pub struct Operator(opendal::Operator);

Expand Down Expand Up @@ -233,31 +237,54 @@ impl Operator {
/// await op.write("path/to/file", Buffer.from("hello world"));
/// // or
/// await op.write("path/to/file", "hello world");
/// // or
/// await op.write("path/to/file", Buffer.from("hello world"), { contentType: "text/plain" });
/// ```
#[napi]
pub async fn write(&self, path: String, content: Either<Buffer, String>) -> Result<()> {
pub async fn write(
&self,
path: String,
content: Either<Buffer, String>,
options: Option<OpWriteOptions>,
) -> Result<()> {
let c = match content {
Either::A(buf) => buf.as_ref().to_owned(),
Either::B(s) => s.into_bytes(),
};
self.0.write(&path, c).await.map_err(format_napi_error)
let mut writer = self.0.write_with(&path, c);
if let Some(options) = options {
writer = writer.with(options);
}
writer.await.map_err(format_napi_error)
}

/// Write multiple bytes into path.
///
/// It could be used to write large file in a streaming way.
#[napi]
pub async fn writer(&self, path: String) -> Result<Writer> {
let w = self.0.writer(&path).await.map_err(format_napi_error)?;
pub async fn writer(&self, path: String, options: Option<OpWriteOptions>) -> Result<Writer> {
let mut writer = self.0.writer_with(&path);
if let Some(options) = options {
writer = writer.with(options);
}
let w = writer.await.map_err(format_napi_error)?;
Ok(Writer(w))
}

/// Write multiple bytes into path synchronously.
///
/// It could be used to write large file in a streaming way.
#[napi]
pub fn writer_sync(&self, path: String) -> Result<BlockingWriter> {
let w = self.0.blocking().writer(&path).map_err(format_napi_error)?;
pub fn writer_sync(
&self,
path: String,
options: Option<OpWriteOptions>,
) -> Result<BlockingWriter> {
let mut writer = self.0.blocking().writer_with(&path);
if let Some(options) = options {
writer = writer.with(options);
}
let w = writer.call().map_err(format_napi_error)?;
Ok(BlockingWriter(w))
}

Expand All @@ -268,14 +295,25 @@ impl Operator {
/// op.writeSync("path/to/file", Buffer.from("hello world"));
/// // or
/// op.writeSync("path/to/file", "hello world");
/// // or
/// op.writeSync("path/to/file", Buffer.from("hello world"), { contentType: "text/plain" });
/// ```
#[napi]
pub fn write_sync(&self, path: String, content: Either<Buffer, String>) -> Result<()> {
pub fn write_sync(
&self,
path: String,
content: Either<Buffer, String>,
options: Option<OpWriteOptions>,
) -> Result<()> {
let c = match content {
Either::A(buf) => buf.as_ref().to_owned(),
Either::B(s) => s.into_bytes(),
};
self.0.blocking().write(&path, c).map_err(format_napi_error)
let mut writer = self.0.blocking().write_with(&path, c);
if let Some(options) = options {
writer = writer.with(options);
}
writer.call().map_err(format_napi_error)
}

/// Append bytes into path.
Expand All @@ -293,16 +331,15 @@ impl Operator {
/// ```
#[napi]
pub async fn append(&self, path: String, content: Either<Buffer, String>) -> Result<()> {
let c = match content {
Either::A(buf) => buf.as_ref().to_owned(),
Either::B(s) => s.into_bytes(),
};

self.0
.write_with(&path, c)
.append(true)
.await
.map_err(format_napi_error)
self.write(
bxb100 marked this conversation as resolved.
Show resolved Hide resolved
path,
content,
Some(OpWriteOptions {
append: Some(true),
..Default::default()
}),
)
.await
}

/// Copy file according to given `from` and `to` path.
Expand Down Expand Up @@ -790,6 +827,131 @@ impl Writer {
}
}

#[napi(object)]
#[derive(Default)]
pub struct OpWriteOptions {
/// Append bytes into path.
///
/// ### Notes
///
/// - It always appends content to the end of the file.
/// - It will create file if the path not exists.
pub append: Option<bool>,

/// Set the chunk of op.
///
/// If chunk is set, the data will be chunked by the underlying writer.
///
/// ## NOTE
///
/// Service could have their own minimum chunk size while perform write
/// operations like multipart uploads. So the chunk size may be larger than
/// the given buffer size.
pub chunk: Option<BigInt>,

/// Set the [Content-Type](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Type) of op.
pub content_type: Option<String>,

/// Set the [Content-Disposition](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Disposition) of op.
pub content_disposition: Option<String>,

/// Set the [Cache-Control](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control) of op.
pub cache_control: Option<String>,
}

trait OpWriteWithOptions {
fn with(self, options: OpWriteOptions) -> Self;
}

impl<F: Future<Output = opendal::Result<()>>> OpWriteWithOptions for FutureWrite<F> {
//noinspection DuplicatedCode
fn with(self, options: OpWriteOptions) -> Self {
let mut writer = self;
if let Some(append) = options.append {
writer = writer.append(append);
}
if let Some(chunk) = options.chunk {
writer = writer.chunk(chunk.get_u64().1 as usize);
}
if let Some(ref content_type) = options.content_type {
writer = writer.content_type(content_type);
}
if let Some(ref content_disposition) = options.content_disposition {
writer = writer.content_disposition(content_disposition);
}
if let Some(ref cache_control) = options.cache_control {
writer = writer.cache_control(cache_control);
}
writer
}
}
impl<F: Future<Output = opendal::Result<opendal::Writer>>> OpWriteWithOptions for FutureWriter<F> {
//noinspection DuplicatedCode
fn with(self, options: OpWriteOptions) -> Self {
let mut writer = self;
if let Some(append) = options.append {
writer = writer.append(append);
}
if let Some(chunk) = options.chunk {
writer = writer.chunk(chunk.get_u64().1 as usize);
}
if let Some(ref content_type) = options.content_type {
writer = writer.content_type(content_type);
}
if let Some(ref content_disposition) = options.content_disposition {
writer = writer.content_disposition(content_disposition);
}
if let Some(ref cache_control) = options.cache_control {
writer = writer.cache_control(cache_control);
}
writer
}
}
impl OpWriteWithOptions for FunctionWrite {
//noinspection DuplicatedCode
fn with(self, options: OpWriteOptions) -> Self {
let mut writer = self;
if let Some(append) = options.append {
writer = writer.append(append);
}
if let Some(chunk) = options.chunk {
writer = writer.chunk(chunk.get_u64().1 as usize);
}
if let Some(ref content_type) = options.content_type {
writer = writer.content_type(content_type);
}
if let Some(ref content_disposition) = options.content_disposition {
writer = writer.content_disposition(content_disposition);
}
if let Some(ref cache_control) = options.cache_control {
writer = writer.cache_control(cache_control);
}
writer
}
}
impl OpWriteWithOptions for FunctionWriter {
//noinspection DuplicatedCode
fn with(self, options: OpWriteOptions) -> Self {
let mut writer = self;
if let Some(append) = options.append {
writer = writer.append(append);
}
if let Some(chunk) = options.chunk {
writer = writer.buffer(chunk.get_u64().1 as usize);
}
if let Some(ref content_type) = options.content_type {
writer = writer.content_type(content_type);
}
if let Some(ref content_disposition) = options.content_disposition {
writer = writer.content_disposition(content_disposition);
}
if let Some(ref cache_control) = options.cache_control {
writer = writer.cache_control(cache_control);
}
writer
}
}

/// Lister is designed to list entries at given path in an asynchronous
/// manner.
#[napi]
Expand Down
30 changes: 30 additions & 0 deletions bindings/nodejs/tests/suites/async.suite.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,5 +84,35 @@ export function run(op) {

op.deleteSync(filename)
})

test.runIf(op.capability().read && op.capability().write)('write with behavior', async () => {
let c = Buffer.from('hello world')
const filename = `random_file_${randomUUID()}`

const options = { chunk: 1024n * 1024n }
if (op.capability().writeCanAppend) {
options.append = true
}
if (op.capability().writeWithContentType) {
options.contentType = 'text/plain'
}
if (op.capability().writeWithContentDisposition) {
options.contentDisposition = 'attachment;filename=test.txt'
}
if (op.capability().writeWithCacheControl) {
options.cacheControl = 'public, max-age=31536000, immutable'
}
await op.write(filename, c, options)

if (op.capability().writeCanMulti) {
const writer = await op.writer(filename, options)
await writer.write(c)
await writer.close()
}

if (op.capability().delete) {
op.deleteSync(filename)
}
})
})
}
Loading
Loading