Skip to content

feat: btreemap#125

Open
HairlessVillager wants to merge 1 commit intoowengage:masterfrom
HairlessVillager:master
Open

feat: btreemap#125
HairlessVillager wants to merge 1 commit intoowengage:masterfrom
HairlessVillager:master

Conversation

@HairlessVillager
Copy link

This PR introduces a new btreemap feature. When this feature is enabled, the HashMap in fastnbt::Value::Compound will be replaced with a BTreeMap. This feature is disabled by default, so it is expected not to conflict with any existing code.

Motivation

In certain scenarios, I need to ensure that the serialization result of Value is consistent. Here is an example:

#[test]
fn test_fastnbt_works() {
    use fastnbt::{Value, nbt};
    let x = nbt!({
        "string": "Hello World",
        "number": 42,
        "nested": {
            "array": [1, 2, 3, 4, 5],
            "compound": {
                "name": "test",
                "value": 3.14,
                "list": ["a", "b", "c"]
            }
        },
        "boolean": 1_i8,
        "long_array": [1_i64, 2_i64, 3_i64]
    });

    let y = fastnbt::to_bytes(&x).unwrap();
    let z: Value = fastnbt::from_bytes(&y).unwrap();
    let w = fastnbt::to_bytes(&z).unwrap();
    assert_eq!(x, z);
    assert_eq!(y, w);
}

In the previous implementation, since HashMap does not guarantee iteration order, the serialization result of the same Value might be different. That is, in the above test, the assertion x == z passes, but the assertion y == w fails.

I tried to use a custom Value type to solve this problem, but it resulted in an error Error: Error("invalid nbt: no root compound") (see #124).

Todo list

The following tasks may still need to be completed for this PR:

  • Relevant documentation
  • Relevant tests

Although I can complete the above tasks myself, I would still like to get feedback from the project maintainers before proceeding, in order to avoid unnecessary work.

@owengage
Copy link
Owner

Hi, sorry for the long delay.

I'm not entirely sure how to go about implementing this, I think it would have to be an entirely new value type to be backwards compatible. You're usage of features is not additive, which would potentially break the crate for others, see these cargo docs on feature unification.

@HairlessVillager
Copy link
Author

Thank you for your feedback! I agree with your concerns about backward compatibility and the additive principle.

If we were to introduce an additional type called OrderedValue, which would be parallel to Value, this approach would ensure backward compatibility with the existing Value and the package's features. Consequently, it should have minimal impact on downstream application developers.

At that time, the code would feature definitions like this:

#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
pub enum OrderedValue {
    Byte(i8),
    Short(i16),
    Int(i32),
    Long(i64),
    Float(f32),
    Double(f64),
    String(String),
    ByteArray(ByteArray),
    IntArray(IntArray),
    LongArray(LongArray),
    List(Vec<OrderedValue>),
    Compound(BTreeMap<String, OrderedValue>), // NOTE: BTreeMap here
}

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

Successfully merging this pull request may close these issues.

2 participants