Add attachment support #160
Conversation
There was a problem hiding this comment.
Nice! 🐎
It seems like a great starting point 👌
I liked how you exposed both metadata and the attachment from a record.
let bytes = record.get_attachment(client).await;
let metadata = match record.attachment {... };FWIW on Firefox Desktop we have a different model, where the record is passed to a downloader, attached to the client.
let bytes = await client.attachments.download(record, options);
let metadata = record.attachment;There may be pros and cons for each approach. I'm happy if we pick the one that matches your use case better. Ideally, we just want to be able to override all the parts that are not related to the interaction with the server and its responses. My first impression is that the composition of a downloader on the client makes it easier, but I could be convinced otherwise :)
| @@ -28,6 +29,7 @@ tokio = { version = "1.8.2", features = ["macros"] } | |||
| remote-settings-client = { path = ".", features = ["viaduct_client"] } | |||
|
|
|||
| [dependencies] | |||
| anyhow = "1" | |||
src/client.rs
Outdated
| where T: TryFrom<Vec<u8>, Error = E>, | ||
| E: 'static + Send + Sync + std::error::Error | ||
| { | ||
| let key = format!("attachment:{}", metadata.hash); |
There was a problem hiding this comment.
Using the hash seems a reasonable approach to lookup the local cache 👌
There was a problem hiding this comment.
Do you think there is any need to delete old attachments? Merino isn't going to keep a cache that lives longer than a single node, so I'm not worried about it for that case.
src/client.rs
Outdated
| } | ||
|
|
||
| Err(err) => Err(ClientError::StorageError(err)), | ||
| }?; |
There was a problem hiding this comment.
Note for later: you would have to check the size / SHA256 / here (see https://searchfox.org/mozilla-central/rev/9a5f36b0ddb9cb8ae556fc5b45f8ccea0f0da6f8/services/settings/SharedUtils.jsm#24-36)
There was a problem hiding this comment.
Ah yes! I meant to ask: Is there a preferred way to calculate that hash already in RS? If not I can pull in something from crates.io easily enough.
There was a problem hiding this comment.
After implementing this, I think we shouldn't verify size, only the hash. Attachments of different sizes will (be likely to) have different hashes, and Merino wants to be able to make all content checks optional. The alternative would be something like having the verifier check the size, or just check the entire content. Since checking size along side signatures is such a weak check, I don't think that's worth the extra complexity.
There was a problem hiding this comment.
For us it was only a way to bypass hash computation if sizes differ.
|
@mythmon after your last commits and after rebasing this PR, I'm able to correctly use the remote-settings-client in Merino. See mozilla-services/merino#232. All the tests pass locally. |
|
Rebased onto master. |
There was a problem hiding this comment.
This is great!
Made a few comments, but could not spot blockers :)
Regarding the storage of attachments. What if we prefix the key with the bucket/collection? (eg. attachment:main/quicksuggest:<hash>). This way if we want to purge all attachments of a specific collection, we can delete attachment:main/quicksuggest:*.
Also what would be a good reason for not enabling the attachments feature?
We don't introduce crazy dependencies, maybe we could just always ship it? What do you think?
src/client.rs
Outdated
| use url::Url; | ||
|
|
||
| #[cfg(feature = "attachments")] | ||
| use anyhow::{anyhow, Context}; |
There was a problem hiding this comment.
nit: would enabling anyhow for the whole crate be an issue right now? (ref #143)
That's a good idea. Originally I left it unprefixed so we could de-duplicate attachments in the rare case they are in multiple collections, but that is a quite weak reason. Having the extra collection metadata is a much better reason to influence the format of the key.
Sounds good to me! |
This is still in progress. It's not documented well, or tested at all (even manual testing). That being said, I think it would be valuable to get feedback at this point.
@leplatrem Does this fit will into the library? Should I move it around or change the format?
@Dexterp37 Does this look useful for integrating into Merino?
Fixes #145