Skip to content
This repository has been archived by the owner on Oct 7, 2022. It is now read-only.

Is call to load() necessary? #57

Closed
kornelski opened this issue Aug 31, 2018 · 7 comments
Closed

Is call to load() necessary? #57

kornelski opened this issue Aug 31, 2018 · 7 comments

Comments

@kornelski
Copy link

kornelski commented Aug 31, 2018

let db = FileDatabase::from_path(path, HashMap::new())?;
db.write();
db.save();

Seems to always overwrite the database with only one item, as if from_path created a fresh new database each time.

If I try to load the db:

let db = FileDatabase::from_path(path, HashMap::new())?;
db.load()?;
db.write();
db.save();

then it works only if there is one already, so it's a bit cumbersome to use. Additionally, load() fails if the file has 0 length. Unfortunately, FileDatabase::from_path does create the path with 0 length, so opening of the db and not saving breaks loading later.

I expected sqlite behavior: if there's no database already, create one. If there is one already, keep (load) the data.

@TheNeikos
Copy link
Owner

It's a fair question, the reason why I've structured it this way is to let you decide what happens.
So far in my applications I have simply tried loading, and ignored if it fails. This means it could fail for a lot of different reasons, but it's the easiest way for small applications.

If you want to be more involved, you can look at the return status and check the error kind, if it was a backend error (Any form of IO Error) or perhaps a Deserialization error? (As seen here, where no bytes are fed into it)

If it is a Deserialization Error the cause is then specific to the library you are using. This allows you to drill down into what exactly went wrong and do the right thing for your specific application.


Regarding having it behave like SQLite, I'd love that behaviour, but I want Rustbreak to be usable from a lazy_static! macro. If a create of any form could fail because of non-existent databases or other issues that are not unrecoverable then it would complicate this heavily. I am happy to discuss this though!

@neithernut
Copy link

Silently overriding an existing file, discarding its contents, does not sound like a good idea to me. It certainly breaks user expectation.

Maybe a differentiation via different functions would be appropriate. E.g. a new function FileDatabase::load_from_path() would behave like SQLite, erroring out if an existing file could not be opened and FileDatabase::from_path() would be flagged with a warning in the documentation. Or you could specify the behavior explicitly using some enum.

@kornelski
Copy link
Author

kornelski commented Aug 31, 2018

Note that sqlite (in its typical usage) does not fail. It always succeeds - if the file doesn't exist, it's created. If it exists, it's loaded.

@neithernut
Copy link

Yep, but it still yields an error if the file could not be opened.

@kornelski
Copy link
Author

Ahm, indeed. In such case I still think it'd be better to have explicit data loss if you choose so, e.g.

let db = Db::from_file("path").unwrap_or_else(|| Db::blank());

@Boscop
Copy link

Boscop commented Mar 1, 2020

I think it would be nice to get sqlite-like behavior, and so that the function that opens the db file also takes a closure that will return the initial data to use if the db file doesn't exist (and will only be called in that case).

@TheNeikos
Copy link
Owner

This discussion should be fixed once #67 (comment) is implemented. I think further discussion should happen there!

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

No branches or pull requests

4 participants