Data Format of Map Values can never change

[t-moe]

Hi @diondokter ,
I realized something today, when using map.
I'm using your excellent crate, to store/restore device settings. As key I use a simple u8. Values are byte-arrays serialized/deserialized with postcard.

You have the following code (invoked by fetch_item):

sequential-storage/src/map.rs

Lines 264 to 272 in 6612981

	while let Some((item, address)) = it.next(flash, data_buffer).await? {
	if I::deserialize_key_only(item.data()).map_err(MapError::Item)? == search_key {
	newest_found_item = Some((
	I::deserialize_from(item.data()).map_err(MapError::Item)?,
	address,
	item.header,
	));
	}
	}

Because you deserialize_from for every item and abort on error, I can never change the data format for a given key.

Example:

• lets say for key 0 I want to store a struct like this:

struct ConfigA {
    counter: u8
}

• I store this struct once, using store_item (and postcard).

• after storing it once, I change the struct to

struct ConfigA {
    counter: u32
}

• upon boot the device will fail to deserialize the config because postcard expects more bytes
• I default construct my config and call store_item again so that on the next boot I have something valid to read.
• But upon the next boot, the old item is still iterated by fetch_item and trying to deserialize_from will immediately abort fetch_item, even though another item would follow which can be deserialized.

I haven't thought about the consequences in detail, but I think we should change the implementation, so that fetch_item only fails, if the last item of the given key can not be deserialized.

Whats your opinion on the topic?

Thank you

[diondokter]

I see your problem. So yeah, this is currently not supported.

It's funny you posted that code snippet because in the name of binary size optimizations I'm looking to change the API in #35.
The change would be to separate the key from the item. And the way that that code snippet would work is that it only knows about the key and the item deserialization would be done later.

So that'd sorta already fix your problem you have here.

The way this was thought out is that if you have an updated value, that it should have a new key.

So, question for you.

What would you rather see?

• store_item(key, item) - fetch_item(key)
  • Key is fully separate to the item (this is the current refactor in Binary size optimizations and map interface changes #35)
• store_item(item) - fetch_item(key)
  • Key is used separately, but the item knows about its own key (this is the current API)
• store_item(item) - fetch_item::<ItemType>
  • The key is never used separately and is a const on the Item type

All of these have their pros and cons... Hard to choose

[t-moe]

What would you rather see?

• store_item(key, item) - fetch_item(key)

  • Key is fully separate to the item (this is the current refactor in Binary size optimizations #35)

• store_item(item) - fetch_item(key)

  • Key is used separately, but the item knows about its own key (this is the current API)

• store_item(item) - fetch_item::<ItemType>

  • The key is never used separately and is a const on the Item type

All of these have their pros and cons... Hard to choose

Currently my items always have a key which is a const on the item type. But not allowing runtime keys might be a dealbreaker for others. So I think fully separating the key from the item (->Value?) is the best way to go. I think this is also the API people are expecting when coming from something like std::collections::HashMap.

[diondokter]

Yes, I agree with your reasoning. But what do you think about the item type? With a normal hashmap you can't store multiple types in it.

I could also forego item types fully by making the API only take byte slices. What you want to do with them is then up to you. Same as with queue which is also only byte slices.

[t-moe]

With a normal hashmap you can't store multiple types in it.

Ah yes. You're right. Well, I need multiple types. But I dont care about having an additional item type. It is somehow pointless if the key is no longer part of it. store_item(key, slice) would work nicely for me.

[diondokter]

Ok, #35 implements a new map interface that explicitly allows and facilitates using multiple types (or even &[u8] if you want). Keys and values are separated now and the value is only deserialized at the very end.

[t-moe]

Cool, thanks a lot for fixing this. I will take a look in the coming weeks and report back.

[t-moe]

I've tested the new master branch and I can now update my data format without erasing the flash. Thank you!