fix(types) ignore tag order for object comparisons #240
Conversation
There is development history behind it. In an early PoC of decK, the code looked a lot different than it is today and there was a DAG built to solve the diffing problem. It turned out to be over-engineered. The code simplification kept major parts of the codebase, including this. There are no parts of code that care about timestamps in diffing.
We don't but these functions are now diverging. Order of tags should be ignored no matter what. I'd recommend the following refactor as part of this change: fn Equal() {
return EqualWithOpt(with, default, arguments)
}
A quick grep says no. |
|
Consider the PR approved once you add in tests and refactor the Equal fns. |
Ignore tag order for object comparisons by sorting the string slices that contain them before checking object equality. This avoids issues with spurious diffs resulting in unnecessary changes when the tag order constructed by deck from dump YAML and the tag order reported by admin API JSON differ. Because Kong object tags are logical string sets, their order does not matter: it is only a perceived difference because JSON and YAML lack set types and represent them with ordered arrays. Fix #239
* Add a function to generate a set of test tags with different orders. * Add test assertions that confirm objects are considered equal when they have the same set of tags, even when the tag order differs.
rainest
force-pushed
the
fix/sort-tags
branch
2 times, most recently
from
f8771a7
to
c2efe87
Compare
|
Double-checking on the refactor, but I think we're good to go here. Grepping shows that we actually do have ID checks enabled in current calls, though presumably most users aren't using The complete signatures follow a Have made that the default in the Not sure if there's some more idiomatic way to handle the tag test without creating the test |
Make object Equal() checks a passthrough to EqualWithOpts() with default options (ignore nothing). This does ignore tag order, since tag order comparisons aren't actually desirable but do happen as a consequence of the data structure we use for them. Passing through allows us to reuse the object copy and modify logic in EqualWithOpts(). Previously Equal() immediately returned reflect.DeepEqual() and we'd need to duplicate copy/modify logic in Equal() to ensure tag order invariance otherwise.
rainest
force-pushed
the
fix/sort-tags
branch
from
c2efe87
to
5171efd
Compare
Ignore tag order for object comparisons by sorting the string slices that contain them before checking object equality. This avoids issues with spurious diffs resulting in unnecessary changes when the tag order constructed by deck from dump YAML and the tag order reported by admin API JSON differ. Because Kong object tags are logical string sets, their order does not matter: it is only a perceived difference because JSON and YAML lack set types and represent them with ordered arrays. Fix #239 From #240
Ignore tag order for object comparisons by sorting the string slices that contain them before checking object equality. This avoids issues with spurious diffs resulting in unnecessary changes when the tag order constructed by deck from dump YAML and the tag order reported by admin API JSON differ. Because Kong object tags are logical string sets, their order does not matter: it is only a perceived difference because JSON and YAML lack set types and represent them with ordered arrays. Fix #239 From #240
Ignore tag order for object comparisons by sorting the string slices that contain them before checking object equality. This avoids issues with spurious diffs resulting in unnecessary changes when the tag order constructed by deck from dump YAML and the tag order reported by admin API JSON differ.
Because Kong object tags are logical string sets, their order does not matter: it is only a perceived difference because JSON and YAML lack set types and represent them with ordered arrays.
Fix #239 and a minor typo ("Copt" to "Copy" in one of the functions).
Implementation notes/questions:
We have
Equal()andEqualWithOpts()functions for each object. The former is justreturn reflect.DeepEqual(thing1, thing2), is not modified by this PR, and would still report the objects not equal as such. The latter copies the objects first and then nulls out their IDs and/or created and updated timestamps based on the options provided before performing a comparison. This PR does modifyEqualWithOpts()to ignore tag order, though it doesn't introduce a new option: IDs and timestamp differences do actually matter for objects, but tag order never does, so it doesn't seem like it makes sense to add an option.I'm curious about the history behind this, since as far as I know deck never cares about timestamps (there's no CLI options to toggle that option, and dumps don't include timestamp data, but this doesn't mean that deck repeatedly updates objects because the
niltimestamps in dumps do not match the not-niltimestamps) and only sort of cares about IDs (dumphas an option to include them or omit them, butsyncdoesn't have an option to ignore them).Equal()in practice, and if so, how/why? Do we need to modify it to also create copies and sort their tags?EqualWithOpts(): do we ever use that without ignoring IDs and timestamps?Unrelated to that, types_test.go does not currently include tags on any of its test objects. We can cover that pretty easily by adding differently-ordered Tags to objects, e.g. after https://github.com/Kong/deck/blob/v1.2.3/state/types_test.go#L51 and confirming that
EqualWithOpts()is still true, but I wanted to get a pass on the proposed implementation here before writing those out for every object (Golang generics, you cannot arrive soon enough).