haskell-servant / servant

Main repository for the servant libraries — DSL for describing, serving, querying, mocking, documenting web applications and more!
https://docs.servant.dev/
1.8k stars 407 forks source link

Allow aeson-2.2 #1695

Closed maksbotan closed 10 months ago

maksbotan commented 1 year ago

@theophile-fl @ysangkok @jkarni and co, I need your advice on this. Servant.API.ContentTypes uses Data.Aeson.Parser:

https://github.com/haskell-servant/servant/blob/16cfd03e74557d8fd8ef6e2ebfb9c26bd645d756/servant/src/Servant/API/ContentTypes.hs#L374-L391

Data.Aeson.Parser module was removed in aeson-2.2, so there is no easy way to upgrade our version constraint.

However, I think that this function is not needed anymore. Haddock mentions that it parser any json value, contrary to aeson's behavior to parse only list and object. See haddock for 'json' in aeson-2.1: https://hackage.haskell.org/package/aeson-2.1.2.1/docs/Data-Aeson-Parser.html#v:json

This function is an alias for value. In aeson 0.8 and earlier, it parsed only object or array types, in conformance with the now-obsolete RFC 4627.

Our lower bound on aeson is 1.4 for a long time, so I propose to remove our eitherDecodeLenient and use aeson's native eitherDecode instead. What do you think? I'm not sure if we have some tests to verify that the change is OK.

I will make the change in this PR a bit later.

tchoutri commented 1 year ago

I support your proposition, in any case we will want to make sure that a pre-release is uploaded to Hackage so that users (me included) can test the behaviour. :)

maksbotan commented 1 year ago

There was a test that servant's eitherDecodeLenient behaves exactly like eitherDecode from aeson:

https://github.com/haskell-servant/servant/blob/104a8ad2f387b68d6844101e01b585d106f4d0d3/servant/test/Servant/API/ContentTypesSpec.hs#L222-L229

I've pushed an update to check if everything works. I've also had to change the tutorial, as it also mentioned old aeson behavior.

maksbotan commented 1 year ago

I've returned the test

maksbotan commented 1 year ago

Do you want me to upload candidate to Hackage to test?

tchoutri commented 1 year ago

That would be lovely yes

maksbotan commented 1 year ago

It's here: https://hackage.haskell.org/package/servant-0.20.1/candidate

You can try it by adding https://hackage.haskell.org/package/servant-0.20.1/candidate/servant-0.20.1.tar.gz to your cabal.project or stack.yaml.

tchoutri commented 1 year ago

@maksbotan Wonderful. I think servant-server & servant-client-core are going to need a pre-release as well, I'm getting:


src/Servant/Server/Internal.hs:686:26: error:
    • Couldn't match type ‘a’ with ‘IO a0’
      Expected: SourceIO chunk -> a
        Actual: SourceIO chunk -> IO a0
      ‘a’ is a rigid type variable bound by
        the instance declaration
        at src/Servant/Server/Internal.hs:(673,5)-(675,68)
    • In the first argument of ‘return’, namely ‘fromSourceIO’
      In the expression: return fromSourceIO
      In an equation for ‘ctCheck’: ctCheck = return fromSourceIO

and

src/Servant/Client/Core/HasClient.hs:430:35: error:
    • Couldn't match type ‘a’ with ‘IO a1’
      Expected: Client m (Stream method status framing ct a)
        Actual: m (IO a1)
      ‘a’ is a rigid type variable bound by
        the instance declaration
        at src/Servant/Client/Core/HasClient.hs:(422,3)-(424,54)
    • In the expression:
        withStreamingRequest req'
          $ \ gres
              -> do let ...
                    return $ fromSourceIO $ framingUnrender' $ responseBody gres
      In an equation for ‘clientWithRoute’:
          clientWithRoute _pm Proxy req
            = withStreamingRequest req'
                $ \ gres
                    -> do let ...
                          ....
            where
                req'
                  = req
                      {requestAccept = fromList [...],
                       requestMethod = reflectMethod (Proxy :: Proxy method)}
      In the instance declaration for
        ‘HasClient m (Stream method status framing ct a)’
    • Relevant bindings include
        clientWithRoute :: Proxy m
                           -> Proxy (Stream method status framing ct a)
                           -> Request
                           -> Client m (Stream method status framing ct a)
          (bound at src/Servant/Client/Core/HasClient.hs:430:3)
    |
430 |   clientWithRoute _pm Proxy req = withStreamingRequest req' $ \gres -> do
    |                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^...
tchoutri commented 1 year ago

I will also send a call for beta-testers on social media to make sure our users can take the time to test things on their own applications

maksbotan commented 1 year ago

That should not be happening, looks like you get servant-server <0.20, which is incompatible with servant-0.20.*

tchoutri commented 1 year ago

ok perfect it compiles here

maksbotan commented 1 year ago

Okay, so let's wait for a bit for more tests, and I'll merge & release.

tchoutri commented 1 year ago

@maksbotan servant-lucid would need a bound bump, I can do it for you if you give me permission on Hackage. :)

ysangkok commented 1 year ago

@maksbotan Has this been sufficiently tested already?

Vekhir commented 10 months ago

@maksbotan Is there anything blocking this from getting merged?

Publishing this patch in a new version would allow the Arch package to be updated without carrying an additional patch.

larskuhtz commented 10 months ago

I would also love to see this merged and released.

(I think, as a web framework, servant should try to always support the latest version of aeson.)

TeofilC commented 10 months ago

Let me know if you'd appreciate any help getting this over the line, but no pressure

Vekhir commented 10 months ago

The CI fails because it tries to build warp-3.3.25 which is incompatible with http2-4.2.0. There is a newer version, warp-3.3.29 which is compatible, but not chosen.

maksbotan commented 10 months ago

Thank you @Vekhir. I'll look into this.

ysangkok commented 10 months ago

@maksbotan : You might want to remove or flip constraints: warp < 3.3.26 in cabal.project.

maksbotan commented 10 months ago

Due to backwards-incompatible changes to master since this PR (see #1697), I will have to make Hackage release from this branch.

@ysangkok @tchoutri @larskuhtz please check out this release candidate: https://hackage.haskell.org/package/servant-0.20.1/candidate

Direct tar gz link: https://hackage.haskell.org/package/servant-0.20.1/candidate/servant-0.20.1.tar.gz

If everything's fine, I'll publish a release.

tchoutri commented 10 months ago

Alright, except for a usage of eitherDecodeLenient and odd-jobs lagging a bit behind for aeson-2.2 compat, I haven't encountered any problem.

maksbotan commented 10 months ago

What do you mean by "odd-jobs lagging"?

tchoutri commented 10 months ago

It still does not support aeson-2.2 https://github.com/saurabhnanda/odd-jobs/pull/101

maksbotan commented 10 months ago

Ah, didn't get that it's a name of a package :)

Great, I'll publish that release on Hackage.

maksbotan commented 10 months ago

Published!