Skip to content

hashable 1.3.1.0 changes aesons property ordering #837

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.

Sign up for GitHub

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

Jump to bottom
Closed
bergmark opened this issue Feb 18, 2021 · 5 comments
Closed

hashable 1.3.1.0 changes aesons property ordering #837

bergmark opened this issue Feb 18, 2021 · 5 comments
Assignees
@phadej

Comments

@bergmark
Copy link
Collaborator

At least I think this is the case, as a result of haskell-unordered-containers/hashable#196 so sorry if this turns out to be incorrect. Thought i'd file this now in case we want to do something about it quickly before a lot of downstream libs have been updated.

aeson doesn't guarantee ordering of fields, which makes sense. But it is unfortunate if this change to hashable breaks test suites, esp. if it's because of a dependency version changing. Many people are matching aeson output against strings. It could affect "actual" code as well I suppose.

I'm not sure what the best way forward is

  1. restrict aeson to an older version of hashable until we make a major release
  2. make the latest hashable release unbuildable and publish a major version for it instead.
  3. do nothing.

We should probably recommend use of Data.Aeson.QQ(.Simple) in test suites either way. It would have prevented the noise.

@juhp do you have examples of packages with test failures?
@bergmark bergmark changed the title hashable 1.3.1.0.0 changes aesons property ordering(?) hashable 1.3.1.0 changes aesons property ordering(?) Feb 18, 2021
@phadej
Copy link
Collaborator

phadej commented Feb 18, 2021 •
edited
Loading

  1. do nothing.

I have fixed testsuites in the past, e.f aesons and swagger2 because on different architectures (32/64) the hashable is different. There is aeson-pretty for example to work-around that.

Comparing printed unordered-containers:HashMap for equality is risky, and I won't consider making hashable major bumps when the hash implementation is tweaked in minor ways.

@bergmark
Copy link
Collaborator Author

Ok thanks for the quick reply! I'm fine with that.

I wasn't aware that aeson-pretty had confCompare, nice :-)

@phadej
Copy link
Collaborator

phadej commented Feb 18, 2021

FWIW, for the next aeson we can (and probably should to avoid making people sad) make Show instance for Value stable.
It can be slow and sort keys. I'll do that, soon-ish.

@phadej phadej reopened this Feb 18, 2021
@phadej phadej self-assigned this Feb 18, 2021
@bergmark
Copy link
Collaborator Author

bergmark commented Feb 18, 2021 •
edited
Loading

good idea!

To summarize for anyone else seeing this, our recommendations to prevent similar issues in the future is to write tests using:

  • The stable Show instance in aeson >= 1.5.6.0
  • aeson-pretty (has confCompare)
  • Values directly (decode @Value)
  • Data.Aeson.QQ.Simple (in aeson) or Data.Aeson.QQ (in aeson-qq)

I'm personally a big fan of aeson-qq :-) the interpolation comes in handy

phadej added a commit that referenced this issue Feb 18, 2021
@phadej
Fix #837: Stable (in key order) Show Value instance
023edfb 
- Add test that read . show roundtrips
- Add test that Value To/FromJSON instances roundtrip (as we got
  Arbitrary Value instance in tests)
- Cleanup some doctest looking comments
phadej added a commit that referenced this issue Feb 18, 2021
@phadej
Fix #837: Stable (in key order) Show Value instance
1df02b6 
- Add test that read . show roundtrips
- Add test that Value To/FromJSON instances roundtrip (as we got
  Arbitrary Value instance in tests)
- Cleanup some doctest looking comments
@phadej phadej closed this as completed in d9107d9 Feb 19, 2021
phadej added a commit that referenced this issue Feb 19, 2021
@phadej
Merge pull request #838 from haskell/issue-837-stable-show
c03c868 
Fix #837: Stable (in key order) Show Value instance
@phadej
Copy link
Collaborator

phadej commented Feb 19, 2021

Fixed in https://hackage.haskell.org/package/aeson-1.5.6.0

@bergmark bergmark changed the title hashable 1.3.1.0 changes aesons property ordering(?) hashable 1.3.1.0 changes aesons property ordering Feb 19, 2021
@juhp juhp mentioned this issue Feb 20, 2021
Closed
@Xitian9 Xitian9 mentioned this issue Jul 20, 2021
Merged
hasura-bot pushed a commit to hasura/graphql-engine that referenced this issue Feb 18, 2022
@hasura-bot
Updates cabal freeze file
d50aae8 
#### TODO

- [x] fix `hashable >= 1.3.1` serialization ordering issue [^1]
  - `test_graphql_mutations.py::TestGraphQLMutateEnums` was failing
- [x] fix `unordered-containers` serialization ordering issue [^2]
  - `test_graphql_queries.py` was failing on Citus
- [ ] verify that no new failures have been introduced
- [ ] open issues to fix the above
  - identify test cases that "leak" implementation details by depending on `hashable` instance ordering
  - bump `hashable >= 1.3.1` and update test cases with new ordering OR modify them so that ordering is stable
  - bump `unordered-containers >= 0.2.15.0` and update test cases with new ordering OR modify them so that ordering is stable
    - one of the test cases was failing on string equality comparison for a generated Citus query
    - we probably don't want to _actually_ do this unless there are _very specific_ guarantees we want to make about generated query structure
---

Just what it says on the tin.

hasura/graphql-engine-mono#3538 updated the freeze file a few weeks ago, but it looks like the index state hadn't been updated since December so a lot of stuff that had newer versions didn't get updated.

---

EDIT: I should add, the motivation for doing this in the first place is that `hspec > 2.8.4` now supports specifying filtering spec trees based on patterns provided by the `HSPEC_MATCH` environment variable.

For example, one could have a script that executes the following:
```
HSPEC_MATCH="PostgreSQL" \
  ghcid \
    --command \
      'cabal repl graphql-engine:test:tests-hspec \
         --repl-option -O0 \
         --repl-option -fobject-code' \
    --test "main"
```
...which will loop on typechecking the `tests-hspec` component, and then as soon as it passes (i.e. no warnings or errors) will run _only_ the `PostgreSQL` sub-components.

[^1]: `hashable >= 1.3.1.0` [updated its default salts](haskell-unordered-containers/hashable#196), which [broke serialization ordering](haskell/aeson#837)
[^2]: `unordered-containers >= 0.2.16.0` [introduced changes to some of its internal functions](https://hackage.haskell.org/package/unordered-containers-0.2.16.0/changelog) which seem like they could have affected serialization stability

PR-URL: hasura/graphql-engine-mono#3672
GitOrigin-RevId: bbd1d48c73db4021913f0b5345b7315a8d6525d3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@phadej phadej

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@phadej @bergmark