LSN and ids Serialize impls force unnecessary copy-paste

[koivunej]

Repeated:

#[serde_with::serde_as]
#[derive(serde::Serialize)]
struct Foo {
  #[serde_as(as = "DisplayFromStr")]
  lsn_start: Lsn,
}

This could be just:

#[deride(serde::Serialize)]
struct Foo {
  lsn_start: Lsn,
}

If we'd query the serializer for https://docs.rs/serde/latest/serde/trait.Serializer.html#method.is_human_readable in the serde::Serialize impl for LSN, id and then decide to use stringified (for json) or u64 (for bincode).

This has been a bigger issue with ids by default serializing to array of bytes.

Deserialize impls should handle strings and u64 or string and array of bytes (haven't checked if they already do).

(I haven't yet implemented this so there might be some issues.)

[SomeoneToIgnore]

Deserialize impls should handle strings and u64 or string and array of bytes (haven't checked if they already do).

AFAIK, this was the issue last time I've tried to implement this: I failed to distinguish "bytes of a json string" from "bytes of a bincode serialization", causing obscure bugs in tests.

[koivunej]

There is #[serde(with = "hex")] as well in safekeeper code, see: #5163 (comment)

[koivunej]

Marking this as good_first_issue.

First thing which needs to be done here is to add image tests for both bincode OR serde-json serialized types which have LSN, TenantId, OR TimelineId within them. Tests should be something like:

#[test]
fn ensure_type_roundtrips() {
  let val = /* creation omitted */;
  
  let expected = b"....";  // for serde-json
  let expected = [...] or hex; // for bincode, whichever you find more aesthetic
  
  assert_eq!(&serialize(&val), &expected);
  assert_eq!(deserialize(&expected).unwrap(), val);
}

After that in the Serialize implementation my plan was to use https://docs.rs/serde/latest/serde/trait.Serializer.html#method.is_human_readable to query which kind of serializer it is, and in the Deserialize implementation, support both kind of values with the ability to drop one for the other (bincode wants a byte array for TenantId, json wants a hex string -- might not be great to support deserializing both, but might be).

I haven't yet implemented this so there might be some issues, but after adding the tests and making the modifications to the serialize and deserialize impls, it would be safe to start removing the copypaste, and tests should tell us if the idea was good. Suggest starting out with one type of each serialization.

Lsn within bincode is most likely just a u64. Within serde-json it is {upper_u32_hex}/{lower_u32_hex}.

[koivunej]

Alternatively a new test type with all (Lsn, TenantId, TimelineId) could be created as two variants, one for bincode and other for serde_json. Then tests could be created for that type and the different formats roundtripping.

However, it would still be better to find the two examples and show that the really used serialization format does not change. We only have "image based" testing for index_part.json, which is not much.

[duguorong009]

I would like to take this issue!

@koivunej
I created the draft PR with Lsn serde implementation.
Can you take a look & give me comments?
Thx!