Could you maybe add a short comment here about what invariants we maintain about order and why? It isn’t immediately clear to me just from reading the code what matters and what doesn’t.

My understanding is that we’re doing this so that metadata export is deterministic, so if you export the same thing twice, you get the same results. That makes complete sense, but it raises questions like: do we guarantee any consistency about ordering across versions of graphql-engine? If so, which invariants? And if not, it would be helpful to explicitly say so just so that people reading/modifying the code don’t worry about it.

The deterministic export of metadata is already achieved via #3230. This PR intends to remove fields with []/null/defaults in exported metadata. And, the order of keys in an object is "hardcoded" using Data.Aeson.Ordered module since there's no way to determine the order of keys in Data.Aeson.

The CLI based developer experience needs the exported metadata should be precise and clean. The developer should be able to edit the exported YAML file and apply them. In an offline discussion between me, @coco98 and @arvi3411301 we decided the order of the keys in ReplaceMetadata, TableMeta etc. and the same followed here.

do we guarantee any consistency about ordering across versions of graphql-engine?

We do guarantee the order of keys in the exported metadata. I'll add the necessary description as comments.

I think there are 2 kinds of "order" we could consider.

1. The order of items in an array. (solved in deterministic ordering of objects in exported metadata (close #3125) #3230)
2. The order of keys in an object. (In this PR)

Okay, thanks, that helps! I’ve tweaked the comment slightly to explicitly mention the fact that the ordering should be maintained across versions; feel free to change it if you think it should be worded differently.

Your note that the encoding should be roundtrippable got me wondering, though: do you think it would make sense to add some quickcheck tests to test that property?

Adding a quick check to test the property makes sense. I'll add them in next commit.

Edit 1:- To add a quick check, we need to defined generator Gen a for all ReplaceMetadata dependent types. A lot of code to write. Is there a way to generate Gen a with a simple template haskell or type class deriving?. I'm looking at the Hedgehog property testing package.

To add a quick check, we need to defined generator Gen a for all ReplaceMetadata dependent types. A lot of code to write. Is there a way to generate Gen a with a simple template haskell or type class deriving?. I'm looking at the Hedgehog property testing package.

Good question. Neither QuickCheck nor hedgehog provide anything like that out of the box, since they (somewhat reasonably) point out that the choice of generator for each type is important for getting value out of the tests. That said, I think in many cases, you really do just want to use a generator that distributes over each field of a record type, for example.

The QuickCheck documentation links to two packages that can do that sort of thing: testing-feat and generic-random; I also found the generic-arbitrary package. In the hedgehog realm, there’s hedgehog-generic, but I’m skeptical of that package’s utility: hedgehog doesn’t have an Aribitrary class, so a generic generator is “deeply” generic. I can’t imagine that being what you want very often.

I don’t actually have a ton of experience using property-based testing for anything non-trivial, so I’m not sure what the right thing to do is here. Maybe @paf31 has some thoughts?

I don't have a lot of useful input here, but I think it would be nice to avoid adding many Arbitrary instances in the src-lib area of the code, especially when they'll probably end up being specific to this sort test. Instead, we can use genericArbitrary or some other method of getting values of type Gen a, entirely within the test suite, and then use the Testable a => Testable (Gen a) instance to pass the composed generator itself to quickCheck.

I'm not sure to what extent that is common knowledge or not, but it seemed worth mentioning.