`Table::position` respected even if it means `ArrayOfTables` children are not rendered under their parent

[tachibanayui]

I'm using toml_edit to merge 2 toml documents for updating configuration file. Before serialization, I debug print the DocumentMut and the ast looks fine, but when calling the to_string() function, it produces invalid toml debug ast, source code:

[[edges]]
id = "edge1"
source = "inf5"
target = "inf3"

[[nodes]]
id = "inf"
type = "InputDeviceName"
[nodes.data]
matches = "Micro"
gain = 1.0
mode = "Regex"
channels = []

[nodes.position]
x = 0.0
y = 0.0

[[nodes]]
id = "inf2"
type = "InputDeviceName"
[nodes.data]
matches = "Speakers (High Definition Audio Device)"
mode = "Exact"
gain = 1.0
channels = []

[nodes.position]
x = 0.0
y = 249.0

[[nodes]]
id = "inf3"
type = "OutputDeviceName"
[nodes.data]
matches = "Sp"
gain = 1.0
mode = "Regex"
channels = []

[[nodes]]
id = "inf4"
type = "ProcessCaptureName"

[nodes.position]
x = 291.0
y = 747.0
[nodes.data]
matches = "osu"
gain = 1.0
mode = "Regex"
channels = []

[[nodes]]
id = "inf5"
type = "ProcessCaptureWinTitle"
[nodes.data]
matches = "BSPow"
gain = 1.0
mode = "Regex"
channels = []

[nodes.position]
x = 0.0
y = 498.0

[nodes.position]
x = 0.0
y = 747.0

seems like "inf3" node position had been "pushed down" to "inf4" node which cascade down to others node under it. I tried to fix this by hand and do another debug print and use a diff tool and nothing could indicate a problem.

[epage]

Can you provide a minimal, yet complete reproduction case?

For instance, if you could have a single file program that contains all of the inputs, that alone would be a big help. As of right now, there is insufficient information for reproducing this with out me doing a lot of experiments.

Peeling off unnecessary steps from that program would also be a big help!

[tachibanayui]

Hi @epage, I have made a minimal reproduction on this repo, I used include_str!(...) to embed the sample input files so it couldn't be a single file program.

[epage]

A single file reproduction case:

#!/usr/bin/env nargo
---
[dependencies]
toml_edit = { version = "0.24", features = ["serde"] }
serde = "1"
---

use std::mem;

use serde::Serialize;
use toml_edit::{DocumentMut, Item, Table, Value};

fn main() {
    let current_str = r#"
[[edges]]
id = "edge1"
source = "inf5"
target = "inf3"

[[nodes]]
id = "inf"
type = "InputDeviceName"
[nodes.data]
matches = "Micro"
gain = 1.0

[[nodes]]
id = "inf2"
type = "InputDeviceName"
[nodes.data]
matches = "Speakers (High Definition Audio Device)"
mode = "Exact"
gain = 1.0

[[nodes]]
id = "inf3"
type = "OutputDeviceName"
[nodes.data]
matches = "Sp"
gain = 1.0

[[nodes]]
id = "inf4"
type = "ProcessCaptureName"
[nodes.data]
matches = "osu"
gain = 1.0

[[nodes]]
id = "inf5"
type = "ProcessCaptureWinTitle"
[nodes.data]
matches = "BSPow"
gain = 1.0
"#;
    let changes_str = r#"
[[edges]]
id = "edge1"
source = "inf5"
target = "inf3"

[[nodes]]
id = "inf"
type = "InputDeviceName"

[nodes.position]
x = 0.0
y = 0.0

[nodes.data]
matches = "Micro"
mode = "Regex"
gain = 1.0
channels = []

[[nodes]]
id = "inf2"
type = "InputDeviceName"

[nodes.position]
x = 0.0
y = 249.0

[nodes.data]
matches = "Speakers (High Definition Audio Device)"
mode = "Exact"
gain = 1.0
channels = []

[[nodes]]
id = "inf3"
type = "OutputDeviceName"

[nodes.position]
x = 291.0
y = 747.0

[nodes.data]
matches = "Sp"
mode = "Regex"
gain = 1.0
channels = []

[[nodes]]
id = "inf4"
type = "ProcessCaptureName"

[nodes.position]
x = 0.0
y = 498.0

[nodes.data]
matches = "osu"
mode = "Regex"
gain = 1.0
channels = []

[[nodes]]
id = "inf5"
type = "ProcessCaptureWinTitle"

[nodes.position]
x = 0.0
y = 747.0

[nodes.data]
matches = "BSPow"
mode = "Regex"
gain = 1.0
channels = []
"#;

    let mut doc: DocumentMut = current_str.parse().unwrap();
    let new_item: DocumentMut = changes_str.parse().unwrap();
    merge_items((&mut doc).as_item_mut(), dbg!(new_item.into_item()));
    let rendered = doc.to_string();
    println!("final: \n{}", doc);
    let _: DocumentMut = rendered.parse().unwrap();
}

/// Merge `src` into `dst`, preserving formatting & comments.
fn merge_items(mut dst: &mut Item, src: Item) {
    match (&mut dst, src) {
        // Merge tables
        (Item::Table(dst_tbl), Item::Table(src_tbl)) => {
            merge_table(dst_tbl, src_tbl);
        }

        (Item::Table(_), Item::Value(Value::InlineTable(src_itbl))) => {
            let tf = src_itbl.into_table();
            merge_items(dst, Item::Table(tf));
        }

        // Upgrade inline table to full table
        (Item::Value(Value::InlineTable(dst_itbl)), src @ Item::Table(_)) => {
            let mut tf = dst_itbl.clone().into_table();
            *tf.decor_mut() = dst_itbl.decor().clone();
            *dst = Item::Table(tf);
            merge_items(dst, src);
        }

        // Merge arrays of tables `[[]]`
        (Item::ArrayOfTables(dst_arr), Item::ArrayOfTables(src_arr)) => {
            let mut diter = dst_arr.iter_mut().enumerate();
            let mut siter = src_arr.into_iter();

            loop {
                match (diter.next(), siter.next()) {
                    (None, None) => break,
                    (None, Some(new_item)) => {
                        drop(diter); // diter have a destructor so we need to drop it now
                        dst_arr.push(new_item);

                        for rest in siter {
                            dst_arr.push(rest);
                        }

                        break;
                    }
                    (Some((idx, _)), None) => {
                        drop(diter); // diter have a destructor so we need to drop it now
                        let mut idx2 = 0;
                        dst_arr.retain(|_| {
                            idx2 += 1;
                            idx2 > idx
                        });

                        break;
                    }
                    (Some((_, dst)), Some(src)) => {
                        merge_table(dst, src);
                    }
                }
            }
        }

        (Item::ArrayOfTables(_), Item::Value(Value::Array(mut a)))
            if a.iter().all(|x| x.is_inline_table()) =>
        {
            let decor = mem::take(a.decor_mut());
            if a.is_empty() {
                return;
            }

            let mut src_aot = Item::Value(Value::Array(a))
                .into_array_of_tables()
                .expect("if condition should rule out");
            src_aot.get_mut(0).map(|x| *x.decor_mut() = decor);
            let src_item = Item::ArrayOfTables(src_aot);
            merge_items(dst, src_item);
        }

        (Item::Value(Value::Array(a)), src @ Item::ArrayOfTables(_))
            if a.iter().all(|x| x.is_inline_table()) =>
        {
            let owned_dst = mem::replace(dst, Item::None)
                .into_array_of_tables()
                .map(|x| Item::ArrayOfTables(x))
                .expect("if condition should rule out");
            *dst = owned_dst;
            merge_items(dst, src);
        }

        (Item::Value(Value::Array(dst_arr)), Item::Value(Value::Array(src_arr))) => {
            let mut diter = dst_arr.iter_mut().enumerate();
            let mut siter = src_arr.into_iter();

            loop {
                match (diter.next(), siter.next()) {
                    (None, None) => break,
                    (None, Some(new_item)) => {
                        drop(diter); // diter have a destructor so we need to drop it now
                        dst_arr.push(new_item);

                        for rest in siter {
                            dst_arr.push(rest);
                        }

                        break;
                    }
                    (Some((idx, _)), None) => {
                        drop(diter); // diter have a destructor so we need to drop it now
                        let mut idx2 = 0;
                        dst_arr.retain(|_| {
                            idx2 += 1;
                            idx2 > idx
                        });

                        break;
                    }
                    (Some((_, dst)), Some(src)) => {
                        merge_value(dst, src);
                    }
                }
            }
        }

        // Primitive values
        (Item::Value(dst_val), Item::Value(src_val)) => {
            merge_value(dst_val, src_val);
        }

        //unmatched types -> replace
        (_, src) => {
            *dst = src;
            panic!("Something wrong!");
        }
    }
}

fn merge_value(dst_val: &mut Value, src_val: Value) {
    let decor = dst_val.decor().clone();
    *dst_val = src_val.clone();
    *dst_val.decor_mut() = decor;
}

fn merge_table(dst_tbl: &mut Table, src_tbl: Table) {
    for (k, v) in src_tbl.into_iter() {
        match dst_tbl.get_mut(dbg!(&k)) {
            Some(dst_child) => merge_items(dst_child, v),
            None => {
                dbg!("insert");
                dst_tbl.insert(&k, dbg!(v));
                println!("inserted repr: \n{} ", dst_tbl.to_string());
                dbg!(&dst_tbl);
            }
        }
    }
}

[epage]

Stripping this down somewhat I get:

#!/usr/bin/env nargo
---
[dependencies]
toml_edit = { version = "0.24", features = ["serde"] }
serde = "1"
---

use std::mem;

use serde::Serialize;
use toml_edit::{DocumentMut, Item, Table, Value};

fn main() {
    let current_str = r#"
[[nodes]]
id = "1"
[nodes.data]
id = "2"

[[nodes]]
id = "3"
[nodes.data]
id = "4"

[[nodes]]
id = "5"
[nodes.data]
id = "6"

[[nodes]]
id = "7"
[nodes.data]
id = "8"
"#;
    let changes_str = r#"
[[nodes]]
id = "9"

[nodes.data]
id = "10"

[nodes.position]
id = "11"

[[nodes]]
id = "12"

[nodes.data]
id = "13"

[nodes.position]
id = "14"

[[nodes]]
id = "15"

[nodes.data]
id = "16"

[nodes.position]
id = "17"

[[nodes]]
id = "18"

[nodes.data]
id = "19"

[nodes.position]
id = "20"
"#;

    let mut doc: DocumentMut = current_str.parse().unwrap();
    let new_item: DocumentMut = changes_str.parse().unwrap();
    merge_items(doc.as_item_mut(), new_item.into_item());
    dbg!(&doc);
    let rendered = doc.to_string();
    println!("final: \n{}", doc);
    let _: DocumentMut = rendered.parse().unwrap();
}

/// Merge `src` into `dst`, preserving formatting & comments.
fn merge_items(mut dst: &mut Item, src: Item) {
    match (&mut dst, src) {
        // Merge tables
        (Item::Table(dst_tbl), Item::Table(src_tbl)) => {
            merge_table(dst_tbl, src_tbl);
        }

        (Item::Table(_), Item::Value(Value::InlineTable(src_itbl))) => {
            let tf = src_itbl.into_table();
            merge_items(dst, Item::Table(tf));
        }

        // Upgrade inline table to full table
        (Item::Value(Value::InlineTable(dst_itbl)), src @ Item::Table(_)) => {
            let mut tf = dst_itbl.clone().into_table();
            *tf.decor_mut() = dst_itbl.decor().clone();
            *dst = Item::Table(tf);
            merge_items(dst, src);
        }

        // Merge arrays of tables `[[]]`
        (Item::ArrayOfTables(dst_arr), Item::ArrayOfTables(src_arr)) => {
            let mut diter = dst_arr.iter_mut().enumerate();
            let mut siter = src_arr.into_iter();

            loop {
                match (diter.next(), siter.next()) {
                    (None, None) => break,
                    (None, Some(new_item)) => {
                        drop(diter); // diter have a destructor so we need to drop it now
                        dst_arr.push(new_item);

                        for rest in siter {
                            dst_arr.push(rest);
                        }

                        break;
                    }
                    (Some((idx, _)), None) => {
                        drop(diter); // diter have a destructor so we need to drop it now
                        let mut idx2 = 0;
                        dst_arr.retain(|_| {
                            idx2 += 1;
                            idx2 > idx
                        });

                        break;
                    }
                    (Some((_, dst)), Some(src)) => {
                        merge_table(dst, src);
                    }
                }
            }
        }

        (Item::ArrayOfTables(_), Item::Value(Value::Array(mut a)))
            if a.iter().all(|x| x.is_inline_table()) =>
        {
            let decor = mem::take(a.decor_mut());
            if a.is_empty() {
                return;
            }

            let mut src_aot = Item::Value(Value::Array(a))
                .into_array_of_tables()
                .expect("if condition should rule out");
            src_aot.get_mut(0).map(|x| *x.decor_mut() = decor);
            let src_item = Item::ArrayOfTables(src_aot);
            merge_items(dst, src_item);
        }

        (Item::Value(Value::Array(a)), src @ Item::ArrayOfTables(_))
            if a.iter().all(|x| x.is_inline_table()) =>
        {
            let owned_dst = mem::replace(dst, Item::None)
                .into_array_of_tables()
                .map(|x| Item::ArrayOfTables(x))
                .expect("if condition should rule out");
            *dst = owned_dst;
            merge_items(dst, src);
        }

        (Item::Value(Value::Array(dst_arr)), Item::Value(Value::Array(src_arr))) => {
            let mut diter = dst_arr.iter_mut().enumerate();
            let mut siter = src_arr.into_iter();

            loop {
                match (diter.next(), siter.next()) {
                    (None, None) => break,
                    (None, Some(new_item)) => {
                        drop(diter); // diter have a destructor so we need to drop it now
                        dst_arr.push(new_item);

                        for rest in siter {
                            dst_arr.push(rest);
                        }

                        break;
                    }
                    (Some((idx, _)), None) => {
                        drop(diter); // diter have a destructor so we need to drop it now
                        let mut idx2 = 0;
                        dst_arr.retain(|_| {
                            idx2 += 1;
                            idx2 > idx
                        });

                        break;
                    }
                    (Some((_, dst)), Some(src)) => {
                        merge_value(dst, src);
                    }
                }
            }
        }

        // Primitive values
        (Item::Value(dst_val), Item::Value(src_val)) => {
            merge_value(dst_val, src_val);
        }

        //unmatched types -> replace
        (_, src) => {
            *dst = src;
            panic!("Something wrong!");
        }
    }
}

fn merge_value(dst_val: &mut Value, src_val: Value) {
    let decor = dst_val.decor().clone();
    *dst_val = src_val.clone();
    *dst_val.decor_mut() = decor;
}

fn merge_table(dst_tbl: &mut Table, src_tbl: Table) {
    for (k, v) in src_tbl.into_iter() {
        match dst_tbl.get_mut(&k) {
            Some(dst_child) => merge_items(dst_child, v),
            None => {
                dst_tbl.insert(&k, v);
            }
        }
    }
}

[epage]

I think I've got it down to the absolute minimal case:

#!/usr/bin/env nargo
---
[dependencies]
toml_edit = { version = "0.24", features = ["serde"] }
serde = "1"
---

use std::mem;

use serde::Serialize;
use toml_edit::{ArrayOfTables, DocumentMut, Item, Table, Value};

fn main() {
    let min_doc = DocumentMut::from(Table::from_iter([(
        "nodes",
        ArrayOfTables::from_iter([
            {
                let mut t = Table::from_iter([
                    ("id", Item::Value(Value::from("9"))),
                    (
                        "data",
                        Item::Table({
                            let mut t = Table::from_iter([("id", Value::from("10"))]);
                            t.set_position(Some(2));
                            t
                        }),
                    ),
                    (
                        "position",
                        Item::Table({
                            let mut t = Table::from_iter([("id", Value::from("11"))]);
                            t.set_position(Some(3));
                            t
                        }),
                    ),
                ]);
                t.set_position(Some(1));
                t
            },
            {
                let mut t = Table::from_iter([
                    ("id", Item::Value(Value::from("12"))),
                    (
                        "data",
                        Item::Table({
                            let mut t = Table::from_iter([("id", Value::from("13"))]);
                            t.set_position(Some(4));
                            t
                        }),
                    ),
                    (
                        "position",
                        Item::Table({
                            let mut t = Table::from_iter([("id", Value::from("14"))]);
                            t.set_position(Some(6));
                            t
                        }),
                    ),
                ]);
                t.set_position(Some(3));
                t
            },
            {
                let mut t = Table::from_iter([
                    ("id", Item::Value(Value::from("15"))),
                    (
                        "data",
                        Item::Table({
                            let mut t = Table::from_iter([("id", Value::from("16"))]);
                            t.set_position(Some(6));
                            t
                        }),
                    ),
                    (
                        "position",
                        Item::Table({
                            let mut t = Table::from_iter([("id", Value::from("17"))]);
                            t.set_position(Some(9));
                            t
                        }),
                    ),
                ]);
                t.set_position(Some(5));
                t
            },
            {
                let mut t = Table::from_iter([
                    ("id", Item::Value(Value::from("18"))),
                    (
                        "data",
                        Item::Table({
                            let mut t = Table::from_iter([("id", Value::from("19"))]);
                            t.set_position(Some(8));
                            t
                        }),
                    ),
                    (
                        "position",
                        Item::Table({
                            let mut t = Table::from_iter([("id", Value::from("20"))]);
                            t.set_position(Some(12));
                            t
                        }),
                    ),
                ]);
                t.set_position(Some(7));
                t
            },
        ]),
    )]));
    dbg!(&min_doc);
    let min_rendered = min_doc.to_string();
    println!("final: \n{}", min_doc);
    let _: DocumentMut = min_rendered.parse().unwrap();
}

[epage]

The root cause is that tables are being merged without any consideration for their position and toml_edit respects the position above all else.

Callers can handle this on their end by clearing all positions or calculating positions for how they want this to work.

The question is what, if anything, can be done on toml_edits end to help address this.

[tachibanayui]

Thanks for the analysis. If I understand correctly, toml_edit uses positions to order items even if it crosses structure boundaries?

[epage]

Correct

[tachibanayui]

I think maybe toml edit could maybe only apply position orderings only for item with in it structure boundaries (eg: only consider immediate children inside tables, lists should not be ordered and respect ast structure) or at the very least return an error when it detects invalid positions since it's hard to debug without knowing toml_edit internals.

By the way, do you know any libraries that handles merging toml documents like this?

[epage]

I think maybe toml edit could maybe only apply position orderings only for item with in it structure boundaries (eg: only consider immediate children inside tables, lists should not be ordered and respect ast structure) or at the very least return an error when it detects invalid positions since it's hard to debug without knowing toml_edit internals.

At a high level, I agree. It is more in structuring this all to work that needs to be figured out which is not trivial.

By the way, do you know any libraries that handles merging toml documents like this?

No. I feel like data structure merging can be context dependent. For example, you are merging arrays but others would instead append arrays.