de: consider local name only for namespaced tags in structs with $value

[Xiphoseer]

This updates the "not_in" check that decides whether to pass a new start tag within a struct to a $value field, to only consider the local part of a QName. It now uses the same decode_name function as the QNameDeserializer that is used for keys/fields already to ensure they stay in sync.

Using the namespaced name in serde (i.e. #[serde(rename = "ns1:tag")]) fails with Custom("missing field `ns1:tag`") today, so this will not break existing code.

Might help with #347

[Xiphoseer]

Looks like I did not enable the "serialize" feature when running tests locally and got confused by the double negative.

I.e. not_in should say

yes, ns1:tag does not appear in [ some, namespace, ns1:tag ]

because the former is considered as tag and the latter is not yet supported

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.51%. Comparing base (29962e7) to head (745278a).
Report is 3 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #736      +/-   ##
==========================================
+ Coverage   61.48%   61.51%   +0.03%     
==========================================
  Files          38       38              
  Lines       16213    16224      +11     
==========================================
+ Hits         9968     9981      +13     
+ Misses       6245     6243       -2     

Flag	Coverage Δ
unittests	61.51% <100.00%> (+0.03%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Mingun]

Could you test if this actually fixes #347? Add regression test into ./tests/serde-issues.rs. Try to keep original code as much as possible to be sure that's user problem is actually solved, but instead of printing results use assertions. That test is needed only if #347 is fixed.

If your patch does not fix the error, then I would like to see a test which will demonstrate working of this fix, so it will not break in the future (you are also free to add it anyway even if regression test fit our needs).

I also interested to check how it behaves with serialization. Ideally, we should be able to deserialize serialized data. I'm not sure that this is always true now, but we strive for this. So if your patch fix that aspect, it also would be welcome to be presented in a test.

When test will demonstrate changes that was made, please add an entry to Changelog.md with a short description of what is fixed.

[Mingun]

Move that function to de/mod.rs instead, so it will be available in all submodules.

[Xiphoseer]

Hey, thank you for the review. My case is probably closer to #212, but that was closed as a duplicate of #347.

I agree that a regression test would be useful so I'll add one when I find time for it. This change doesn't do anything to fix serialization, but it tries to improve upon the current behavior of only considering the local part of a QName for deserialization.

My actual case is as follows:

<ns1:parent>
    <ns1:field />
    <ns1:one />
</ns1:parent>

with the following data model

#[derive(Deserialize)]
#[serde[renameAll = "camelCase")]
enum Choice {
  One,
  Two
}

#[derive(Deserialize)]
struct Field;

#[derive(Deserialize)]
#[serde[renameAll = "camelCase")]
struct Parent {
  field: Field,
  #[serde(rename = "$value")]
  choice: Vec<Choice>,
}

Today, this will try to find "field" as a variant in "Choice" because the not_in && has_value_field case triggers when deserializing the struct.

With the change in this PR, it will correctly identify "field" as a key on the struct even for "ns1:field".

Renaming the field to "ns1:field" via serde does not work, which I guess is what #347 is about: The not_in would trigger but the QNameDeserializer will pick just the local part and fail to find the field so you get "missing field" errors.

I imagine that adding a toggle in Deserializer (use local name vs full name for decode_name) could be a backwards compatible change to resolve #347.

[Mingun]

I checked, and this PR does not solve #347, but solve #683, because it actually does the same thing as #702.

Unfortunately, #683 and #347 seems to contradict each other. #347 requires to consider prefix part of a tag. Well, it should be possible to fix both issues, but this simple fix just solve the #683 case, but makes us a little more far from solving #347 (in particular, this change makes it impossible to deal with that example).

I would prefer to investigate possibilities to solve both cases (#683 and #347).

cc @leftmostcat