Loading database with empty File fails about rustbreak HOT 18 CLOSED

FileDatabase::from_path knowns whether it creates a new file, so an obvious solution would be for from_path to write data (it's second argument) to the file on file creation.

This won't initialize over a corrupted database.

from rustbreak.

@TheNeikos Sounds like a sensible API.
Yes, I can make a PR. I think I'll add some tests to the codebase first though, since coverage is currently quite low, and I want to be sure I don't break things.

from rustbreak.

Hm, maybe:

• load_from_path(path): error is file doesn't exits, otherwise load data from file.
• load_from_path_or_else(path, closure): if file exists, then load data; otherwise execute closure and use the result for data, directly save to file
• load_from_path_or(path, data): load_from_path_or_else(path, || {data}) (Result and Option types in std also always have a separate method for this; for example unwrap_or and unwrap_or_else)
• load_from_path_or_default(path): load_from_path_or_else(path, Default::default)
• create_at_path(path, data): always use data for the Database.data field (never load()), if path didn't exist save() this data (This was what I intended as from_path_or_data)

?

from rustbreak.

Yes, since an empty file is not a valid YAML structure. You need to check for invalid loads and handle the case, either by creating a new database or trying to rescue it (if that is applicable in your situation).

from rustbreak.

Would a convenience function load_or_init() be an idea? Because this is kind of an edge-case you have in all backends,... like ... is an empty string a valid JSON object? Or a valid RON object?

from rustbreak.

Well, the problem is, you can't know, since the deserialization is completely detached from the backend. And there is no 'Default' for a given DeSer. Maybe this could be added? It wouldn't be on the backend though but on the deserialization trait.

from rustbreak.

I don't know about the details...

Maybe the simple solution load_or_init_with(closure) can be added (2.1.0 then, of course).

from rustbreak.

Aye, that could work. It is rather dangerous though, as a corrupted DB could be just initialized over instead of being possibly saved.

from rustbreak.

Hmh. Okay... maybe there's a way to handle this. The problem is that the obvious thing (for a user) does not work: instantiate() and_then load(). Maybe there's a way around the whole edge cases here, I don't know.

from rustbreak.

I like the idea of @niluxv that on file creation an empty but valid data structure according to the chosen encoding is written to the file.

In practice loading from an empty file created by other means could still fail depending on the chosen encoding. I think this can be defined as "expected behaviour" because failed deserialization has to be handled by the library user not by the library itself.

If I understand correctly the challenge could be to also consider the data structure defined. An empty HashMap<u32, String> looks differently than an empty HashMap<u32, serde_yaml::Value>. This examples can not even considered as empty databases because there is still a record 0.

serde_yaml::Value::Null

---
0: ~

Empty String

---
0: ""

from rustbreak.

If I understand correctly the challenge could be to also consider the data structure defined.

Yes, I think you understand it correctly. But let's clarify it a bit anyway.
We have the following method signature:

pub fn from_path<S>(path: S, data: Data) -> Result<FileDatabase<Data, DeSer>>
    where S: AsRef<Path>

The current behaviour is as follows:

• use data as the in-memory database contents (as stored in the data field of a Database)
• open the file at path and use it as the backend; if no file exists at that path, then create the file (empty)

The problem is that one wants to use the data from the file (backend), instead of the supplied data, so one needs to load. But if the file was created in the from_path call, it is empty so invalid for most/all data representations (serializer/deserializer), so load fails. We would actually want to write some initialisation data on file creation. There is however no reasonable way for the library user to do this (in the current api). (One could save on load failure, but that is dangerous since it can overwrite a corrupted database.

So the proposal is:

• use data as the in-memory database contents (as stored in the data field of a Database)
• open the file at path, if it exists, and use it as the backend
• if it doesn't exist, create the file and save data to it

If you use Default::default() as data, then the default value (for a hash map that is the empty one) will be saved on file creation.

In particular if the file already exists, the behaviour of from_path stays exactly the same.

@TheNeikos If you agree to this proposal, I can make a PR.

from rustbreak.

@niluxv I like the idea! I would propose this API:

• from_path(path) -> fails if file does not exist
• from_path_or_else(path, closure) -> If file does not exist, execute closue that creates the default value which is then saved
• from_path_or_default(path) -> If file does not exist, save default

Do you still offer creating a PR? Otherwise I'd add this.

from rustbreak.

Can confirm the same thing with Bincode - it fails with "unexpected EOF".

It's easy to work around by providing initial data, or saving db immediately once it's created on fresh file.

from rustbreak.

@TheNeikos I'd also add a from_path_or_data which acts like I described in this comment.

And maybe it would be clearer to name the one failing on the file on existing from_path_or_fail, from_path_or_error, from_path_must_exist or something like that?

And a question about from_path_or_else(path, closure, data). If a file at path doesn't yet exist, should still data be used for the Database.data field? Or did you intend the API you propose to be for the backend, not the Database?

from rustbreak.

I feel from_path_or_data is the same as my proposed from_path_or_else(path, || MyData::new(...)). Or how else would it differ?

from rustbreak.

from_path_or_else(path, closure) -> If file does not exist, execute closue that creates the default value which is then saved

This only executes closure if the file didn't exist. What should go in the data field if the file did exist? Do you want to automatically load the file?

from rustbreak.

This sounds good, yes

from rustbreak.

I think this has been covered with the new release

from rustbreak.