wooorm / markdown-rs

CommonMark compliant markdown parser in Rust with ASTs and extensions
https://docs.rs/markdown/1.0.0-alpha.21/markdown/
MIT License
906 stars 50 forks source link

Can't deserialize mdast #56

Closed ales-tsurko closed 8 months ago

ales-tsurko commented 1 year ago

I serialized it using serde_json (also tried bincode, but anyway it's serde-based), but when I deserialize it, I have errors saying that it's expecting 'Root' instead of 'root' etc. I forked the project and removed all serde attributes related to field renaming and it fixed the issue. But I believe those attributes are there not just because you wanted them to be there :)

wooorm commented 1 year ago

Can you provide code? I don't know where you get that error.

ales-tsurko commented 1 year ago

Well, it is as I described, just serialize it and deserialize:

use markdown::*;

fn main() {
    let mdast = to_mdast("# Hello", &ParseOptions::default()).unwrap();
    let ser = serde_json::to_string(&mdast).unwrap();
    let _: mdast::Node = serde_json::from_str(&ser).unwrap();
}

Panics with:

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Error("duplicate field `type`", line: 1, column: 21)', src/main.rs:6:53
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

It panics for Root as well. It panics with different errors depending on input.

ales-tsurko commented 1 year ago

I "fixed" it on my fork by removing additional serde attributes for some fields: https://github.com/wooorm/markdown-rs/commit/2fac09bf9244fed6b687093e4aed09394ca5f73b

I mean, the reason is in the renaming and setting additional fields.

wooorm commented 1 year ago

Thanks for the repro!

If invalid JSON is produced, that serde can serialize but not parse, that sounds like a bug with serde to.

But, this still points to an inconsistency in the objects we choose to produce. There are two type field, apparently.

The renaming is important. We should produce mdast here. Removing all those fields does not produce valid mdast.

If you’re interested in improving this, a PR would be welcome. It would have to solve your issue, and also follow mdast: https://github.com/syntax-tree/mdast

wooorm commented 8 months ago

This this was solved with https://github.com/wooorm/markdown-rs/issues/58, https://github.com/wooorm/markdown-rs/commit/bbb311979c9e9b7305c855c5b93493d285c6ac7b.