rust-csv icon indicating copy to clipboard operation
rust-csv copied to clipboard

serializing nested struct with #[serde(flatten)] is not supported

Open t-cadet opened this issue 2 years ago • 17 comments

What version of the csv crate are you using?

version = "1.1.6"

Briefly describe the question, bug or feature request.

The program panics when serializing a struct that contains a nested struct decorated by #[serde(flatten)]. I am not sure whether this is a bug or expected behavior but I would expect the program to output a csv. If it is expected are there any alternatives to obtain the desired output (without removing nesting) ?

Include a complete program demonstrating a problem.

use serde::Serialize;

fn main() {
    let mut w = csv::Writer::from_writer(std::io::stdout());
    w.serialize(Message::default()).unwrap();
}

#[derive(Serialize, Default)]
struct Message {
    content: String,
    #[serde(flatten)]
    to: Person,
}

#[derive(Serialize, Default)]
struct Person {
    name: String,
}

What is the observed behavior of the code above?

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Error(Serialize("serializing maps is not supported, if you have a use case, please file an issue at https://github.com/BurntSushi/rust-csv"))', src/main.rs:5:37
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

What is the expected or desired behavior of the code above?

content,name
,

t-cadet avatar Aug 10 '21 12:08 t-cadet

OK, so this isn't a panic. The CSV writer is returning an error, and you're unwrapping it, and that is what is causing the panic. I've changed the title to update this. A panic inside the CSV writer itself is a serious bug. But it returning an error is totally normal when you do something that it doesn't support.

If it is expected are there any alternatives to obtain the desired output (without removing nesting) ?

The alternative is to write your own Serde impls or change your type definitions to be structured in a way that you want.

I am not sure whether this is a bug or expected behavior

The error message is pretty clear I think: "serializing maps is not supported, if you have a use case."

It does ask you to open an issue. And there are many other issues related to serde(flatten). Unfortunately, there are impedance mismatches here that are difficult to resolve.

BurntSushi avatar Aug 10 '21 12:08 BurntSushi

@tristanCadet There is an experimental branch with support for that! I've been using it for a few months already and haven't had any issues. It's tracked over here: https://github.com/BurntSushi/rust-csv/pull/223. You can try it in your project by adding the following to your Cargo.toml:

csv = { git = "https://github.com/gootorov/rust-csv.git", rev = "31d1105f9ee50bf02dff178f20be4a1ec9fdff2d" }

I'd be great if you could try it too. If you have any issues, please report - that would really help.

gootorov avatar Sep 07 '21 17:09 gootorov

Hi, I just started using this library today and I ran into this issue having created struct references using #[serde(flatten)]. Is this feature support in the works?

darrell-roberts avatar Apr 29 '22 15:04 darrell-roberts

Nope.

BurntSushi avatar Apr 29 '22 15:04 BurntSushi

I also just run into this, trying to avoid the limitation with writing headers of a struct containing an array by flattening the struct. It would be great for this to be fixed.

497e0bdf29873 avatar Jul 01 '22 11:07 497e0bdf29873

I want this so much ;/

Niedzwiedzw avatar Aug 03 '22 11:08 Niedzwiedzw

Hi! I happen to have a use case that I didn't see mentioned anywhere until now:

I need to be able to serialize as extra CSV columns a generic struct provided by my crate's user (order of those columns doesn't matter in my case).

Expected output with the following example code:

a,b,custom
0,,

Here's an example code of what would be ideal (csv version 1.1.6):

use serde::Serialize;

fn main() {
    let mut w = csv::Writer::from_writer(std::io::stdout());
    w.serialize(Data::<CustomColumns>::default()).unwrap();
}

#[derive(Debug, Serialize, Default)]
struct Data<Extra: Serialize + Default> {
	a: usize,
	b: String,
	#[serde(flatten)]
	extra: Extra,
}

#[derive(Debug, Serialize, Default)]
struct CustomColumns {
    custom: String,
}

Which, naturally, fails with:

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Error(Serialize("serializing maps is not supported, if you have a use case, please file an issue at https://github.com/BurntSushi/rust-csv"))', src/main.rs:5:51

If I don't try to use #[serde(flatten)], as expected, it fails with:

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Error(Serialize("cannot serialize CustomColumns container inside struct when writing headers from structs"))', src/main.rs:5:51

Instead, I'm forced to workaround this situation by using a tuple struct + an intermediate inner struct for the consistent fields. This has a negative impact on ergonomics.

use serde::Serialize;

fn main() {
	let mut w = csv::Writer::from_writer(std::io::stdout());
	w.serialize(Data::<CustomColumns>::default()).unwrap();
}

#[derive(Debug, Serialize, Default)]
struct Data<Extra: Serialize + Default>(DataInner, Extra);

#[derive(Debug, Serialize, Default)]
struct DataInner {
	a: usize,
	b: String,
}

#[derive(Debug, Serialize, Default)]
struct CustomColumns {
	custom: String,
}

blaas avatar Nov 21 '22 09:11 blaas

Instead, I'm forced to workaround this situation by using a tuple struct + an intermediate inner struct for the consistent fields. This has a negative impact on ergonomics.

When you say "negative impact on ergonomics," I think that applies to your internal code and not the public API you present to your callers, right?

BurntSushi avatar Nov 21 '22 12:11 BurntSushi

Instead, I'm forced to workaround this situation by using a tuple struct + an intermediate inner struct for the consistent fields. This has a negative impact on ergonomics.

When you say "negative impact on ergonomics," I think that applies to your internal code and not the public API you present to your callers, right?

Those required workarounds impact negatively the ergonomics of both the csv crate and of my internal code.

You're right about it not being a matter of public interface of my crate here, since I can still expose to the user only the tailored struct I want, and then go through all the intermediate representations I need in the internal code.

It's starting to be a lot of boilerplate, though:

  • With support for #[serde(flatten)], 1 struct required: 1 public generic struct, serialized as-is
  • As things are currently, 3 structs required: 1 public generic struct + 1 intermediate generic tuple struct + 1 non-generic inner struct (see previous example)

I won't go over the reasons why one would want to reduce boilerplate in a codebase, but the support for #[serde(flatten)] would contribute to reducing it when working with the csv crate with similar needs.

Going forward

  • #223 gave a shot at implementing this feature. My company Stockly has been using Rust as its core backend language for 4 years. As user of the csv crate, our company would be happy to contribute to this feature, directly by cleaning the PR or by testing it in our codebase.
  • You expressed your concern regarding the definitive aspect of what is added to the csv crate on the PR I mentioned. If that's ok with you, the #[serde(flatten)] support could be feature-gated behind serde-flatten-unstable, or a similarly explicit name. That would allow the feature the be proof-tested by users explicitly opting-in.

If you're ok with that, Stockly can look at it before the end of the year :smile:

blaas avatar Nov 21 '22 15:11 blaas

Aye okay, understood.

Basically what it comes down to here is that serde(flatten) has proven to be both costly to implement and result in lots of confusion on the deserialization side of things. So if we can avoid adding it at all and the "only" cost is a bit of boiler plate, I'm probably pretty happy with that trade off.

I think the thing required here is not just a simple matter of doing the work to implement this, but to do a very thorough investigation of how it would work and what kinds of use cases it would support. In theory, your serde-flatten-unstable feature would let one sidestep that, and I'm sympathetic but that looks like a different kind of headache to me. It'd be one thing if I was able to be very responsive to user feedback and iterate on it quickly to get it stabilized. But the more likely scenario is that it will sit in its unstable state for a long while and folks will come to rely on it to the point that I can't really break it without pissing a bunch of people off.

I do appreciate the offer though.

BurntSushi avatar Nov 21 '22 15:11 BurntSushi

Hi @BurntSushi,

I feel like Stockly can provide solutions to the pain points highlighted. We could:

  1. First provide an RFC-like document with a thorough investigation of how it would work and what kinds of use cases it would support.
  2. Once you've validated it, implement it and feature gate under serde-flatten-unstable
  3. Use it on our code base to iterate over the implementation
  4. Add a tag serde-flatten on all issues from user feedback. If you keep the triage of those issues we can commit to handling them.
  5. Once you feel enough time has passed without any issue we could stabilize it.

Would that work for you?

Thanks a have a nice day!

Elrendio avatar Nov 21 '22 19:11 Elrendio

If I had the bandwidth to engage with that kind of contribution, I would love it. But as of right now, the most likely outcome is that y'all post an RFC and I'm not able to meaningfully digest it for quite some time.

You can still go forward with it, but I just want to be very clear that my bandwidth is extremely low at present. Most of my focus is on the regex crate, and I don't anticipate that changing in the next few months. More to the point, there are several higher priority things that the csv crate needs. So if my focus were to shift to csv, it wouldn't be on this.

So I don't want to say "no I don't want your contribution," but I also don't want to say "yes and I will be very responsive to all your work and get everything merge and be an amazing maintainer." Does that make sense?

BurntSushi avatar Nov 21 '22 19:11 BurntSushi

Hi @BurntSushi

Perfectly clear thanks 😊 We'll move on with the RFC then and no pressure to review it.

As a side question, what are the higher priority things for the csv crate?

Thanks a have a nice day, Oscar

Elrendio avatar Nov 22 '22 09:11 Elrendio

CI needs to be fixed. I also need to decide how to straighten out the existing flatten feature for deserialization. And fix the dependency situation as there are a few that need to be updated.

There's probably more.

BurntSushi avatar Nov 22 '22 11:11 BurntSushi

Would make sense if serde attributes would get better support. I used JSON to CSV and CSV to JSON online tools, that flattened objects okeish. With flatten attribute, it can flatten names as in inner struct, in case of conflict fail. In case of no flatten, still can write prefixed names with some delimiter imho. That for serialize of objects to CSV.

dzmitry-lahoda avatar Mar 19 '23 15:03 dzmitry-lahoda

I've just hit this issue trying to deserialize a nested flattened struct, I really appreciate the related PR. Is there any chance to see a viable solution integrated into this crate? If not, is there a usable fork or alternative supporting this feature in a reliable way?

izolyomi avatar Jun 26 '23 09:06 izolyomi

@izolyomi There's a chance, sure. Multiple work arounds have been discussed above. You may not like them though.

BurntSushi avatar Jun 26 '23 10:06 BurntSushi