search

kitsune-soc / kitsune

🦊 (fast) ActivityPub-federated microblogging
https://joinkitsune.org
Other
319 stars 20 forks source link

Add fixtures and migrate to `serde_with` #563

Closed aumetra closed 3 months ago

aumetra commented 3 months ago

@tesaguri There are two failures right now and I'm not quite sure why. Since you wrote the code, I thought you could maybe look over it. Because a lot of the migration from raw serde types to the serde_with traits worked flawless.

There is just some weird bug with FirstOk and with the Id deserializer..

aumetra commented 3 months ago

Never mind, fixed one issue, and replaced the other with a different one :^) Really fun!

aumetra commented 3 months ago

@tesaguri just one little question, I can't quite wrap my head around that yet, why do we need this SeqAccess indirection here? I get that it's required for it to work (I tried other things), but not quite sure why. Why do we need this indirection instead of doing something with seq.next_element() and some mapping operations?

https://github.com/kitsune-soc/kitsune/blob/7dea8fd2109be5340db06cfecc64450d5a2916ab/crates/kitsune-type/src/jsonld/serde/id.rs#L87-L115

tesaguri commented 3 months ago

@tesaguri just one little question, I can't quite wrap my head around that yet, why do we need this SeqAccess indirection here? I get that it's required for it to work (I tried other things), but not quite sure why. Why do we need this indirection instead of doing something with seq.next_element() and some mapping operations?

I don't remember my thought at that time exactly so it's going to be a wild guess, but with seq.next_element::<T>(), I think <T as Deserialize>::deserialize would see map values like {"id": "https://example.com/", …} as-is. On the other hand, our goal here is to show the deserialize method a string value like "https://example.com/".

In addition to the map type, we also need to be able to deserialise verbatim string inputs like "https://example.com/". I think (unverified) the only way of handling both the types via seq.next_element() without the SeqAccess indirection would be to deserialise the next element with seq.next_element::<Value>() and matching on Value::String and Value::Object, which wouldn't be very efficient (if we don't need efficiency, we would be deserialising the entire input JSON into a DOM instead of bothering with the whole jsonld::serde megalith in the first place).

Also, Id::deserialize_as(SeqAccessDeserializer::new(seq)) would run into infinite recursion since the SeqAccessDeserializer::deserialize_any method would call Visitor::visit_seq.