ConsoleWriter mangles JSON order

[FGasper]

ConsoleWriter reorders JSON fields when printing them, even if the field was added via RawJSON().

It should instead preserve the order of those fields.

[timmysutanto]

Hi, I've been using zerolog and ran into the same issue. When I dive into the source code, I've found the lines that cause the issue link

if len(w.FieldsOrder) > 0 {
	w.orderFields(fields)  
} else {  
	sort.Strings(fields)  
}

Perhaps adding a config like w.IsAutoOrdered would help? I'd gladly create a PR if necessary. Thanks

[IDisposable]

Of note, this only changes the order of fields what ConsoleWriter emits. The underlying log data is not reordered at all.

Several things need to change here:

• In orderFields we need to use sort.SliceStable to ensure we don't move things around for duplicate fields.
• To make it conditional, we would still have to honor the w.FieldsOrder, so we really should just always call that method
• For the case where we would want the non-ordered (e.g. not named in w.FieldsOrder) fields to be left alone, would need a bool like DoNotSortFields that would default to false for current standard behavior
• The "forced" reordering that puts the ErrorFieldName field first could be handled in orderFields as well if we always call it

ConsoleWriter gets this addition

	// DoNotSortFields does not sort contextual fields alphabetically (default false).
	DoNotSortFields bool

...then orderFields would look like

// orderFields takes an array of field names and an array representing field order
// and returns an array with any ordered fields at the beginning, in order,
// and the remaining fields after in their original order or sorted alphabetically.
func (w ConsoleWriter) orderFields(fields []string) {
	if w.fieldIsOrdered == nil {
		ordered := make(map[string]int)
		for i, fieldName := range w.FieldsOrder {
			ordered[fieldName] = i
		}
		ordered[ErrorFieldName] = -1 // Always put error field first
		w.fieldIsOrdered = ordered
	}

	indexes := w.fieldIsOrdered

	if w.DoNotSortFields {
		offset := len(indexes)
		newindexes := make(map[string]int, offset)
		for fieldName, index := range indexes {
			newindexes[fieldName] = index
		}

		for i, field := range fields {
			if _, ok := newindexes[field]; !ok {
				newindexes[field] = offset + i
			}
		}

		indexes = newindexes
	}

	sort.SliceStable(fields, func(i, j int) bool {
		ii, iOrdered := indexes[fields[i]]
		jj, jOrdered := indexes[fields[j]]
		if iOrdered && jOrdered {
			return ii < jj
		}
		if iOrdered {
			return true
		}
		if jOrdered {
			return false
		}
		return fields[i] < fields[j]
	})
 }

... and we could delete this block of code in writeFields

	if len(w.FieldsOrder) > 0 {
		w.orderFields(fields)
	} else {
		sort.Strings(fields)
	}

	// Move the "error" field to the front
	ei := sort.Search(len(fields), func(i int) bool { return fields[i] >= ErrorFieldName })
	if ei < len(fields) && fields[ei] == ErrorFieldName {
		fields[ei] = ""
		fields = append([]string{ErrorFieldName}, fields...)
		var xfields = make([]string, 0, len(fields))
		for _, field := range fields {
			if field == "" { // Skip empty fields
				continue
			}
			xfields = append(xfields, field)
		}
		fields = xfields
	}

... and replace with just

	w.orderFields(fields)

I've started a draft PR #739, but it randomly (every once in a while!) fails unit tests... I'm tracking that down now.

[IDisposable]

Well, after much agony with the random failures of the unit tests in PR #739... it turns out that we cannot guarantee the order of the fields actually matches the input order of the underlying Event. See comment