-
Notifications
You must be signed in to change notification settings - Fork 559
Port the synchronous event serialization core to Rust #19837
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
erikjohnston
wants to merge
26
commits into
develop
Choose a base branch
from
erikj/rust_event_serialization
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+1,528
−703
Open
Changes from all commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
4b0fd01
Port the BundledAggregations class to Rust
erikjohnston 40a2180
Split event serialization into async/sync parts
erikjohnston fc56cf9
Port the synchronous event serialization core to Rust
erikjohnston 7d5dc63
Add changelog
erikjohnston e6f7bf5
Fix misleading doc comment on JsonObject::as_map
erikjohnston a3c4302
Rename SerializeEventConfig.only_event_fields -> event_field_allowlist
erikjohnston d6110fa
Add field-level doc comments to SerializeEventConfig
erikjohnston 812180f
Rename serialize_core_inner -> serialize_event
erikjohnston 3aa6fe8
Clarify `event_id` comment
erikjohnston 722407f
Use `unsigned_mut` in age and update comment.
erikjohnston ebb587e
Use `unsigned_mut` for stripping room state
erikjohnston 1cb9fab
Comment why as i64 is safe
erikjohnston 32b0eaf
Comment on what transaction IDs are
erikjohnston a1cd8f3
Add Unsigned::age_ts function
erikjohnston 586368c
Create and use `Event::redacts()`
erikjohnston 3254557
Error if object_entry_mut finds non-object
erikjohnston a6c349e
Add create_config helper
erikjohnston 1abf5e7
Apply suggestions from code review
erikjohnston 7124eeb
Expand 'age' comment
erikjohnston 74616d6
simplify object_entry_mut
erikjohnston 18d488c
Add FIXME
erikjohnston 1ace531
Replace Clone with shallow_copy
erikjohnston 15a1eea
Merge remote-tracking branch 'origin/develop' into erikj/rust_event_s…
erikjohnston 64d33f3
Fix merge
erikjohnston 4c7d2b5
Revert "Replace Clone with shallow_copy"
erikjohnston 0efa5d5
Replace Clone with Py<..> references
erikjohnston File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Port the synchronous core of client event serialization to Rust. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,14 +17,16 @@ use std::{collections::BTreeMap, sync::Arc}; | |
|
|
||
| use pyo3::{ | ||
| exceptions::{PyKeyError, PyTypeError}, | ||
| prelude::Borrowed, | ||
| pyclass, pymethods, | ||
| types::{ | ||
| PyAnyMethods, PyIterator, PyList, PyListMethods, PyMapping, PySet, PySetMethods, PyTuple, | ||
| }, | ||
| Bound, IntoPyObject, IntoPyObjectExt, Py, PyAny, PyResult, Python, | ||
| Bound, FromPyObject, IntoPyObject, IntoPyObjectExt, Py, PyAny, PyErr, PyResult, Python, | ||
| }; | ||
| use pythonize::{depythonize, pythonize}; | ||
| use serde::{Deserialize, Serialize}; | ||
| use serde_json::Value; | ||
|
|
||
| /// A generic class for representing immutable JSON objects. | ||
| /// | ||
|
|
@@ -40,34 +42,46 @@ pub struct JsonObject { | |
| object: Arc<BTreeMap<Box<str>, serde_json::Value>>, | ||
| } | ||
|
|
||
| #[pymethods] | ||
| impl JsonObject { | ||
| #[new] | ||
| #[pyo3(signature = (content = None))] | ||
| fn new<'a, 'py>(content: Option<&'a Bound<'py, PyAny>>) -> PyResult<Self> { | ||
| let Some(content) = content else { | ||
| // If no content is provided, default to an empty object. | ||
| return Ok(Self::default()); | ||
| }; | ||
| // We implement `FromPyObject` to allow `JsonObject` to be used as function | ||
| // arguments. | ||
| impl<'py> FromPyObject<'_, 'py> for JsonObject { | ||
| type Error = PyErr; | ||
|
|
||
| if let Ok(content) = content.cast::<JsonObject>() { | ||
| // If the content is already a JsonObject, we can just clone the | ||
| // underlying map (this is safe as the object is immutable). | ||
| fn extract(ob: Borrowed<'_, 'py, PyAny>) -> Result<Self, Self::Error> { | ||
| // Fast path: already a JsonObject, so just share the underlying map | ||
| // (cheap, as it's immutable and behind an `Arc`). | ||
| if let Ok(obj) = ob.cast::<JsonObject>() { | ||
| return Ok(JsonObject { | ||
| object: content.get().object.clone(), | ||
| object: obj.get().object.clone(), | ||
| }); | ||
| } | ||
|
|
||
| let Ok(content) = content.cast::<PyMapping>() else { | ||
| return Err(PyTypeError::new_err("'content' must be a mapping")); | ||
| }; | ||
|
|
||
| // Use pythonize to try and convert from a mapping. | ||
| let content = depythonize(content)?; | ||
| Ok(Self { | ||
| object: Arc::new(content), | ||
| // Otherwise accept any mapping and convert it via pythonize. Unlike the | ||
| // `#[new]` constructor we don't accept `None` here: an absent value is | ||
| // represented as `Option<JsonObject>` at the field/argument level. | ||
| let mapping = ob | ||
| .cast::<PyMapping>() | ||
| .map_err(|_| PyTypeError::new_err("expected a mapping"))?; | ||
| let object: BTreeMap<Box<str>, Value> = depythonize(&mapping)?; | ||
| Ok(JsonObject { | ||
| object: Arc::new(object), | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| #[pymethods] | ||
| impl JsonObject { | ||
| #[new] | ||
| #[pyo3(signature = (content = None))] | ||
| fn new(content: Option<&Bound<'_, PyAny>>) -> PyResult<Self> { | ||
| match content { | ||
| // If no content is provided, default to an empty object. | ||
| None => Ok(Self::default()), | ||
| // Otherwise reuse the `FromPyObject` path, which accepts an | ||
| // existing `JsonObject` or any Python mapping. | ||
| Some(content) => JsonObject::extract(content.as_borrowed()), | ||
| } | ||
| } | ||
|
|
||
| fn __len__(&self) -> usize { | ||
| self.object.len() | ||
|
|
@@ -197,6 +211,29 @@ impl JsonObject { | |
| pub fn get_field(&self, key: &str) -> Option<&serde_json::Value> { | ||
| self.object.get(key) | ||
| } | ||
|
|
||
| /// Returns a reference to the underlying map of this object's entries. | ||
| pub fn as_map(&self) -> &BTreeMap<Box<str>, Value> { | ||
| &self.object | ||
| } | ||
|
Comment on lines
+215
to
+218
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unused Helpful?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, yeah it's become unused now. I think it's likely going to be useful in future, so am minded to leave it in now that we wrote it. |
||
|
|
||
| /// Whether the object has no entries. | ||
| pub fn is_empty(&self) -> bool { | ||
| self.object.is_empty() | ||
| } | ||
|
|
||
| pub fn iter(&self) -> impl Iterator<Item = (&Box<str>, &Value)> { | ||
| self.object.iter() | ||
| } | ||
| } | ||
|
|
||
| impl<'a> IntoIterator for &'a JsonObject { | ||
| type Item = (&'a Box<str>, &'a serde_json::Value); | ||
| type IntoIter = std::collections::btree_map::Iter<'a, Box<str>, serde_json::Value>; | ||
|
|
||
| fn into_iter(self) -> Self::IntoIter { | ||
| self.object.as_ref().iter() | ||
| } | ||
| } | ||
|
|
||
| /// Helper class returned by `JsonObject.keys()` to act as a view into the keys | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
Cloneseems like a new footgunThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, yes. Not really sure I see much alternative though.
If this was a pure Rust class we could get rid of the interior mutability and have a top level
Arc, however to make Python work we need to be able to get a mutable version ofEventInternalMetadataand that requires interior mutability.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we do what was previously suggested: "and must be deep-copied if the event is cloned"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm, suppose we could. Really, we want a reference to the same event here, in that we could also store it as a
Py<Event>(I somewhat want to avoid storing python references as then you have to partake in the GC).Event::deep_copyis really only intended for if you want a completely new copy of an event that you can edit (which is very rare, mostly we just share a reference to the same event).I suppose we could also implement a
shallow_clone()method instead, so as to force users to choose?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a
shallow_copyin 1ace531, though in practice it has highlighted a couple of things:ThreadAggregation) has to manually derive clone.FormattedEventthat aren't in anArc.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed tack and made the aggregations structs store a
Py<Event>for now, and dropped bothCloneandshallow_copy.It's not entirely clear to me if we have to implement
__traverse__, since we know there won't be any reference cycles between the relation classes and the Event. Nonetheless I've implemented it to make sure we don't accidentally leak memory.c.f. https://pyo3.rs/v0.28.3/class/protocols#garbage-collector-integration and https://docs.python.org/3/c-api/gcsupport.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is
FormattedEventmeant to still haveClone? and the docstring is accurate? (hasn't changed)Feels like using
Py<Event>means we're forever tying ourselves to Python when we want this code to stand on its own in a pure Rust codebase.To make it more clear, why aren't we also implementing
__clear__?Without
__clear__, it seems like thelatest_eventEventwill forever be stuck (can't be GC'ed)Do we care about PyPy at all?