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.

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

Implement Object Store for Viceroy #167

Merged
merged 8 commits into from
Aug 19, 2022
Merged

Implement Object Store for Viceroy #167

merged 8 commits into from
Aug 19, 2022

Conversation

mgattozzi
Copy link
Contributor

@mgattozzi mgattozzi commented Aug 11, 2022

This commit implements support for Object Store in Viceroy allowing for
users to simulate locally being able to query for blobs or add blobs to
the store. This commit does so with the following important changes:

  • Bump the version of the Rust SDK to 0.8.7 which contains support for
    the Object Store with higher level APIs rather than having users use
    raw host calls

  • Add config support to fastly.toml in the following format:

    [local_server]
    object_store.empty_store = []
    object_store.store_one = [{key = "first", data = "This is some data"},{key = "second", path = "../test-fixtures/data/object-store.txt"}]
    

    or alternatively with the following TOML syntax

    [local_server]
    [object_store]
    empty_store = []
    [[object_store.store_one]]
    key = "first"
    data = "This is some data"
    [[object_store.store_one]]
    key = "second"
    path = "../test-fixtures/data/object-store.txt"
    

    Semantically these are the same and it comes down to style preference
    as the TOML parsing should work the same.

    object_store is a new field in the local_server config option. It
    takes any string value which is the name of the object store. This
    then contains an array of objects that contain a key field which is
    the name of the object stored in the store and either a path or a
    data key value not both. path points to a file relative to the
    project root and data is a string of bytes. This lets the user either
    point to some file, useful for cases where the file might be really
    large, or inline if the data is small.

    The object store must be defined in the fastly.toml file beforehand
    as new Object Stores cannot be created on the fly, however data can be
    inserted after opening the file. The store will maintain the state
    across requests and will lose it when Viceroy is stopped.

  • Add Object Store hostcalls to Viceroy

  • Implement the Object Store hostcalls for Viceroy

With this users can now locally test their code as if they were running
in production.

I wanted to get this up to have a second pair of eyes, but the following are tasks I still need to finish up before getting approval. Note CI will fail until we release the new version of the SDK.

TODO List before merging:
-[x] Add validation checks for the object key names
-[x] Test more aspects of the SDK/Object Store

cc: @JakeChampion since you were working on the JS SDK support I figured you'd also want to kick the tires with this!

This commit implements support for Object Store in Viceroy allowing for
users to simulate locally being able to query for blobs or add blobs to
the store. This commit does so with the following important changes:

- Bump the version of the Rust SDK to 0.8.7 which contains support for
  the Object Store with higher level APIs rather than having users use
  raw host calls
- Add config support to `fastly.toml` in the following format:

  ```
  [local_server]
  object_store.empty_store = []
  object_store.store_one = [{key = "first", data = "This is some data"},{key = "second", path = "../test-fixtures/data/object-store.txt"}]
  ```

  or alternatively with the following TOML syntax

  ```
  [local_server]
  [object_store]
  empty_store = []
  [[object_store.store_one]]
  key = "first"
  data = "This is some data"
  [[object_store.store_one]]
  key = "second"
  path = "../test-fixtures/data/object-store.txt"
  ```

  Semantically these are the same and it comes down to style preference
  as the TOML parsing should work the same.

  `object_store` is a new field in the `local_server` config option. It
  takes any string value which is the name of the object store. This
  then contains an array of objects that contain a `key` field which is
  the name of the object stored in the store and either a `path` *or* a
  `data` key value not both. `path` points to a file relative to the
  project root and data is a string of bytes. This lets the user either
  point to some file, useful for cases where the file might be really
  large, or inline if the data is small.

  The object store must be defined in the `fastly.toml` file beforehand
  as new Object Stores cannot be created on the fly, however data can be
  inserted after opening the file. The store will maintain the state
  across requests and will lose it when Viceroy is stopped.
- Add Object Store hostcalls to Viceroy
- Implement the Object Store hostcalls for Viceroy

With this users can now locally test their code as if they were running
in production.

TODO: Add validation checks for the object key names
TODO: Test more aspects of the SDK/Object Store
Copy link
Contributor

@acw acw left a comment

Choose a reason for hiding this comment

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

Hello! This looks pretty good, I'd just love to get a few more tests in there to validate that the exceptional cases work as expected.

lib/src/error.rs Show resolved Hide resolved
use fastly::ObjectStore;

fn main() {
let store_one = ObjectStore::open("store_one").unwrap().unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a negative version of this test, 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.

Added in 4dc177e


#[tokio::test(flavor = "multi_thread")]
async fn object_store() -> TestResult {
const FASTLY_TOML: &str = r#"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add some negative checks for mis-formatted toml files, to make sure your nice error messages pop out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in e7555b0

@mgattozzi
Copy link
Contributor Author

@acw I've updated the PR with fixes in the implementation due to adding more tests (yaaaaay) and more tests to cover all the failure cases for configs

Copy link
Contributor

@acw acw left a comment

Choose a reason for hiding this comment

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

Let's fix that error message, otherwise looks good. (MAKE FORMAT!!! says CI)


#[derive(Debug, PartialOrd, Ord, PartialEq, Eq, thiserror::Error)]
pub enum ObjectStoreError {
#[error("lol it's missing ya dingus")]
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, we probably shouldn't publish this error message. 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And this is why we do code review

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 284832e

@mgattozzi
Copy link
Contributor Author

@acw I think with this error message fix and having run cargo fmt everywhere we just need a release of the new sdk. I can confirm that locally these tests pass at least with the new one.

@mgattozzi
Copy link
Contributor Author

Ah wait I need to add validation for object names like we do in the SDK locally

@mgattozzi
Copy link
Contributor Author

@acw key validation is added now in a9a7d75 with accompanying tests so I think we have all the things we need done now!

acw
acw previously approved these changes Aug 17, 2022
Copy link
Contributor

@acw acw left a comment

Choose a reason for hiding this comment

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

👯

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

Successfully merging this pull request may close these issues.

2 participants