Skip to content
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

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

Open
t-cadet opened this issue Aug 10, 2021 · 17 comments
Open

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

t-cadet opened this issue Aug 10, 2021 · 17 comments

Comments

@t-cadet
Copy link

t-cadet commented Aug 10, 2021

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
,
@BurntSushi BurntSushi changed the title Panic when serializing nested struct decorated by #[serde(flatten)] serializing nested struct with #[serde(flatten)] is not supported Aug 10, 2021
@BurntSushi
Copy link
Owner

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.

@gootorov
Copy link

gootorov commented Sep 7, 2021

@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: #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.

@darrell-roberts
Copy link

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?

@BurntSushi
Copy link
Owner

Nope.

@497e0bdf29873
Copy link

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.

@Niedzwiedzw

This comment was marked as off-topic.

@blaas
Copy link

blaas commented Nov 21, 2022

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,
}

@BurntSushi
Copy link
Owner

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?

@blaas
Copy link

blaas commented Nov 21, 2022

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

  • Add support for #[serde(flatten)] #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 😄

@BurntSushi
Copy link
Owner

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.

@Elrendio
Copy link

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!

@BurntSushi
Copy link
Owner

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?

@Elrendio
Copy link

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

@BurntSushi
Copy link
Owner

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.

@dzmitry-lahoda
Copy link

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.

@izolyomi
Copy link

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?

@BurntSushi
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants