-
Notifications
You must be signed in to change notification settings - Fork 590
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
Merged
Merged
Changes from 17 commits
Commits
Show all changes
36 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 417e38b
Remove clone
erikjohnston b752732
Note lack of __clear__
erikjohnston 0ab0578
Add FIXME
erikjohnston 50eb453
Better variable names for JsonObject::extract
erikjohnston 7581cb0
Remove stale Clone comment
erikjohnston d078b1d
Swap to constants
erikjohnston 8091722
Avoid double-peek
erikjohnston 34a42fd
event_field_allow_list includes everything if empty
erikjohnston 50d834b
Use get_all on SerializeEventConfig
erikjohnston 6ddd8f7
Minor comment
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> { | ||
|
erikjohnston marked this conversation as resolved.
Outdated
|
||
| // 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>() { | ||
|
erikjohnston marked this conversation as resolved.
Outdated
|
||
| 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.
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?
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.
Removed the clones.
I see this as a stepping stone while we figure out what we want to do about
Event. I'm somewhat leaning towards havingstruct Event(Arc<EventInner>), but that is a bigger refactor.Have added a comment to
__traverse__, but basically__clear__is only need to break reference cycles, which we know we don't have. (This is also the reason I'm not sure we need to implement__traverse__)We do, though I think we should be fine due to lack of reference cycles.
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 guess the outstanding question here is what tack do we take in this PR, I think we probably either want:
EventandFormattedEventto beArc<...>around an inner type, and have cheap clones.Py<..>references for nowUh oh!
There was an error while loading. Please reload this page.
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.
Strictly speaking, I don't think we need to implement
__traverse__or__clear__according to the docs:We could justify it with this note from the docs (for better debugging if the code evolves to ever have any cycles):
I think the framing of the comment on
__traverse__needs updating to reflect both of those points.I think we should also explain the PyPy distinction but doesn't matter because we don't store references and this is a best effort debugging aid.
Which means we go with "2. Keep
Py<..>references for now"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 don't think this point was addressed ^