Skip to content

Commit

Permalink
Fix property parsing when a property contains a struct (#210)
Browse files Browse the repository at this point in the history
Properties are parsed lazily by deserializing the data into an `IgnoredAny`
and then returning the bytes that have been read.

We do parse structs as an enum in order to emit the struct tag as enum tag.
The `IgnoredAny` then deserializes a newtype enum variant, expecting a single item.
The `VariantAccess` impl for the packstream deserializer would then only emit the next field in the struct, introducing the bug.
The fix is to emit all fields when the list of items is from a struct and we are parsing properties.
  • Loading branch information
knutwalker authored Jan 9, 2025
1 parent a56e2d2 commit d719bd1
Show file tree
Hide file tree
Showing 3 changed files with 80 additions and 13 deletions.
34 changes: 33 additions & 1 deletion lib/src/bolt/structs/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,10 @@ mod tests {
use serde_test::{assert_de_tokens, Token};
use test_case::{test_case, test_matrix};

use crate::packstream::{bolt, from_bytes_ref, BoltBytesBuilder, Data};
use crate::{
bolt::LegacyDateTime,
packstream::{bolt, from_bytes_ref, BoltBytesBuilder, Data},
};

use super::*;

Expand Down Expand Up @@ -335,4 +338,33 @@ mod tests {
.tiny_string("Alice")
.build()
}

fn deserialize_properties_with_structs() {
let mut data = Data::new(
bolt()
.structure(3, 0x4E)
.tiny_int(42)
.tiny_list(1)
.tiny_string("Label")
.tiny_map(1)
.tiny_string("p")
.structure(3, b'F')
.int32(42)
.tiny_int(0)
.tiny_int(0)
.build(),
);
let mut node: NodeRef = from_bytes_ref(&mut data).unwrap();

assert_eq!(node.id(), 42);
assert_eq!(node.labels(), &["Label"]);
assert_eq!(node.element_id(), None);

assert_eq!(node.keys(), &["p"]);

let p = node.get::<LegacyDateTime>("p").unwrap().unwrap();
assert_eq!(p.seconds_since_epoch(), 42);
assert_eq!(p.nanoseconds(), 0);
assert_eq!(p.timezone_offset_seconds(), 0);
}
}
36 changes: 24 additions & 12 deletions lib/src/packstream/de.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
use std::{fmt, marker::PhantomData};
use std::{any::type_name, fmt, marker::PhantomData};

use bytes::{Buf, Bytes};
use serde::{
de::{
self, value::SeqDeserializer, DeserializeSeed, EnumAccess, IntoDeserializer as _,
MapAccess, SeqAccess, VariantAccess, Visitor,
self, value::SeqDeserializer, DeserializeSeed, EnumAccess, IgnoredAny,
IntoDeserializer as _, MapAccess, SeqAccess, VariantAccess, Visitor,
},
forward_to_deserialize_any,
};
Expand Down Expand Up @@ -207,8 +207,10 @@ impl<'de> Deserializer<'de> {
let start = full_bytes.as_ptr();
let end = self.bytes.as_ptr();

// SAFETY: both pointers are from the same allocation and end is >= start
let len = unsafe { end.offset_from(start) };
full_bytes.truncate(len.unsigned_abs());

Ok(full_bytes)
}

Expand Down Expand Up @@ -358,20 +360,25 @@ impl<'a> ItemsParser<'a> {
self.excess = excess;
self
}

fn drain(&mut self) -> Result<(), Error> {
let bytes = self.bytes.get();
for _ in 0..(self.len + self.excess) {
Deserializer { bytes }.skip_next_item()?;
}
Ok(())
}
}

impl<'a, 'de> SeqAccess<'de> for ItemsParser<'a> {
impl<'de> SeqAccess<'de> for ItemsParser<'_> {
type Error = Error;

fn next_element_seed<T>(&mut self, seed: T) -> Result<Option<T::Value>, Self::Error>
where
T: DeserializeSeed<'de>,
{
if self.len == 0 {
let bytes = self.bytes.get();
for _ in 0..self.excess {
Deserializer { bytes }.skip_next_item()?;
}
self.drain()?;
return Ok(None);
}
self.len -= 1;
Expand All @@ -388,7 +395,7 @@ impl<'a, 'de> SeqAccess<'de> for ItemsParser<'a> {
}
}

impl<'a, 'de> MapAccess<'de> for ItemsParser<'a> {
impl<'de> MapAccess<'de> for ItemsParser<'_> {
type Error = Error;

fn next_key_seed<K>(&mut self, seed: K) -> Result<Option<K::Value>, Self::Error>
Expand Down Expand Up @@ -420,7 +427,7 @@ impl<'a, 'de> MapAccess<'de> for ItemsParser<'a> {
}
}

impl<'a, 'de> VariantAccess<'de> for ItemsParser<'a> {
impl<'de> VariantAccess<'de> for ItemsParser<'_> {
type Error = Error;

fn unit_variant(mut self) -> Result<(), Self::Error> {
Expand All @@ -431,6 +438,11 @@ impl<'a, 'de> VariantAccess<'de> for ItemsParser<'a> {
where
T: DeserializeSeed<'de>,
{
if type_name::<T>() == type_name::<PhantomData<IgnoredAny>>() {
self.drain()?;
return seed.deserialize(().into_deserializer());
}

self.next_value_seed(seed)
}

Expand Down Expand Up @@ -498,14 +510,14 @@ struct SharedBytes<'a> {
}

#[cfg(all(test, debug_assertions))]
impl<'a> fmt::Debug for SharedBytes<'a> {
impl fmt::Debug for SharedBytes<'_> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
crate::packstream::Dbg(unsafe { &*self.bytes }).fmt(f)
}
}

#[cfg(not(all(test, debug_assertions)))]
impl<'a> fmt::Debug for SharedBytes<'a> {
impl fmt::Debug for SharedBytes<'_> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.debug_struct("SharedBytes").finish_non_exhaustive()
}
Expand Down
23 changes: 23 additions & 0 deletions lib/tests/node_property_parsing.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
use chrono::{DateTime, FixedOffset};
use neo4rs::{query, Node};

mod container;

#[tokio::test]
async fn node_property_parsing() {
let neo4j = container::Neo4jContainer::new().await;
let graph = neo4j.graph();

graph
.run(query("CREATE (:A {p1:DATETIME('2024-12-31T08:10:35')})"))
.await
.unwrap();

let mut result = graph.execute(query("MATCH (p:A) RETURN p")).await.unwrap();

while let Ok(Some(row)) = result.next().await {
let node: Node = row.get("p").unwrap();
let p1 = node.get::<DateTime<FixedOffset>>("p1").unwrap();
assert_eq!(p1.timestamp(), 1735632635);
}
}

0 comments on commit d719bd1

Please sign in to comment.