-
Notifications
You must be signed in to change notification settings - Fork 7
Description
I think that the API could be improved to feel more natural and consistent.
This PR is a proposal for new types to be introduced, while deprecating other types.
That being said, we should deliver on our promise not to introduce breaking changes to the API before a v1.
The backbone of the API: Interface
Rename to Storage
First thing is the interface called Interface.
It's inexpressive (maybe inspired by std sort.Interface?), something like Storage should be preferred.
Using another name is an opportunity to make the switch to another API more obvious and non-breaking.
Member functions
In Interface (to be Storage), the Store, Load and Delete functions are perfectly fine as is, and should not change (they are also consistent with sync.Map).
The List function is fine, but the returned BlobList has problems IMO:
- It is defined as a slice of
Blob. The type nameBlobis misleading as it only provides information about a the abstract content that we'd expect a blob to be. - It only provides the name and the size of a blob, while other information such as a timestamp, would be very useful and doesn't seem to contradict the desired simplicity.
We should replace List with other ways to iterate over the content of a Storage.
Expecting a potentially large amount of keys in a storage, and since we're only supporting recent Go versions, an iter.Seq or iter.Seq2 implementation would be suitable.
To keep being able to provide a prefix as a parameter, an optional interface that provides a Glob function could be introduced.
The following definition of Storage allows to pass a Storage to any function that accepts an Interface.
In function signatures/arguments, Interface will be seamlessly replaced when breaking changes are enacted.
type Storage interface {
Interface // expose the same API as Interface
All(ctx context.Context) iter.Seq2[BlobInfo, error]
}
type BlobInfo interface {
Name() string
Size() uint64
Timestamp() time.Time
}Registration and fetching of backends
Functions RegisterBackend and GetBackend will keep on existing as is (accepting/returning an Interface) to keep compatibility of externally-defined backends until v1.
Functions Register and Open will be created to accept a Storage.
New optional interfaces
Stream interfaces (StreamReader and StreamWriter)
I think the optional interfaces are good.
However, while using NewWriter I realised its current implementation has a shortcoming.
With the S3 backend, I noticed that the only way to cancel saving data that has been written requires cancelling the context associated with it.
This behaviour also forces retaining the context, that breaks expectations one should have of a well-defined Go API.
As per go doc context:
Do not store Contexts inside a struct type; instead, pass a Context explicitly to each function that needs it. This is discussed further in https://go.dev/blog/context-and-structs.
To remediate the lack of explicit cancellation, I'd like to introduce new types Reader and Writer, returned by NewReader and NewWriter.
Note that to avoid sneaky breaking changes, the context will be retained in the S3 writer until v1.
type BlobReader interface {
io.ReadCloser
}
type BlobWriter interface {
io.WriteCloser
// Cancel should inhibit saving the content being written when the writer is closed.
// If an error has already occurred, this error should be returned instead.
Cancel(context.Context) error
}Those two new types would allow introducing the following change without breaking anything:
-func NewReader(context.Context, Interface, string) (io.ReadCloser, error)
+func NewReader(context.Context, Interface, string) (Reader, error)
-func NewWriter(context.Context, Interface, string) (io.WriteCloser, error)
+func NewWriter(context.Context, Interface, string) (Writer, error)Other optional interfaces
As mentioned above, I'd like to introduce an optional interface that provides a Glob function.
I also have played with implementing an optional interface to provide a fs.FS.
Being able to provide a timestamp, as proposed with [BlobInfo] above, would allow having a more complete definition.
There may be cases that prompt for a timestamp to be provided through an optional interface, but none that I can think of at the moment.