cannot combine prefix with deny_unknown_fields

[fbenkstein]

I would like to combine with_prefix with deny_unknown_fields:

#[derive(Serialize, Deserialize)]
#[serde(deny_unknown_fields)]
struct Match {
    #[serde(flatten, with = "prefix_player1")]
    player1: Player,
    #[serde(flatten, with = "prefix_player2")]
    player2: Player,
}

#[derive(Serialize, Deserialize)]
struct Player {
    name: String,
    votes: u64,
}

serde_with::with_prefix!(prefix_player1 "player1_");
serde_with::with_prefix!(prefix_player2 "player2_");

#[test]
fn deny_flatten_with_prefix() {
    let s = serde_json::to_string(&Match {
        player1: Player {
            name: "bob".to_string(),
            votes: 2,
        },
        player2: Player {
            name: "alice".to_string(),
            votes: 5,
        },
    })
    .unwrap();
    let m: Match = serde_json::from_str(&s).unwrap();
}

This doesn't seem possible at the moment:

test test::deny_flatten_with_prefix ... FAILED

failures:

---- test::deny_flatten_with_prefix stdout ----
thread 'test::deny_flatten_with_prefix' panicked at 'called `Result::unwrap()` on an `Err` value: Error("unknown field `player1_name`", line: 1, column: 81)', src/libcore/result.rs:997:5
note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

Is there a workaround? CC @dtolnay

[jonasbb]

Thanks for the interest. I will be searching for a way to fix the bug. I am not familiar with the internals of serde. so let's see.

Here is what I think so far:
The problem is that the deserialization code doesn't register that some fields are already processed. There is a __content field in the generated code which has to be "empty", or rather only contain None values.

On the serde side everything seems to be passed through the FlatMapDeserializer. If this deserializes a struct or enum, the options in the vec are consumed and set to None.
However, not so if it processes a map.

Now on the serde_with side the WithPrefix treats most things as a map.

serde_with/src/with_prefix.rs

Lines 419 to 427 in 4289ed4

	fn deserialize_any<V>(self, visitor: V) -> Result<V::Value, Self::Error>
	where
	V: Visitor<'de>,
	{
	self.delegate.deserialize_map(WithPrefix {
	delegate: visitor,
	prefix: self.prefix,
	})
	}

That's my current guess, why entries are not removed from this internal __content variable.

Now how to fix this I'm not sure. Currently, the WithPrefix is completely agnostic of whichever type it wraps. It simply add/removes the prefix.

[jonasbb]

I looked more into the underlying implementation to understand the issue better. Deserializing something as struct or enum does consume the values and ensures they cannot be processed twice. Processing it as a map will not consume the values. This was a deliberate change to serde to allow using the withprefix implementation (serde-rs/serde@5ee2fc0).

Now the withprefix wrapper deserilizes the data like a map. It has to do this, because it does not care about the wrapped type. It is fully based on checking or adding the prefix during runtime, which makes it possible to work with all types, not just types where the definition is known.

Changing the implementation to deserialize like a struct is not easy. The function signature requires a list of all fields which exist in that struct. Unfortunately, the type must be &'static [&' static str] which cannot be created during runtime without leaking memory. So, in order to support that, the list of all fields must be known at compile time.

It might be possible to add another variant of the withprefix, which is based on a custom derive. That way, inspecting all field names and adding a prefix would be possible. However, this would loose two benefits of the current approach. 1. The source of the wrapped type is not required. 2. You could currently instantiate the withprefix structs with a runtime determined prefix, if you are willing to write some deserialization code yourself.

The current workaround I see is this: serde-rs/serde#970 (comment)
That requires more manual work, but should work with deny_unknown_fields. It is basically the manual version of a derive macro.

I will document that restriction to make it clearer in the future. I am currently unsure if it would be benefitial to add a custom derive as well for this use case, partially just because then there would be two very similar ways to perform the same thing.

[fbenkstein]

Thank you so much for this investigation. Just for context: I wanted to have this feature to ensure that I don’t break an existing JSON based protocol while refactoring. I don’t think I can be of any practical help here but I’ll monitor this issue for any progress . Thanks again!