-
Notifications
You must be signed in to change notification settings - Fork 259
feat: support serializing structs into $value
-named fields
#878
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #878 +/- ##
==========================================
- Coverage 60.74% 55.30% -5.45%
==========================================
Files 41 42 +1
Lines 16044 15335 -709
==========================================
- Hits 9746 8481 -1265
- Misses 6298 6854 +556
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This constraint was added with the idea that the data type name is not part of a type. So if serialize them, we actually introduce strings from air, which could lead to the oddity. When we serialize top-level struct, there are no problems: root tag name is a part of XML document, not part of the XML type (as defined by XSD) of this document. And we map rust types to XSD types. Type aliases also makes things difficult for understanding. Unlike type name, field names cannot be aliased. For example, in you example you would be able to deserialize two different types #[derive(Serialize)]
struct A {
c: String,
}
#[derive(Serialize)]
struct B {
c: String,
}
#[derive(Serialize)]
struct SoapEnvBody<T> {
#[serde(rename = "$value")]
body: T,
} from both XMLs to which each of them are serialized: <soapenv:Envelope>
<soapenv:Body>
<A><c>Data</c></A>
</soapenv:Body>
</soapenv:Envelope>
<soapenv:Envelope>
<soapenv:Body>
<B><c>Data</c></B>
</soapenv:Body>
</soapenv:Envelope> If you are not embarrassed by this, then I am ready to accept this PR, but I want explicitly clarify in the documentation, that although struct name is serialized, any element name anyway will be accepted as valid on deserialization, and mark, that this is supported only for use with generics. You can past your example with SOAP here. For usual you should use correct field names. I also want to point that you problem may be solved by manual implementation of the impl<T: serde::Serialize> serde::Serialize for SoapEnvBody<T> {
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
where S: serde::Serializer,
{
let mut state = serializer.serialize_map(None)?;
self.body.serialize(serde::__private::ser::FlatMapSerializer(&mut state))?;
serde::ser::SerializeMap::end(state)
// For reference, without #[serde(flatten)]:
// let mut state = serializer.serialize_struct("SoapEnvBody", 1)?;
// state.serialize_field("body", &self.body)?;
// serde::ser::SerializeStruct::end(state)
}
}
|
Thanks for taking your time and reviewing this!
I see! Thanks for clarifying that.
Sounds fair to me; I'll add a piece to the documentation mentioning this behavior. To be clear, this PR only introduces the serialization part of this behavior. The fact that deserialization would accept any element name is already present in the current version of the library. You should be able to verify this by running this example in // Run this example in `master`, not this PR's branch
#[derive(Serialize, Deserialize)]
struct A {
c: String,
}
#[derive(Serialize, Deserialize)]
struct B {
c: String,
}
#[derive(Serialize, Deserialize)]
struct Body<T> {
#[serde(rename = "$value")]
body: T,
}
#[derive(Serialize, Deserialize)]
struct Envelope<T> {
body: Body<T>,
}
let a = "<Envelope><body><A><c>Data</c></A></body></Envelope>";
let big_a: Envelope<A> = from_str(a).unwrap(); // Ok!
let big_b: Envelope<B> = from_str(a).unwrap(); // Ok!
Thanks for pointing that out. Of course, generally speaking, I could write a custom deserializer for most issues I'd encounter. In this particular case, I'd need to write it more than twice, as I have at least 3 structs which would need to support the same type of serialization as is fixed by this PR. At that point, I could write a macro to automate that, or contribute to the library itself. Seeing as it does not break any current behavior, I assumed it was fine to move this into the library itself. Please let me know otherwise! For the moment being, I'll be updating the docs. |
I merged it as #883. I was not able to push my edits to you branch, so new PR was needed. |
About this PR
This PR implemensts support for serializing
$value
-named struct fields. These fields can already be correctly deserialized, but causes this runtime error when attempted to be serialized:Rationale
For an integration I'm working on, I've got to consume a relatively slim SOAP API. All requests are expected to be wrapped around
<soapenv:Envelope><soapenv:Body> ... </soapenv:Body></soapenv:Envelope>
.This is very similar to the issue in #654, but that could be solved by using enums. However, for various reasons, my specific use case would benefit from using a generic field inside the
SoapEnvBody
struct, like so:However, that would produce an unwanted
<body>
tag. To get rid of it, I attempted to use#[serde(flatten)]
, but that unexpectedly gets rid of one of the inner tags as well. To add to the previous example:Long example
The output of that example is:
This output is missing
A
's tag. With the implementation provided in this PR for$value
, the above example can be modified like so:Previous example, swapping `flatten` for `rename = "$value"`
To produce the desired output (notice the
<A>
tag):Closing thoughts
Admittedly, this is a very simple solution, which just reuses what's already implemented. A lot of test comments mention how we "cannot" and "should not" wrap structs in this way, and that got me wondering if the comment was just referring to the current state, or if I'm missing any specific reason this should not be implemented. Considering the only tests that failed are exactly the tests that expect this behavior not to be implemented, I believe it's the former, but I'm open to hearing otherwise.
Also, some test modules include both
err!
andserialize_as!
macros which acutally behave differently, usually withserialize_as!
wrapping the input around a struct, map or something else, while the correspondingerr!
macro does not. This looks silly when reviewing the code changes, as it almost looks like I removed some tests and replaced them with completely different tests. That's not the case.Regarding Documentation
I have not updated the repository's documentation. I'd first like to see whether this is a wanted feature or not, before investing time into rewriting the docs. I'm fully open to updating them before merging this PR, though, I just wanted to get a general review first.