Allow aeson-2.2 #1695

maksbotan · 2023-07-11T11:31:05Z

No description provided.

maksbotan · 2023-07-11T12:17:04Z

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

servant/servant/src/Servant/API/ContentTypes.hs

Lines 374 to 391 in 16cfd03

    
           -- | Like 'Data.Aeson.eitherDecode' but allows all JSON values instead of just 
        
           -- objects and arrays. 
        
           -- 
        
           -- Will handle trailing whitespace, but not trailing junk. ie. 
        
           -- 
        
           -- >>> eitherDecodeLenient "1 " :: Either String Int 
        
           -- Right 1 
        
           -- 
        
           -- >>> eitherDecodeLenient "1 junk" :: Either String Int 
        
           -- Left "trailing junk after valid JSON: endOfInput" 
        
           eitherDecodeLenient :: FromJSON a => ByteString -> Either String a 
        
           eitherDecodeLenient input = 
        
               parseOnly parser (cs input) >>= parseEither parseJSON 
        
             where 
        
               parser = skipSpace 
        
                     *> Data.Aeson.Parser.value 
        
                     <* skipSpace 
        
                     <* (endOfInput <?> "trailing junk after valid JSON")

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 · 2023-07-11T14:40:00Z

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 · 2023-07-11T17:15:41Z

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

servant/servant/test/Servant/API/ContentTypesSpec.hs

Lines 222 to 229 in 104a8ad

    
           -- aeson >= 0.9 decodes top-level strings 
        
           describe "eitherDecodeLenient" $ do 
        
               it "parses top-level strings" $ do 
        
                   let toMaybe = either (const Nothing) Just 
        
                   -- The Left messages differ, so convert to Maybe 
        
                   property $ \x -> toMaybe (eitherDecodeLenient x) 
        
                       `shouldBe` (decode x :: Maybe String)

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.

servant/test/Servant/API/ContentTypesSpec.hs

maksbotan · 2023-07-12T08:25:41Z

I've returned the test

maksbotan · 2023-07-12T08:25:54Z

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

tchoutri · 2023-07-12T08:34:03Z

That would be lovely yes

maksbotan · 2023-07-12T08:37:33Z

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 · 2023-07-12T10:08:39Z

@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 · 2023-07-12T10:09:08Z

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 · 2023-07-12T12:26:41Z

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

tchoutri · 2023-07-12T14:12:12Z

ok perfect it compiles here

maksbotan · 2023-07-12T14:12:59Z

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

tchoutri · 2023-07-12T14:15:59Z

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

ysangkok · 2023-07-26T14:30:29Z

@maksbotan Has this been sufficiently tested already?

Vekhir · 2023-09-11T23:52:30Z

@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 · 2023-09-12T15:52:23Z

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.)

larskuhtz

I propose to keep eitherDecodeLenient as an deprecated alias of eitherDecode, since both have (probably) the same behavior since version 0.9 of aeson. (I haven't checked possible differences is the handling of of trailing characters, but I expect that to be a negligible issue.)

This would reduce the potential of the PR to break users and will hopefully help this change to be release soon.

larskuhtz · 2023-09-15T17:18:48Z

.github/workflows/master.yml

+    branches:
+      - master


This seems unrelated to this PR.

larskuhtz · 2023-09-15T17:19:02Z

.github/workflows/master.yml

@@ -20,6 +22,7 @@ jobs:
          - "9.2.8"
          - "9.4.5"
          - "9.6.2"
+      fail-fast: false


This change seems unrelated to this PR.

larskuhtz · 2023-09-15T17:27:16Z

servant/src/Servant/API/ContentTypes.hs

@@ -65,7 +65,6 @@ module Servant.API.ContentTypes
    , AllMime(..)
    , AllMimeRender(..)
    , AllMimeUnrender(..)
-    , eitherDecodeLenient


From the comments below it seems that with currently supported aeson versions eitherDecodeLenient and eitherDecode have the same behavior.

What about making eitherDecodeLenient an alias of eitherDecode and just mark it as deprecated? That way it would not break the API.

larskuhtz · 2023-09-15T17:38:00Z

servant/src/Servant/API/ContentTypes.hs

@@ -371,28 +364,9 @@ instance NFData NoContent
 --------------------------------------------------------------------------
 -- * MimeUnrender Instances



Suggested change

-- | Deprecated: since aeson version 0.9 `eitherDecode` has lenient behavior.

--

eitherDecodeLenient :: FromJSON a => ByteString -> Either String a

eitherDecodeLenient = eitherDecode

{-# DEPRECATED eitherDecodeLenient "use eitherDecode instead" #-}

larskuhtz · 2023-09-15T17:39:29Z

servant/src/Servant/API/ContentTypes.hs

@@ -65,7 +65,6 @@ module Servant.API.ContentTypes
    , AllMime(..)
    , AllMimeRender(..)
    , AllMimeUnrender(..)
-    , eitherDecodeLenient
    , canHandleAcceptH


Keep eitherDecodeLenient as alias of eitherDecde, but mark it as deprecated (cf. below).

Suggested change

, canHandleAcceptH

, eitherDecodeLenient

, canHandleAcceptH

larskuhtz · 2023-09-15T17:41:49Z

servant/test/Servant/API/ContentTypesSpec.hs

@@ -219,15 +219,13 @@ spec = describe "Servant.API.ContentTypes" $ do
                handleCTypeH (Proxy :: Proxy '[JSONorText]) "image/jpeg"
                    "foobar" `shouldBe` (Nothing :: Maybe (Either String Int))

-    -- aeson >= 0.9 decodes top-level strings
    describe "eitherDecodeLenient" $ do


Suggested change

describe "eitherDecodeLenient" $ do

describe "eitherDecode is lenient" $ do

TeofilC · 2023-09-29T10:55:45Z

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

https://docs.github.com/en/actions/using-jobs/using-a-matrix-for-your-jobs#handling-failures

Vekhir · 2023-09-29T17:06:25Z

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 · 2023-09-29T17:07:33Z

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

ysangkok · 2023-09-29T19:35:03Z

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

Relevant issue seems to be fixed upstream.

maksbotan · 2023-10-01T13:43:19Z

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 · 2023-10-01T14:46:13Z

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 · 2023-10-01T15:05:09Z

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

tchoutri · 2023-10-01T15:06:50Z

It still does not support aeson-2.2 saurabhnanda/odd-jobs#101

maksbotan · 2023-10-01T15:13:19Z

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

Great, I'll publish that release on Hackage.

maksbotan · 2023-10-02T07:39:28Z

Published!

maksbotan force-pushed the maksbotan/aeson-2.2 branch from 29658e5 to a408400 Compare July 11, 2023 11:31

maksbotan force-pushed the maksbotan/aeson-2.2 branch from 8b302f4 to 160bf12 Compare July 11, 2023 22:08

ysangkok reviewed Jul 11, 2023

View reviewed changes

servant/test/Servant/API/ContentTypesSpec.hs Show resolved Hide resolved

maksbotan force-pushed the maksbotan/aeson-2.2 branch from 160bf12 to c194e5c Compare July 12, 2023 07:48

larskuhtz mentioned this pull request Sep 14, 2023

Secp256k1 kadena-io/kadena-ethereum-bridge#14

Merged

larskuhtz reviewed Sep 15, 2023

View reviewed changes

maksbotan added 4 commits September 29, 2023 17:07

Allow aeson-2.2

ce45c8e

fail-fast: false for github actions

96aa9d9

https://docs.github.com/en/actions/using-jobs/using-a-matrix-for-your-jobs#handling-failures

Get rid of eitherDecodeLenient

97dba02

Run CI on push only to master

d074bba

maksbotan force-pushed the maksbotan/aeson-2.2 branch from c194e5c to d074bba Compare September 29, 2023 14:22

maksbotan added 4 commits October 1, 2023 16:07

Return eitherDecodeLenient as deprecated synonym

b9da2f3

servant 0.20.1

3ce5741

Drop constraint on warp in cabal.project

57e0f29

Relevant issue seems to be fixed upstream.

Constraint http2 <4.2

8a4a7f5

maksbotan force-pushed the maksbotan/aeson-2.2 branch from 92a6426 to 8a4a7f5 Compare October 1, 2023 13:07

maksbotan merged commit 459ecef into master Oct 2, 2023
8 checks passed

ysangkok deleted the maksbotan/aeson-2.2 branch October 10, 2023 22:10

ysangkok mentioned this pull request Oct 10, 2023

Add attoparsec-aeson dependency #1687

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow aeson-2.2 #1695

Allow aeson-2.2 #1695

maksbotan commented Jul 11, 2023

maksbotan commented Jul 11, 2023

tchoutri commented Jul 11, 2023

maksbotan commented Jul 11, 2023

maksbotan commented Jul 12, 2023

maksbotan commented Jul 12, 2023

tchoutri commented Jul 12, 2023

maksbotan commented Jul 12, 2023

tchoutri commented Jul 12, 2023

tchoutri commented Jul 12, 2023

maksbotan commented Jul 12, 2023

tchoutri commented Jul 12, 2023

maksbotan commented Jul 12, 2023

tchoutri commented Jul 12, 2023

ysangkok commented Jul 26, 2023

Vekhir commented Sep 11, 2023

larskuhtz commented Sep 12, 2023

larskuhtz left a comment •

edited

Loading

larskuhtz Sep 15, 2023

larskuhtz Sep 15, 2023

larskuhtz Sep 15, 2023

larskuhtz Sep 15, 2023

larskuhtz Sep 15, 2023

larskuhtz Sep 15, 2023

TeofilC commented Sep 29, 2023 •

edited

Loading

Vekhir commented Sep 29, 2023

maksbotan commented Sep 29, 2023

ysangkok commented Sep 29, 2023 •

edited

Loading

maksbotan commented Oct 1, 2023

tchoutri commented Oct 1, 2023

maksbotan commented Oct 1, 2023

tchoutri commented Oct 1, 2023

maksbotan commented Oct 1, 2023

maksbotan commented Oct 2, 2023

		@@ -371,28 +364,9 @@ instance NFData NoContent
		--------------------------------------------------------------------------
		-- * MimeUnrender Instances

+-- | Deprecated: since aeson version 0.9 `eitherDecode` has lenient behavior.
+--
+ eitherDecodeLenient :: FromJSON a => ByteString -> Either String a
+ eitherDecodeLenient = eitherDecode
+ {-# DEPRECATED eitherDecodeLenient "use eitherDecode instead" #-}

	describe "eitherDecodeLenient" $ do
	describe "eitherDecode is lenient" $ do

Allow aeson-2.2 #1695

Allow aeson-2.2 #1695

Conversation

maksbotan commented Jul 11, 2023

maksbotan commented Jul 11, 2023

tchoutri commented Jul 11, 2023

maksbotan commented Jul 11, 2023

maksbotan commented Jul 12, 2023

maksbotan commented Jul 12, 2023

tchoutri commented Jul 12, 2023

maksbotan commented Jul 12, 2023

tchoutri commented Jul 12, 2023

tchoutri commented Jul 12, 2023

maksbotan commented Jul 12, 2023

tchoutri commented Jul 12, 2023

maksbotan commented Jul 12, 2023

tchoutri commented Jul 12, 2023

ysangkok commented Jul 26, 2023

Vekhir commented Sep 11, 2023

larskuhtz commented Sep 12, 2023

larskuhtz left a comment • edited Loading

Choose a reason for hiding this comment

larskuhtz Sep 15, 2023

Choose a reason for hiding this comment

larskuhtz Sep 15, 2023

Choose a reason for hiding this comment

larskuhtz Sep 15, 2023

Choose a reason for hiding this comment

larskuhtz Sep 15, 2023

Choose a reason for hiding this comment

larskuhtz Sep 15, 2023

Choose a reason for hiding this comment

larskuhtz Sep 15, 2023

Choose a reason for hiding this comment

TeofilC commented Sep 29, 2023 • edited Loading

Vekhir commented Sep 29, 2023

maksbotan commented Sep 29, 2023

ysangkok commented Sep 29, 2023 • edited Loading

maksbotan commented Oct 1, 2023

tchoutri commented Oct 1, 2023

maksbotan commented Oct 1, 2023

tchoutri commented Oct 1, 2023

maksbotan commented Oct 1, 2023

maksbotan commented Oct 2, 2023

larskuhtz left a comment •

edited

Loading

TeofilC commented Sep 29, 2023 •

edited

Loading

ysangkok commented Sep 29, 2023 •

edited

Loading