Skip to content
This repository has been archived by the owner on Apr 6, 2024. It is now read-only.

feat: add KvStore.put_slice method #3

Merged
merged 3 commits into from Sep 13, 2021

Conversation

fkettelhoit
Copy link
Contributor

This PR adds a method to put a slice of bytes into a KV store, which is not possible with the existing methods as a slice does not implement the necessary trait bounds.

Copy link
Owner

@zebp zebp left a comment

Choose a reason for hiding this comment

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

Thanks for the pr, I thought JsValue implemented Serialize so I didn't realize this would be an issue. I am a bit uneasy about the method you introduced as it's specific to byte slices.

I think it would be better if we could provide something like a put_js_value for any value the user would like to store that cannot be represented with ToRawKvValue. So for the same result as your method we'd have something like:

let buffer = &[1, 2, 3, 4];
let typed_array = Uint8Array::new_with_length(buffer.len() as u32);
typed_array.copy_from(buffer);

kv.put_js_value("example", JsValue::from(typed_array))
    .execute()
    .await?;

@fkettelhoit
Copy link
Contributor Author

You are right that the method is specific to byte slices and I understand that it sticks out a bit. But the alternative of passing in an arbitrary JsValue seems to me to go against the idea of exposing a high-level API to the Rust side (and ToRawKvValue with its implicit serialization is quite high-level compared to passing in a JsValue) because it moves the rather tedious boilerplate of converting byte slices to typed arrays to every call site.

I think ideally there would be multiple different put methods for high-level Rust types (it would probably also make sense to support streams, for example) but of course I understand if you do not want to go ahead with such a change right now. Do you want me to go ahead and replace the put_slice method with the put_js_value method?

@zebp
Copy link
Owner

zebp commented Sep 13, 2021

You are right that the method is specific to byte slices and I understand that it sticks out a bit. But the alternative of passing in an arbitrary JsValue seems to me to go against the idea of exposing a high-level API to the Rust side (and ToRawKvValue with its implicit serialization is quite high-level compared to passing in a JsValue) because it moves the rather tedious boilerplate of converting byte slices to typed arrays to every call site.

I think ideally there would be multiple different put methods for high-level Rust types (it would probably also make sense to support streams, for example) but of course I understand if you do not want to go ahead with such a change right now. Do you want me to go ahead and replace the put_slice method with the put_js_value method?

You have a good point about a potential put_js_value going against the idea of the API. As for multiple different put methods, I don't really see a case where that would be necessary outside of storing raw bytes as everything is stored as text on Cloudflare's end from my understanding. After some thinking I realized we could just impl ToRawKvValue on a byte slice like below, which seems like the best option assuming storing raw bytes is the only edge case.

impl ToRawKvValue for [u8] {
    fn raw_kv_value(&self) -> Result<JsValue, KvError> {
        let typed_array = Uint8Array::new_with_length(self.len() as u32);
        typed_array.copy_from(self);
        Ok(typed_array.into())
    }
}

@fkettelhoit
Copy link
Contributor Author

You have a good point about a potential put_js_value going against the idea of the API. As for multiple different put methods, I don't really see a case where that would be necessary outside of storing raw bytes as everything is stored as text on Cloudflare's end from my understanding. After some thinking I realized we could just impl ToRawKvValue on a byte slice like below, which seems like the best option assuming storing raw bytes is the only edge case.

impl ToRawKvValue for [u8] {
    fn raw_kv_value(&self) -> Result<JsValue, KvError> {
        let typed_array = Uint8Array::new_with_length(self.len() as u32);
        typed_array.copy_from(self);
        Ok(typed_array.into())
    }
}

Are you sure the above solves the issue? This implements ToRawKvValue for [u8] but not for &[u8], but usually a &[u8] is all you have (and needs to be persisted in KV). Implementing the trait for &[u8] is not possible:

error[E0119]: conflicting implementations of trait `ToRawKvValue` for type `&[u8]`
   --> src/lib.rs:263:1
    |
255 | impl ToRawKvValue for &[u8] {
    | --------------------------- first implementation here
...
263 | impl<T: Serialize> ToRawKvValue for T {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `&[u8]`

I think it would be extremely nice if the API would support the other types (or rather their Rust equivalents) that Cloudflare accepts:

TypeError: KV put() accepts only strings, ArrayBuffers, ArrayBufferViews, and ReadableStreams as values.
panicked at 'TypeError: KV put() accepts only strings, ArrayBuffers, ArrayBufferViews, and ReadableStreams as values.', assemblage_broadcast/src/lib.rs:49:1

Are you sure that all values are stored as text on Cloudflare's end? The following snippet from the docs leads me to believe that binary values are supported and I haven't been able to find any information indicating the opposite:

For simple values it often makes sense to use the default "text" type which provides you with your value as a string. For convenience a "json" type is also specified which will convert a JSON value into an object before returning it to you. For large values you can request a ReadableStream, and for binary values an ArrayBuffer.

For large values, the choice of type can have a noticeable effect on latency and CPU usage. For reference, the types can be ordered from fastest to slowest as "stream", "arrayBuffer", "text", and "json".

@zebp
Copy link
Owner

zebp commented Sep 13, 2021

Are you sure the above solves the issue? This implements ToRawKvValue for [u8] but not for &[u8], but usually a &[u8] is all you have (and needs to be persisted in KV). Implementing the trait for &[u8] is not possible:

You're right, I didn't think my reply through!

Are you sure the above solves the issue? This implements ToRawKvValue for [u8] but not for &[u8], but usually a &[u8] is all you have (and needs to be persisted in KV). Implementing the trait for &[u8] is not possible:

This image leads me to believe they are, but that could just be the dashboard.

I think it would be extremely nice if the API would support the other types (or rather their Rust equivalents) that Cloudflare accepts:

TypeError: KV put() accepts only strings, ArrayBuffers, ArrayBufferViews, and ReadableStreams as values.
panicked at 'TypeError: KV put() accepts only strings, ArrayBuffers, ArrayBufferViews, and ReadableStreams as values.', >assemblage_broadcast/src/lib.rs:49:1```

Ah, I see. If you could just rename put_slice to put_bytes I'll merge, don't want users to think they can store a slice of any type. I'll implement storing a byte stream at a later date.

@zebp zebp merged commit e72c701 into zebp:master Sep 13, 2021
@zebp
Copy link
Owner

zebp commented Sep 13, 2021

I will make a release with this and a few other bug fixes by the end of the week hopefully.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants