CertainLach / jrsonnet

Rust implementation of Jsonnet language
MIT License
302 stars 33 forks source link

Optionnal feature preserving the order of fields in objects #76

Closed biomorphe closed 2 years ago

biomorphe commented 2 years ago

Awesome work ! Your library is so fast and so easy to integrate with JS bindings (wasm-pack + wasm-bindgen).

I'm working on a project where object fields order matters : I use Jsonnet as templating tool like many others, and I need the output not to be alphanumerically sorted. This kind of enhancement is well known and also needed by the community, see : https://github.com/google/go-jsonnet/issues/222 https://github.com/google/jsonnet/issues/407 https://github.com/google/jsonnet/issues/903

I partially managed to fix it in google/jsonnet except for binary plus operator (merge), and didn't manage to do it with google/go-jsonnet where sort order becomes quite random... So I tried with jrsonnet and the result is just perfect and much more faster than others processors. I just had to comment this line https://github.com/CertainLach/jrsonnet/blob/master/crates/jrsonnet-evaluator/src/obj.rs#L160 . Could you please add an optionnal feature to preserve key ordering when processing ? This feature exists for serde_json, for example and is named preserve_order : https://docs.serde.rs/serde_json/enum.Value.html#variant.Object.

CertainLach commented 2 years ago

Removing sort is not enough, as internal data structures use hashmaps, and data still will be randomly reordered (through "random" is consistent, as jrsonnet has no hashdos mitigations)

It is possible to implement, but this violates jsonnet spec, and disallows some optimizations

Also, how should it work in cases like

{
a: 1,
b: 2, 
c: 3,
} + {
b: 4
}

?

In such simple case we know exactly how fields were initially ordered, but it becomes quite hard and unintuitive in presence of mixins

I dont think this may fit in scope of this project, as required changes are huge, and results in language, which is not jsonnet

I think you should just use other explicit ordering primitive instead, i.e dependency graph + toposort, or sorted key prefixes (01_key, 02_other_key, 03_yet_another_key)

biomorphe commented 2 years ago

With only sort removal (fields.sort_unstable()) and input :

{
  e: 1,
  d: 2,
  c: 3,
} + {
  b: 4,
} + {
  a: 5,
}

I get output :

{
  "e": 1,
  "d": 2,
  "c": 3,
  "b": 4,
  "a": 5
}

Which is fine for me and my usage. I'm not familiar with the optimizations you're talking about so I believe you, and I'm conscient it breaks jsonnet specs, that's why I suggested to implement it with an optionnal feature, called only when integrating as library, not impacting CLI itself. If it's not so easy, I will have to pull your sources, patch and build them to obtain the wanted result, not a so big deal. Thanks for your quick answer and your work on this library.

CertainLach commented 2 years ago

Even if it worked in this particular case, it should be broken with arbitrary input, as without sort_unstable fields are ordered by hash, not by original order

I have implemented experimental feature to preserve keys, as i see this feature is really requested: https://github.com/CertainLach/jrsonnet/commit/90e93cc51b3a7b39390bb4ef230ea4c866de36d7

In my implementation this is not only compile-time flag, but also arguments to std.fields/std.manifest*/CLI, as otherwise it will break other code, especially one which performs operations such as std.md5(std.manifestJsonEx(object))

Jrsonnet should be compiled with exp-preserve-order feature to use this: cargo build --release --features=exp-preserve-order

CLI usage:

jrsonnet --exp-preserve-order -e '{a: 1, b: 2, c: 3} + { d: 4, e: 5, a: 6 }'

Jsonnet usage:

std.manifestJsonEx({a: 1, b: 2, c: 3} + { d: 4, e: 5, a: 6 }, '   ', preserve_order = true)

In both cases

{
   "b": 2,
   "c": 3,
   "d": 4,
   "e": 5
   "a": 6,
}

Will be returned (a is last because it was overwritten later)

CertainLach commented 2 years ago

Released this feature as experimental in https://github.com/CertainLach/jrsonnet/releases/tag/v0.5.0-pre1-test Lets move all additional discussions to https://github.com/google/jsonnet/issues/903, as this i can't stabilize this in only mine implementation, it also should be implemented somewhere else