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

Optional feature preserving the order of fields in objects #76

Closed
biomorphe opened this issue Jan 25, 2022 · 7 comments
Closed

Optional feature preserving the order of fields in objects #76

biomorphe opened this issue Jan 25, 2022 · 7 comments

Comments

@biomorphe
Copy link

biomorphe commented Jan 25, 2022

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 :
google/go-jsonnet#222
google/jsonnet#407
google/jsonnet#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
Copy link
Owner

CertainLach commented Jan 25, 2022

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
Copy link
Author

biomorphe commented Jan 25, 2022

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 added a commit that referenced this issue Apr 20, 2022
@CertainLach
Copy link
Owner

CertainLach commented Apr 20, 2022

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: 90e93cc

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
Copy link
Owner

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

@ClausKlein
Copy link

what is the state of this feature?

iMac:test clausklein$ /Users/clausklein/Workspace/rust/jrsonnet/target/release/jrsonnet --preserve-order -e '{a: 1, b: 2, c: 3} + { d: 4, e: 5, a: 6 }'
{
   "a": 6,
   "b": 2,
   "c": 3,
   "d": 4,
   "e": 5
}
iMac:test clausklein$ /Users/clausklein/Workspace/rust/jrsonnet/target/release/jrsonnet --version
jrsonnet 0.5.0-pre96
iMac:test clausklein$ 

@CertainLach
Copy link
Owner

Works as intended, overriden field preserves its original ordering, for thinks like

{
apiVersion: error "set apiVersion",
spec: 1
} + { apiVersion: 1 }

To work correctly

@ClausKlein
Copy link

ClausKlein commented Oct 11, 2024

OK, this generate a sorted output:

# jrsonnet --preserve-order CMakeUserPresets.jsonnet > CMakeUserPresets.json

local archs = [
  'native',
  # 'x86_64',
  # 'aarch64',
];

local types = [
  'default',
  'asan',
];

local link_modes = [
  'static',
  'dynamic',
];

local configs = [
  'Debug',
  'Release',
];

local cp_generator(arch, type, link_mode) =
  {
    name: arch + '-' + type + '-' + link_mode,
    inherits: [type, link_mode],
  };

local bp_generator(arch, type, link_mode, config) =
  {
    name: arch + '-' + type + '-' + link_mode + '-' + std.asciiLower(config),
    configurePreset: arch + '-' + type + '-' + link_mode,
    configuration: config,
  };

local tp_generator(arch, type, link_mode, config) =
  {
    name: arch + '-' + type + '-' + link_mode + '-' + std.asciiLower(config),
    configurePreset: arch + '-' + type + '-' + link_mode,
    output: { outputOnFailure: true },
    execution: { noTestsAction: 'ignore', stopOnFailure: true },
  };

local wp_generator(arch, type, link_mode, config) =
  {
    name: arch + '-' + type + '-' + link_mode + '-' + std.asciiLower(config),
    steps: [
      {
        type: 'configure',
        name: arch + '-' + type + '-' + link_mode,
      },
      {
        type: 'build',
        name: arch + '-' + type + '-' + link_mode + '-' + std.asciiLower(config),
      },
      {
        type: 'test',
        name: arch + '-' + type + '-' + link_mode + '-' + std.asciiLower(config),
      },
      {
        type: 'package',
        name: arch + '-' + type + '-' + link_mode + '-' + std.asciiLower(config),
      },
    ],
  };

local pp_generator(arch, type, link_mode, config) =
  {
    name: arch + '-' + type + '-' + link_mode + '-' + std.asciiLower(config),
    configurePreset: arch + '-' + type + '-' + link_mode,
    generators: [
      'TGZ',
    ],
    "configurations": [
      config
    ],
  };

{
  version: 6,
  cmakeMinimumRequired: {
    major: 3,
    minor: 25,
    patch: 0,
  },
  configurePresets: [
    {
      name: 'default',
      hidden: true,
      displayName: 'Default Config',
      description: 'Default build using Ninja Multi-Config generator',
      generator: 'Ninja Multi-Config',
      binaryDir: '${sourceDir}/build/${presetName}',
      cacheVariables: {
        CMAKE_EXPORT_COMPILE_COMMANDS: true,
        CPM_USE_LOCAL_PACKAGES: true,
        BUILD_TESTING: true,
      },
    },
    {
      name: 'asan',
      inherits: 'default',
      hidden: true,
      cacheVariables: {
        CMAKE_CXX_FLAGS_SANITIZE: '-U_FORTIFY_SOURCE -O2 -g -fsanitize=address,undefined -fno-omit-frame-pointer -fno-common',
      },
    },
    {
      name: 'static',
      hidden: true,
      cacheVariables: {
        'BUILD_SHARED_LIBS': false,
      },
    },
    {
      name: 'dynamic',
      hidden: true,
      cacheVariables: {
        'BUILD_SHARED_LIBS': true,
      },
    },
  ] + [cp_generator(arch, type, link_mode) for arch in archs for type in types for link_mode in link_modes],

  buildPresets: [] + [bp_generator(arch, type, link_mode, config) for arch in archs for type in types for link_mode in link_modes for config in configs],
  testPresets: [] + [tp_generator(arch, type, link_mode, config) for arch in archs for type in types for link_mode in link_modes for config in configs],
  packagePresets: [] + [pp_generator(arch, type, link_mode, config) for arch in archs for type in types for link_mode in link_modes for config in configs],
  workflowPresets: [] + [wp_generator(arch, type, link_mode, config) for arch in archs for type in types for link_mode in link_modes for config in configs],
}

@JarvisCraft JarvisCraft changed the title Optionnal feature preserving the order of fields in objects Optional feature preserving the order of fields in objects Oct 11, 2024
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

3 participants