Upgrade aeson to 2.0.* #2292
Upgrade aeson to 2.0.* #2292
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
That was because I'd copied over the aeson source hash. So, since it has the aeson sources in the store as a fixed output thingy, it tries building the aeson sources as OneTuple which... can't work. |
|
Whoa, this looks like hard work. Unfortunately I'm also lost and don't know a simple way to solve the many dependency errors. Another option(not sure how feasible), would be to try and replace aeson? I've been eyeing jsonifier for a while. |
Ah don't worry, I think it's a simple change that's just made complicated by a combination of nixpkg's rigidity and my lack of understanding. I think I can probably sort it out by also updating the nixpkgs pin, was just hoping to do only one thing at a time. |
|
One reliable test failure is the following: Any idea what happened there? Is the test fragile, or did something break with the update? (I could see that relying on the key order of JSON maps might have changed with the new aeson version, but that doesn't obviously apply here...) |
Yeah, that test can be changed, no problem about it. I've been doing the same on recent commits. In fact that root spec feature is not documented and will be dropped once we have #2299. |
Whereas the test used to specify a body of
[{"qiName":"referrals","qiSchema":"test"},"private"]
we see both
[{"qiName":"authors_w_entities","qiSchema":"test"},"test"]
and
[{"qiName":"organizations","qiSchema":"test"},"test"]
on the GHC 9.2 upgrade PR.
This changes the test to no longer expect a particular body.
Compare:
- PostgREST#2292 (comment)
- PostgREST#1698
|
@steve-chavez I think this is at a point now where it's "stable" from the point of view of the underlying nixpkgs change -- would it make sense to update cachix ( |
Whereas the test used to specify a body of
[{"qiName":"referrals","qiSchema":"test"},"private"]
we see both
[{"qiName":"authors_w_entities","qiSchema":"test"},"test"]
and
[{"qiName":"organizations","qiSchema":"test"},"test"]
on the GHC 9.2 upgrade PR.
This changes the test to no longer expect a particular body.
Compare:
- #2292 (comment)
- #1698
@robx Do you want me to do this manually for this PR? Because |
I was thinking about manually doing this, yeah, but not sure it's a good idea... Right now every single CI job takes 20 minutes to set up the nix environment. (I don't mind the time, just seems a bit expensive while iterating.) Besides requiring manual work, a good argument against is that the nixpkgs version might need to change again after all. There's two outstanding issues here, the load test encoding failure and the static build. I expect they should be fixable without changing the nixpkgs version, but it's not really clear at this point. |
We can ignore that one here. It will be gone when merging to main. As seen on #2302 (comment) |
robx
force-pushed
the
aeson2
branch
6 times, most recently
from
91492ff
to
3fb1245
Compare
| pkgsPrev = import nixpkgsPrev { }; | ||
| inherit (pkgsPrev) hlint; | ||
| in | ||
| pkgs.callPackage nix/tools/style.nix { inherit hlint hsie; }; |
There was a problem hiding this comment.
I'd file an issue and/or a follow-up PR to make postgrest hlint-clean for the updated hlint version, and to drop this shim again. But would rather not add linter fixes into the mix here, the PR ballooned enough already.
|
Assuming the tests pass 🤞, this should be good for review now. The one thing I can't evaluate is if the switch to |
With both HashMap and Map imported as M in different modules, linter rules prevented ever importing both modules in one place.
This means that we're now using Data.Map.Strict instead of Data.HashMap.Strict for JSON objects in general, and specifically for claims maps and CSV rows. This addresses certain hash flooding vulnerabilities, but may have performance downsides. Compare e.g. https://frasertweedale.github.io/blog-fp/posts/2021-10-12-aeson-hash-flooding-protection.html
jose version before 0.8.5.1 lacked an upper bound on aeson-2, causing build failures with aeson-2 present.
wai-logger version 2.4.0 fixes log output to not say 'unknownSocket' for unix sockets.
This patches static-haskell-nix to work for building postgrest
with updated nixpkgs (from ~202203):
- The ncurses 'enableStatic' argument doesn't exist anymore.
We use the vanilla package instead, which seems to work fine.
- The 'isExecutable' check fails with a strange error related
to trying to override 'mkDerivation'.
We patch 'isExecutable' to check explicitly whether we're building
postgrest. ('isExecutable' is used to determine whether to build
a package statically.)
This keeps the pre-nixpkgs update hlint version, so we can defer addressing the warnings to a follow-up PR.
Ah, that would be the |
|
Here are load test results post-merge on main: https://github.com/PostgREST/postgrest/runs/6874664942 |
|
Randomly pulling the
The Other than that, it looks like the upgrade (whether aeson2 or some of the other dependency changes) rather improved results, looks to me like 15% throughput increase, matching latency decrease. |
|
@robx Loadtest results look great! I've been trying to run other loadtests but.. Unfortunately, I no longer seem to be able to produce a static binary: 9.0.1 shows: We need a test on CI to ensure the above output. On another NixOS server, trying to run produces an error( |
Aeson 2.0 abstracts the object type (from
Data.HashMap.StricttoData.Aeson.KeyMap). From version 2.0.1.0, the default map implementation is switched toData.Map.Strict, addressing the hash flooding vulnerability. This is also one step towards compatibility with GHC 9.2 (#2288).Compare https://frasertweedale.github.io/blog-fp/posts/2021-10-12-aeson-hash-flooding-protection.html for some background.
Besides fixing the vulnerability, the change to
Data.Map.StrictfromData.HashMap.Strictis likely to have a performance impact for some work loads.Overview:
These don't appear to have important changes.
KeyMaptype, and change auth claims and CSV row maps accordinglyThe load test fails as per #2308 due to the nix dependency update; this will go away after merging.