Use ordered map to guarantee consistency of JSON ordering

[alecsammon]

Uses orderedmap instead of map[string]interface{} to maintain ordering of entries in the marshalled JSON.

I appreciate there are some disadvantages to this.

1. It adds an additional dependency.
2. It will add a little overhead - although I believe this will be very minimal based on the operations in orderedmap

As you note then JSON is not designed to be ordered and as such this is maybe a little redundant.
I've opened this as we are looking to use this library to replace the default json.Marshall across a large code base, and refactoring some tests due to the ordering change would be prohibitively difficult!

fixes #49

[coveralls]

coverage: 88.785% (+0.2%) from 88.626%
when pulling 5b80c45 on alecsammon:ordered_map
into 270e679 on liip:master.

[coveralls]

coverage: 88.785% (+0.2%) from 88.626%
when pulling 5b80c45 on alecsammon:ordered_map
into 270e679 on liip:master.

[coveralls]

coverage: 88.785% (+0.2%) from 88.626%
when pulling 26de22b on alecsammon:ordered_map
into 270e679 on liip:master.

[alecsammon]

as we use a map as the input the order of the result cannot be guaranteed.

Using JSONEq will ensure the json is correct (even if unordered)

[mweibel]

Looking at the benchmark we have in the repo. On my Macbook Pro M1 (10 cores) I get the following benchmark result on master:

goos: darwin
goarch: arm64
pkg: github.com/liip/sheriff/v2
BenchmarkModelsMarshaller_Marshal_NativeJSON-10           539744              2216 ns/op
BenchmarkModelsMarshaller_Marshal-10                       78115             15395 ns/op
PASS
ok      github.com/liip/sheriff/v2      3.777s

with this branch:

goos: darwin
goarch: arm64
pkg: github.com/liip/sheriff/v2
BenchmarkModelsMarshaller_Marshal_NativeJSON-10           543945              2211 ns/op
BenchmarkModelsMarshaller_Marshal-10                       43054             28691 ns/op
PASS
ok      github.com/liip/sheriff/v2      3.770s

when using github.com/wk8/go-ordered-map/v2 instead:

goos: darwin
goarch: arm64
pkg: github.com/liip/sheriff/v2
BenchmarkModelsMarshaller_Marshal_NativeJSON-10           542030              2218 ns/op
BenchmarkModelsMarshaller_Marshal-10                       53034             22425 ns/op
PASS
ok      github.com/liip/sheriff/v2      3.607s

branch: 46.34% slower
wk8/go-ordered-map: 31.35% slower

Now I guess for those who really need it, this might be acceptable. I wonder though if we should support both cases?

FYI this was the patch I used to use wk8/go-ordered-map instead:

diff --git a/sheriff.go b/sheriff.go
index 2c8a38e..e28aa18 100644
--- a/sheriff.go
+++ b/sheriff.go
@@ -8,7 +8,7 @@ import (
        "strings"

        "github.com/hashicorp/go-version"
-       "gitlab.com/c0b/go-ordered-json"
+       orderedmap "github.com/wk8/go-ordered-map/v2"
 )

 // Options determine which struct fields are being added to the output map.
@@ -83,7 +83,7 @@ func Marshal(options *Options, data interface{}) (interface{}, error) {
                return marshalValue(options, v)
        }

-       dest := ordered.NewOrderedMap()
+       dest := orderedmap.New[string, any]()

        for i := 0; i < t.NumField(); i++ {
                field := t.Field(i)
@@ -198,14 +198,9 @@ func Marshal(options *Options, data interface{}) (interface{}, error) {

                // when a composition field we want to bring the child
                // nodes to the top
-               nestedVal, ok := v.(*ordered.OrderedMap)
+               nestedVal, ok := v.(*orderedmap.OrderedMap[string, any])
                if isEmbeddedField && ok {
-                       iter := nestedVal.EntriesIter()
-                       for {
-                               pair, ok := iter()
-                               if !ok {
-                                       break
-                               }
+                       for pair := nestedVal.Newest(); pair != nil; pair = pair.Prev() {
                                dest.Set(pair.Key, pair.Value)
                        }
                } else {
@@ -274,7 +269,7 @@ func marshalValue(options *Options, v reflect.Value) (interface{}, error) {
                if mapKeys[0].Kind() != reflect.String {
                        return nil, MarshalInvalidTypeError{t: mapKeys[0].Kind(), data: val}
                }
-               dest := ordered.NewOrderedMap()
+               dest := orderedmap.New[string, any]()
                for _, key := range mapKeys {
                        d, err := marshalValue(options, v.MapIndex(key))
                        if err != nil {

what do you think about:

1. should we use kw8/go-ordered-map instead? Or what made you choose the other implementation?
2. should we support both code paths (i.e. original one with unordered, and new one with ordered)?

[alecsammon]

1. Yes - should definitely use kw8/go-ordered-map. I used gitlab.com/c0b/go-ordered-jso as I had used it before - didn't know the kw8 existed.
2. Given your benchmarking then I think it does make sense to have this as a option defaulted to false. I don't think it will be too complicated to modify to do this. If you're happy then I'll update the PR.

[mweibel]

Sounds great! Thanks 👍

[alecsammon]

One possibility for this would be to define an interface like this (needs better naming!)

type Map interface {
	Set(k string, v interface{})
	Each(f func(k string, v interface{}))
}

The default implementation could be something like this:

type StandardMap map[string]interface{}

func (m StandardMap) Set(k string, v interface{}) {
	m[k] = v
}

func (m StandardMap) Each(f func(k string, v interface{})) {
	for k, v := range m {
		f(k, v)
	}
}

Which would be included in this library.

You could then use configuration to allow passing in a different implementation. I.e. you could use

type OrderedMap struct {
	*orderedmap.OrderedMap[string, interface{}]
}

func NewOrderedMap() *OrderedMap {
	return &OrderedMap{orderedmap.New[string, interface{}]()}
}

func (om *OrderedMap) Set(k string, v interface{}) {
	om.OrderedMap.Set(k, v)
}

func (om *OrderedMap) Each(f func(k string, v interface{})) {
	for pair := om.Newest(); pair != nil; pair = pair.Prev() {
		f(pair.Key, pair.Value)
	}
}

This would mean consumers of the library could define their own implementation (documentation could be added with the above example).

This would then mean you don't have to add a new dependency to your library, and consumers could build whatever implementation they wanted

[mweibel]

true, that would be also a possibility. Wondering about the UX of it - I guess a README update would be in order to supply this possibility? I like it though. Out of curiosity - what kind of perf impact does this have if you compare current master and the changes with the Map interface (but using the default implementation)? You can run the benchmark using go test -bench .

[alecsammon]

Added to a new draft PR here so we can discuss: #52

Performance looks very similar between both.

[alecsammon]

closing in favour of #52