khonsulabs / bonsaidb

A developer-friendly document database that grows with you, written in Rust
https://bonsaidb.io/
Apache License 2.0
1.01k stars 37 forks source link

derive Key #239

Closed ModProg closed 2 years ago

ModProg commented 2 years ago

fixes #106

ModProg commented 2 years ago

@ecton It is probably a good idea, if you run cargo expand --test key and have a read of what the macro produces.

ModProg commented 2 years ago

also feel free to suggest some actual tests :D

ecton commented 2 years ago

Just wanted to say thank you for getting this submitted. I started feeling a little under the weather this afternoon, so I'll check this out soon but it'll be at least tomorrow.

ecton commented 2 years ago

This all looks great. As for real tests, I would look at #240 for example of two values being encoded that produces an interesting edge case and uses two datatypes. I wouldn't worry too much about thorough testing -- the main test the macro needs to worry about it to ensure every bit of data is encoded. The testing of the composite key encoding/decoding can be handled independently, as long as we are confident in the unit tests calling the encoding/decoding functions in the proper order.

There are some changes in main that hopefully will clean up some of the ugliness from the return types in decode_composite_key. I've deprecated encode/decode_composite_key in favor of two structs: CompositeKeyEncoder, CompositeKeyDecoder. An example usage:

 let value1 = String::from("hello");
 let value2 = 42_u32;
 let mut encoder = CompositeKeyEncoder::default();
 encoder.encode(&value1).unwrap();
 encoder.encode(&value2).unwrap();
 let encoded = encoder.finish();

 let mut decoder = CompositeKeyDecoder::new(&encoded);
 let decoded_string = decoder.decode::<String>().unwrap();
 assert_eq!(decoded_string, value1);
 let decoded_u32 = decoder.decode::<u32>().unwrap();
 assert_eq!(decoded_u32, value2);
 decoder.finish().expect("trailing bytes");

The last thing is to add one more option to the derive macro: allow_null_bytes = true. By default this should be false. When this is set, call CompositeKeyEncoder::allow_null_bytes_in_variable_fields. This relaxes a restriction that might be irrelevant for many users, but should be enabled by default to avoid the appearance of bugs.

ModProg commented 2 years ago

@ecton I added the new encoder/decoder and some test.

ecton commented 2 years ago

Thank you as always @ModProg!