Skip to content

feat(binding/go): add write with metadata#7827

Open
ryankert01 wants to merge 1 commit into
apache:mainfrom
ryankert01:feat/go-write-with-metadata
Open

feat(binding/go): add write with metadata#7827
ryankert01 wants to merge 1 commit into
apache:mainfrom
ryankert01:feat/go-write-with-metadata

Conversation

@ryankert01

Copy link
Copy Markdown
Member

Which issue does this PR close?

N/A

Rationale for this change

The Go binding discarded the metadata returned by write operations, so callers
could not read the written object's etag/version/last-modified without a second
Stat. The Rust core and the Python binding already return Metadata from
write; this brings the Go binding in line, additively.

What changes are included in this PR?

  • C FFI: add opendal_operator_write_with_metadata and
    opendal_writer_close_with_metadata, plus the opendal_result_write struct
    (mirrors opendal_result_stat); regenerate opendal.h. The existing
    opendal_operator_write / opendal_writer_close are left unchanged.
  • Go: add Operator.WriteWithMetadata(...) (*Metadata, error) and
    Writer.CloseWithMetadata() (*Metadata, error). Write and Writer.Close
    keep their error-only signatures (still satisfy io.WriteCloser).
  • Unit tests (FFI signatures) and behavior tests.

Are there any user-facing changes?

Yes, additive only. Two new methods are added; no existing signatures change, so
existing callers keep compiling. The populated Metadata fields depend on the
service (e.g. content length is always set; etag/version when supported).

AI Usage Statement

Implemented and reviewed with Claude Code (Claude Opus 4.8).

@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 27, 2026
@ryankert01 ryankert01 marked this pull request as draft June 27, 2026 19:32
@dentiny

dentiny commented Jun 28, 2026

Copy link
Copy Markdown
Member

Ping me when it's ready to review :) @ryankert01

@ryankert01 ryankert01 force-pushed the feat/go-write-with-metadata branch from 4e696a7 to a5ce0be Compare June 29, 2026 17:41
@ryankert01 ryankert01 marked this pull request as ready for review June 29, 2026 17:41
@ryankert01 ryankert01 force-pushed the feat/go-write-with-metadata branch from a5ce0be to a6348d2 Compare June 29, 2026 17:54
@ryankert01

Copy link
Copy Markdown
Member Author

it ok now! cc @dentiny

meta, err := w.CloseWithMetadata()
assert.Nil(err, "close with metadata must succeed")
assert.NotNil(meta, "returned metadata must not be nil")
assert.Equal(uint64(len(content)), meta.ContentLength(), "writer close metadata content length")

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.

I'm not sure if it's proper to assert on content length, based on the doc, the return value could be 0 depending on the storage backend.

meta, err := op.WriteWithMetadata(path, content)
assert.Nil(err, "write with metadata must succeed")
assert.NotNil(meta, "returned metadata must not be nil")
assert.Equal(uint64(size), meta.ContentLength(), "write metadata content length")

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.

ditto

Comment thread bindings/go/writer.go Outdated
// WriteWithMetadata is a wrapper around the C-binding function
// `opendal_operator_write_with_metadata`. Which fields of the returned Metadata
// are populated depends on the service.
func (op *Operator) WriteWithMetadata(path string, data []byte, opts ...WithWriteFn) (*Metadata, error) {

@dentiny dentiny Jun 29, 2026

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.

Hi @yuchanns I'm wondering if you think it better to add a new API, or we could break current Write API and return metadata?

I kinda prefer to update the return value for Write API:

  • Current go binding is not finalized -- we need at least context as the first argument for all APIs
  • One single interface is (1) more clear; (2) stick to rust API

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

@ryankert01 ryankert01 Jun 30, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks for the context. Gotcha! Since go bindings not finalized we can do that.

@ryankert01 ryankert01 force-pushed the feat/go-write-with-metadata branch 2 times, most recently from 94f4486 to 4694896 Compare June 30, 2026 06:56
Expose the written object's metadata (etag, version, last modified, content
length) from the Go binding's write path, matching the Rust core and the Python
binding which already return Metadata from write.

Operator.Write and Writer.Close now return (*Metadata, error) instead of only
error. This is a deliberate breaking change to the Go binding's public API,
agreed on the PR thread: the binding is not yet finalized, and a single
metadata-returning interface is clearer and mirrors the Rust API.

The C binding gains the additive functions opendal_operator_write_with_metadata
and opendal_writer_close_with_metadata (plus the opendal_result_write struct,
mirroring opendal_result_stat); the existing error-only C functions are kept.
@ryankert01 ryankert01 force-pushed the feat/go-write-with-metadata branch from 4694896 to 26b8394 Compare July 2, 2026 00:10
@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label Jul 2, 2026
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: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