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

[Improvement]: Add list, remove, clear, and size operations to shuttle-persist #1066

Merged
merged 28 commits into from
Aug 17, 2023
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
40fc370
Initial commit (framed up function signature)
sentinel1909 Jun 29, 2023
5357cb4
improvement: add list feature to shuttle persist
sentinel1909 Jul 2, 2023
c58fac0
Merge remote-tracking branch 'shuttle-hq/main' into improvement-shutt…
sentinel1909 Jul 3, 2023
96f11a6
Rewrite list, add remove and associated tests
sentinel1909 Jul 3, 2023
9e3eea1
Run cargo fmt to fix failing CircleCI persist
sentinel1909 Jul 3, 2023
9ea36f7
Resolve failing unit tests for list and remove
sentinel1909 Jul 3, 2023
f866b73
Add clear method & remove by passing indexed item
sentinel1909 Jul 4, 2023
ece68b7
Adjustment to list_item parameter in remove method
sentinel1909 Jul 4, 2023
f7eee8b
Merge remote-tracking branch 'shuttle-hq/main' into improvement-shutt…
sentinel1909 Jul 4, 2023
407c95a
list method strips .bin, updated remove unit test
sentinel1909 Jul 5, 2023
45be093
Merge remote-tracking branch 'shuttle-hq/main' into improvement-shutt…
sentinel1909 Jul 7, 2023
f7186e1
list, remove, and clear, with tests, updated
sentinel1909 Jul 8, 2023
abc1704
Address clippy warnings in CI check
sentinel1909 Jul 8, 2023
5ff0e37
Address sloppy typo and clippy warning
sentinel1909 Jul 8, 2023
5a8a883
Fix another clippy warning in CI
sentinel1909 Jul 8, 2023
1eba10a
Merge remote-tracking branch 'shuttle-hq/main' into improvement-shutt…
sentinel1909 Jul 13, 2023
bda6a71
improvement: add size method and associated test
sentinel1909 Jul 15, 2023
fb9cea5
improvement: correct clippy linting error in tests
sentinel1909 Jul 15, 2023
568e1c2
Add tests for all list, size, remove and clear
sentinel1909 Jul 24, 2023
9c84868
Clean deps, fix error propagation in list, add new
sentinel1909 Jul 25, 2023
eced7cd
Resolve issue in circleci check
sentinel1909 Jul 25, 2023
1912a92
New method creates the storage folder
sentinel1909 Jul 25, 2023
107c461
New method returns shuttle_service::Error
sentinel1909 Jul 28, 2023
c51f298
Fix issue causing circleci/persist to fail
sentinel1909 Jul 28, 2023
d02dcc1
Update resource builder to use new method
sentinel1909 Jul 29, 2023
4058a69
Merge remote-tracking branch 'shuttle-hq/main' into improvement-shutt…
sentinel1909 Jul 30, 2023
c7fe00b
Merge branch 'improvement-shuttle-persist-list' of github.com:sentine…
sentinel1909 Jul 30, 2023
0472444
chore: update document comments per review
sentinel1909 Aug 17, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions resources/persist/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
# output files from tests
shuttle_persist/**/*.bin
3 changes: 3 additions & 0 deletions resources/persist/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,6 @@ bincode = "1.2.1"
serde = { version = "1.0.0", features = ["derive"] }
shuttle-service = { path = "../../service", version = "0.21.0" }
thiserror = "1.0.32"

[dev-dependencies]
rand = "0.8.5"
Copy link
Member

Choose a reason for hiding this comment

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

Don't need this anymore :)

131 changes: 130 additions & 1 deletion resources/persist/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,12 @@ pub enum PersistError {
Open(std::io::Error),
#[error("failed to create folder: {0}")]
CreateFolder(std::io::Error),
#[error("failed to list contents of folder: {0}")]
ListFolder(std::io::Error),
#[error("failed to clear folder: {0}")]
RemoveFolder(std::io::Error),
#[error("failed to remove file: {0}")]
RemoveFile(std::io::Error),
#[error("failed to serialize data: {0}")]
Serialize(BincodeError),
#[error("failed to deserialize data: {0}")]
Expand All @@ -41,6 +47,47 @@ impl PersistInstance {
Ok(serialize_into(&mut writer, &struc).map_err(PersistError::Serialize))?
}

/// list method returns a vector of strings containing all the keys associated with a PersistInstance
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: should we capitalize these doc comments like we do elsewhere in the codebase? I also don't think we need "list method" as the first part of the comment.

Suggested change
/// list method returns a vector of strings containing all the keys associated with a PersistInstance
/// Returns a vector of strings containing all the keys associated with a [PersistInstance]

pub fn list(&self) -> Result<Vec<String>, PersistError> {
let storage_folder = self.get_storage_folder();

let mut list = Vec::new();

let entries = fs::read_dir(storage_folder).map_err(PersistError::ListFolder)?;
for entry in entries {
let key = entry.map_err(PersistError::ListFolder)?;
let key_name = key
.path()
.file_stem()
.unwrap_or_default()
.to_str()
.unwrap_or("file name contains non-UTF-8 characters")
Copy link
Member

Choose a reason for hiding this comment

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

This error should propagate instead of being turned into a key name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one is causing a bit of head bashing, but will continue to bash. I'm not seeing how to propagate right in this moment, but will get there.

Copy link
Member

Choose a reason for hiding this comment

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

You can take a shortcut and map the error to a ListFolder and propagate with ? like the statement before it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is exactly what I tried but it's not working. The .to_str() method gives off an Option, which I thought .ok_or() would help me convert into a Result<T, E> which I could then .map_err on, but nope. Will get back at it this evening.

Copy link
Member

Choose a reason for hiding this comment

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

Aha, well then don't you already have a Result<&str, PersistError> after doing .ok_or(PersistError::ListFolder)? If so, you can do the ? immediately, then to_string().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmmmm...might just have to take my laptop with me to work today and continue to work on this at lunch :)

Copy link
Contributor Author

@sentinel1909 sentinel1909 Jul 24, 2023

Choose a reason for hiding this comment

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

Ok, I think I've got this worked out. I figured out how to use ok_or to convert the error related to the potential for invalid Unicode characters. Here's the revised list method:

/// list method returns a vector of strings containing all the keys associated with a PersistInstance
    pub fn list(&self) -> Result<Vec<String>, PersistError> {
        let storage_folder = self.get_storage_folder();

        let mut list = Vec::new();

        let entries = fs::read_dir(storage_folder).map_err(PersistError::ListFolder)?;
        for entry in entries {
            let key = entry.map_err(PersistError::ListFolder)?;
            let key_name = key
                .path()
                .file_stem()
                .unwrap_or_default()
                .to_str()
                .ok_or("the file name contains invalid characters").map_err(PersistError::ListName)?
                .to_string();
            list.push(key_name);
        }
        Ok(list)
    }

I did have to introduce a lifetime specifier on our PersistError enum, to satisfy the compiler:

#[derive(Error, Debug)]
pub enum PersistError <'a> {
    #[error("failed to open file: {0}")]
    Open(std::io::Error),
    #[error("failed to create folder: {0}")]
    CreateFolder(std::io::Error),
    #[error("failed to list contents of folder: {0}")]
    ListFolder(std::io::Error),
    #[error("failed to list the file name: {0}")]
    ListName(&'a str),
    #[error("failed to clear folder: {0}")]
    RemoveFolder(std::io::Error),
    #[error("failed to remove file: {0}")]
    RemoveFile(std::io::Error),
    #[error("failed to serialize data: {0}")]
    Serialize(BincodeError),
    #[error("failed to deserialize data: {0}")]
    Deserialize(BincodeError),
}

Copy link
Member

Choose a reason for hiding this comment

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

I would say that error does not need a test case, since it should realistically never happen. Since the keys (file names) are Strings when they are saved, they should be valid utf-8 when reading the directory as well.

.to_string();
list.push(key_name);
}
Ok(list)
}

/// clear method removes the keys within the PersistInstance
pub fn clear(&self) -> Result<(), PersistError> {
let storage_folder = self.get_storage_folder();
fs::remove_dir_all(&storage_folder).map_err(PersistError::RemoveFolder)?;
fs::create_dir_all(&storage_folder).map_err(PersistError::CreateFolder)?;
Ok(())
}

/// size method returns the number of keys in a folder within a PersistInstance
pub fn size(&self) -> Result<usize, PersistError> {
Ok(self.list()?.len())
}

/// remove method deletes a key from the PersistInstance
pub fn remove(&self, key: &str) -> Result<(), PersistError> {
let file_path = self.get_storage_file(key);
fs::remove_file(file_path).map_err(PersistError::RemoveFile)?;
Ok(())
}

pub fn load<T>(&self, key: &str) -> Result<T, PersistError>
where
T: DeserializeOwned,
Expand Down Expand Up @@ -111,13 +158,95 @@ mod tests {
assert_eq!(result, "test");
}

#[test]
fn test_list_and_size() {
let persist = PersistInstance {
service_name: ServiceName::from_str("test1").unwrap(),
Copy link
Member

Choose a reason for hiding this comment

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

The tests are looking really good now! However, there is one case that is not yet covered, but it might simplify the code.

If list/load/remove etc are called before any key has been saved, they will fail due to the folder not existing yet. This can be avoided if we ensure the folder exists before any method is called.

My idea is to add a method new(service_name: ServiceName) -> Result<Self, PersistError> that creates the folder upfront. Creating the folder at this time also means it is not needed in save.
This would mean all constructions of PersistInstance { ... } change to PersistInstance::new(...) + .unwrap()/?. It also means that the error needs to map_err into a shuttle_service::Error (or other way of converting) in ResourceBuilder::output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So a new method after all then...got it.

Copy link
Contributor Author

@sentinel1909 sentinel1909 Jul 25, 2023

Choose a reason for hiding this comment

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

I'm doing a push as a checkpoint to show where I'm at. As a start, I've made a skeleton new method that instantiates a PersistInstance struct, given a ServiceName. I've substituted this in the tests and all works as before. I'm having difficulty now understanding how to get the folder creation incorporated. If I put a &self in as a parameter to new, how do I then instantiate in the tests? Just need a couple of breadcrumbs to set the path forward.

Copy link
Member

Choose a reason for hiding this comment

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

Ah! Good point.
I think this is a nice workaround. Using the fact that we "have" the instance (self) in the new() function since we are creating it right there.

fn new(...) -> Result<...> {
    let instance = Self { ... };
    let directory = instance.get_storage_folder();
    fs::create_dir_all(...).map_err(...)?;

    Ok(instance)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It also means that the error needs to map_err into a shuttle_service::Error (or other way of converting) in ResourceBuilder::output.

Having some difficulty now with this piece. I understand what you're saying but am not sure how to make it happen. I will reflect through the day today.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, time to admit it. After sititng staring at the code for a fair bit of time, I have no clue how to convert the errors into a shuttle_service::Error in ResourceBuilder::output. I think I need a hint.

Copy link
Member

@jonaro00 jonaro00 Jul 27, 2023

Choose a reason for hiding this comment

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

This works

// Get the PersistError in, let's say, a match statement, then
return Err(shuttle_service::Error::Custom(
    /*The PersistError*/.into(),
));

EDIT: based on the above I would imagine that something like this will work:

PersistInstance::new(...).map_err(|e| shuttle_service::Error::Custom(e.into()))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I feel I've got the new method working. It will convert a PersistError into a shuttle_service::Error. I'm having difficulty with a test though. All I managed to do yesterday evening was test ServiceName, which panics if you pass it something invalid. This doesn't really test the new method erroring out because of an issue with create_dir_all. Also, the cargo-shuttle circleci check is still failing, and I'm not clear why (I have tried to read the log output) or how to resolve it.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the create_dir_all is hard to test failures of. Perhaps creating a folder in a root-owned directory, but that might not be reproducible on all machines. I would say that we can trust it without a test.

The CI fail on cargo-shuttle init is a sporadic error (a hard one to fix :/ ), and not related to your changes.

};

persist.save("test", "test").unwrap();
let list_result = persist.list().unwrap().len();
let size_result = persist.size().unwrap();
assert_eq!(list_result, 1);
assert_eq!(size_result, 1);
}

#[test]
fn test_list_error() {
let persist = PersistInstance {
service_name: ServiceName::from_str("test2").unwrap(),
};

// unwrap error
let result = persist.list().unwrap_err();
assert_eq!(
result.to_string(),
"failed to list contents of folder: No such file or directory (os error 2)"
);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it is unintuitive that calling list() on an empty persist store returns an error. If you also call create_dir_all in list() like save() does, you can make sure that the folder always exists when asked for, and an empty Vec can be returned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now just removing the files instead of the entire folder, test adjusted accordingly.

}

#[test]
fn test_remove() {
let persist = PersistInstance {
service_name: ServiceName::from_str("test3").unwrap(),
};

persist.save("test", "test").unwrap();
persist.save("test2", "test2").unwrap();
let list = persist.list().unwrap();
let key = list[0].as_str();
persist.remove(key).unwrap();
let result = persist.list().unwrap();
assert_eq!(result.len(), 1);
}

#[test]
fn test_remove_error() {
let persist = PersistInstance {
service_name: ServiceName::from_str("test4").unwrap(),
};

// unwrap error
let result = persist.remove("test4").unwrap_err();
assert_eq!(
result.to_string(),
"failed to remove file: No such file or directory (os error 2)"
);
}

#[test]
fn test_clear() {
let persist = PersistInstance {
service_name: ServiceName::from_str("test5").unwrap(),
};

persist.save("test5", "test5").unwrap();
persist.clear().unwrap();
let result = persist.list().unwrap();
assert_eq!(result.len(), 0);
}

#[test]
fn test_clear_error() {
let persist = PersistInstance {
service_name: ServiceName::from_str("test6").unwrap(),
};

// unwrap error
let result = persist.clear().unwrap_err();
assert_eq!(
result.to_string(),
"failed to clear folder: No such file or directory (os error 2)"
);
}

#[test]
fn test_load_error() {
let persist = PersistInstance {
service_name: ServiceName::from_str("test").unwrap(),
};

// unwrapp error
// unwrap error
let result = persist.load::<String>("error").unwrap_err();
assert_eq!(
result.to_string(),
Expand Down