Skip to content
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.

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

support Aeson 2 #1475

Merged
merged 9 commits into from
Nov 26, 2021
Merged

support Aeson 2 #1475

merged 9 commits into from
Nov 26, 2021

Conversation

akhesaCaro
Copy link
Contributor

@akhesaCaro akhesaCaro commented Oct 29, 2021

Fixes : #1467 (for aeson)
Following : #1404 (for servant-swagger moving)

Tasks

Bump Aeson

  • min and max bound aeson2 and resolve dependency problems. Needed bounds with aeson2 :
  • aeson >= 2.0.1.0 && < 3
  • jose >= 0.9 && < 0.10
  • containers >= 0.6 && < 0.7
  • swagger2 >= 2.2.2 && < 3
  • base64-bytestring >= 1.0.0.1 && < 2
  • Fix server-auth with the new Aeson (use Data.Map instead of Data.HashMap.Strict)

Move servant-swagger into this repo

  • Move servant-swagger into the repo because it needed to bump aeson2 (and it easier than making a release). Needed bounds with aeson2
  • Add servant-swagger to the main servant cabal and resolve
  • aeson >= 2.0.1.0 && < 3
  • Cabal >= 1.24 && <3.3,
  • Remove obsolete files dans mentions
  • Change URLs in README and cabal file for hackage

@akhesaCaro
Copy link
Contributor Author

dependecy with swagger2

should we use openAPI3 instead? #1470

[__2] trying: servant-auth-swagger-0.2.10.1 (user goal)
[__3] next goal: swagger2 (dependency of servant-auth-swagger)
[__3] rejecting: swagger2-2.6 (conflict: aeson==2.0.0.0, swagger2 =>
aeson>=1.4.2.0 && <1.6)
[__3] skipping: swagger2-2.5, swagger2-2.4, swagger2-2.3.1.1, swagger2-2.3.1,
swagger2-2.3.0.1, swagger2-2.3, swagger2-2.2.2, swagger2-2.2.1, swagger2-2.2
(has the same characteristics that caused the previous version to fail:
excludes 'aeson' version 2.0.0.0)
[__3] rejecting: swagger2-2.1.6 (conflict: servant-auth-swagger =>
swagger2>=2.2.2 && <2.7)
[__3] skipping: swagger2-2.1.5, swagger2-2.1.4.1, swagger2-2.1.4,
swagger2-2.1.3, swagger2-2.1.2.1, swagger2-2.1.2, swagger2-2.1.1,
swagger2-2.1, swagger2-2.0.2, swagger2-2.0.1, swagger2-2.0, swagger2-1.2.1,
swagger2-1.2, swagger2-1.1.1, swagger2-1.1, swagger2-1.0, swagger2-0.4.1,
swagger2-0.4, swagger2-0.3, swagger2-0.2, swagger2-0.1 (has the same
characteristics that caused the previous version to fail: excluded by
constraint '>=2.2.2 && <2.7' from 'servant-auth-swagger')
[__3] fail (backjumping, conflict set: aeson, servant-auth-swagger, swagger2)
After searching the rest of the dependency tree exhaustively, these were the
goals I've had most trouble fulfilling: aeson, swagger2, servant-auth-swagger,
cookbook-basic-auth
Try running with --minimize-conflict-set to improve the error message.

@akhesaCaro akhesaCaro mentioned this pull request Oct 29, 2021
@alpmestan
Copy link
Contributor

Do we really need to force aeson >= 2?

@arianvp
Copy link
Member

arianvp commented Oct 30, 2021

We could perhaps CPP magic a bit to also support older versions.

however given Servant actually parses untrusted input from the network, thus is one of the few actual libraries where the aeson vulnerability is actually a vulnerability. I think forcing users to switch to Aeson 2.0 is not a bad idea.

However this might require us also doing a major version bump.

@akhesaCaro
Copy link
Contributor Author

Maybe it is a bit early to require aeson 2 or more, but we should do it at some point.
For instance, I am stuck because of swagger which max bump aeson "< 1.6". We probably don't want to put our user in a dead end because of a min bound.

@tchoutri
Copy link
Contributor

Indeed, these kinds of migrations can take some time to ripple throughout the ecosystem. Let's require 2.1 in the future.

@JakubBarta
Copy link

Hi! What's the status here? Having servant version that supports aeson 2.x would be very nice.

@alpmestan
Copy link
Contributor

Hi! What's the status here? Having servant version that supports aeson 2.x would be very nice.

@akhesaCaro and others: Does this require more than a revision to the servant packages? To simply support aeson 2.x, that is, as opposed to having it as the min bound.

@arianvp
Copy link
Member

arianvp commented Nov 12, 2021

swagger2 now only supports aeson >= 2.0 too So we can fix the dependnecy error by bumping swagger

https://hackage.haskell.org/package/swagger2

@arianvp
Copy link
Member

arianvp commented Nov 12, 2021

Hmm https://github.com/haskell-servant/servant-swagger should probably bump the swagger2 dependency too. Do we plan to merge that repo into the monorepo ?

@akhesaCaro
Copy link
Contributor Author

akhesaCaro commented Nov 12, 2021

Yes it is planned to merge servant-swagger into the monorepo, but as I said, I was waiting for them to bump aeson. It is done since a few days.

I just need some times to finish this PR.
I will try to finish this within the end of the week end.

@maksbotan
Copy link
Contributor

@akhesaCaro would it make sense to move servant-openapi3 from @biocad to monorepo as well?

@akhesaCaro
Copy link
Contributor Author

@maksbotan I think it would make sense to move servant-openapi3 into the servant repo but the repo isn't owned by servant so I don't have any rights on it and it isn't in the scope of this PR imho.

@akhesaCaro
Copy link
Contributor Author

Now Jose is not happy

src/Crypto/JOSE/Types/Internal.hs:58:35: error:
    • Couldn't match type: Data.Aeson.KeyMap.KeyMap Value
                     with: M.HashMap Key Value
      Expected: M.HashMap Key Value
        Actual: Object
    • In the first argument of ‘M.toList’, namely ‘o’
      In the expression: M.toList o
      In an equation for ‘objectPairs’:
          objectPairs (Object o) = M.toList o
   |
58 | objectPairs (Object o) = M.toList o
   |                                   ^
cabal: Failed to build jose-0.8.4 (which is required by
exe:cookbook-hoist-server-with-context from

@akhesaCaro
Copy link
Contributor Author

And now base64-bystestring

Resolving dependencies...
cabal: Could not resolve dependencies:
[__0] trying: servant-auth-server-0.4.6.0 (user goal)
[__1] trying: jose-0.9 (dependency of servant-auth-server)
[__2] next goal: base64-bytestring (dependency of servant-auth-server)
[__2] rejecting: base64-bytestring-1.2.1.0 (conflict: servant-auth-server =>
base64-bytestring>=1.0.0.1 && <1.2)
[__2] skipping: base64-bytestring-1.2.0.1, base64-bytestring-1.2.0.0 (has the
same characteristics that caused the previous version to fail: excluded by
constraint '>=1.0.0.1 && <1.2' from 'servant-auth-server')
[__2] rejecting: base64-bytestring-1.1.0.0 (conflict: jose =>
base64-bytestring>=1.2.1.0 && <1.3)
[__2] skipping: base64-bytestring-1.0.0.3, base64-bytestring-1.0.0.2,
base64-bytestring-1.0.0.1, base64-bytestring-1.0.0.0,
base64-bytestring-0.1.2.0, base64-bytestring-0.1.1.3,
base64-bytestring-0.1.1.1, base64-bytestring-0.1.1.0,
base64-bytestring-0.1.0.3, base64-bytestring-0.1.0.2,
base64-bytestring-0.1.0.1, base64-bytestring-0.1.0.0 (has the same
characteristics that caused the previous version to fail: excluded by
constraint '>=1.2.1.0 && <1.3' from 'jose')
[__2] fail (backjumping, conflict set: base64-bytestring, jose,
servant-auth-server)
After searching the rest of the dependency tree exhaustively, these were the
goals I've had most trouble fulfilling: jose, servant-auth-server,
base64-bytestring

@akhesaCaro
Copy link
Contributor Author

Code needs to be fixed :

src/Servant/Auth/JWT.hs:20:39: error:
    • Couldn't match expected type: HM.HashMap
                                      k0 aeson-2.0.2.0:Data.Aeson.Types.Internal.Value
                  with actual type: containers-0.6.4.1:Data.Map.Internal.Map
                                      T.Text aeson-2.0.2.0:Data.Aeson.Types.Internal.Value
    • In the second argument of ‘HM.lookup’, namely
        ‘(m ^. Jose.unregisteredClaims)’
      In the expression: HM.lookup "dat" (m ^. Jose.unregisteredClaims)
      In the expression:
        case HM.lookup "dat" (m ^. Jose.unregisteredClaims) of
          Nothing -> Left "Missing 'dat' claim"
          Just v
            -> case fromJSON v of
                 Error e -> Left $ T.pack e
                 Success a -> Right a
   |
20 |   decodeJWT m = case HM.lookup "dat" (m ^. Jose.unregisteredClaims) of
   |                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

@akhesaCaro akhesaCaro force-pushed the aeson_2 branch 3 times, most recently from 3e2f28c to 0f160b8 Compare November 14, 2021 13:56
@akhesaCaro
Copy link
Contributor Author

servant-swagger is not happy against older GHC (8.6.5 for instance) :

I thought I would be able to bump aeson without migrating servant-swagger into this repo but it seems that I dont have the choice :)

Resolving dependencies...
cabal: Could not resolve dependencies:
[__0] trying: cookbook-basic-auth-0.1 (user goal)
[__1] trying: base-4.14.1.0/installed-4.14.1.0 (dependency of
cookbook-basic-auth)
[__2] trying: aeson-2.0.2.0 (dependency of cookbook-basic-auth)
[__3] trying: cookbook-uverb-0.0.1 (user goal)
[__4] trying: swagger2-2.7 (dependency of cookbook-uverb)
[__5] next goal: servant-swagger (dependency of cookbook-uverb)
[__5] rejecting: servant-swagger-1.1.10 (conflict: aeson==2.0.2.0,
servant-swagger => aeson>=1.4.2.0 && <1.6)
[__5] skipping: servant-swagger-1.1.9, servant-swagger-1.1.8,
servant-swagger-1.1.7.1, servant-swagger-1.1.7, servant-swagger-1.1.6,
servant-swagger-1.1.5, servant-swagger-1.1.4, servant-swagger-1.1.3.1,
servant-swagger-1.1.3, servant-swagger-1.1.2.1, servant-swagger-1.1.2,
servant-swagger-1.1.1, servant-swagger-1.1 (has the same characteristics that
caused the previous version to fail: excludes 'aeson' version 2.0.2.0)
[__5] rejecting: servant-swagger-1.0.3 (conflict:
base==4.14.1.0/installed-4.14.1.0, servant-swagger => base>=4.7 && <4.9)
[__5] skipping: servant-swagger-1.0.2, servant-swagger-1.0.1,
servant-swagger-1.0 (has the same characteristics that caused the previous
version to fail: excludes 'base' version 4.14.1.0)
[__5] rejecting: servant-swagger-0.1.2 (conflict: swagger2==2.7,
servant-swagger => swagger2>=1 && <2)
[__5] skipping: servant-swagger-0.1.1, servant-swagger-0.1 (has the same
characteristics that caused the previous version to fail: excludes 'swagger2'
version 2.7)
[__5] trying: servant-swagger-0.0.0.1
[__6] next goal: servant (user goal)
[__6] rejecting: servant-0.18.3 (conflict: servant-swagger => servant>=0.4 &&
<0.5)
[__6] skipping: servant-0.18.2, servant-0.18.1, servant-0.18, servant-0.17,
servant-0.16.2, servant-0.16.1, servant-0.16.0.1, servant-0.16, servant-0.15,
servant-0.14.1, servant-0.14, servant-0.13.0.1, servant-0.13, servant-0.12.1,
servant-0.12, servant-0.11, servant-0.10, servant-0.9.1.1, servant-0.9.1,
servant-0.9.0.1, servant-0.9, servant-0.8.1, servant-0.8, servant-0.7.1,
servant-0.7, servant-0.6.1, servant-0.6, servant-0.5 (has the same
characteristics that caused the previous version to fail: excluded by
constraint '>=0.4 && <0.5' from 'servant-swagger')
[__6] rejecting: servant-0.4.4.7, servant-0.4.4.6, servant-0.4.4.5,
servant-0.4.4.4, servant-0.4.4.3, servant-0.4.4.2, servant-0.4.4,
servant-0.4.3.1, servant-0.4.3, servant-0.4.2, servant-0.4.1, servant-0.4.0,
servant-0.2.2, servant-0.2.1, servant-0.2, servant-0.1 (constraint from user
target requires ==0.18.3)
[__6] fail (backjumping, conflict set: servant, servant-swagger)
After searching the rest of the dependency tree exhaustively, these were the
goals I've had most trouble fulfilling: servant, aeson, servant-swagger, base,
swagger2, cookbook-uverb, cookbook-basic-auth
Try running with --minimize-conflict-set to improve the error message.

@akhesaCaro
Copy link
Contributor Author

akhesaCaro commented Nov 17, 2021

Moving servant-swagger into this repo : #1483

@akhesaCaro akhesaCaro marked this pull request as ready for review November 17, 2021 13:32
@akhesaCaro akhesaCaro marked this pull request as draft November 17, 2021 13:33
@akhesaCaro akhesaCaro force-pushed the aeson_2 branch 2 times, most recently from 7d3fd44 to bfc4323 Compare November 18, 2021 10:56
@akhesaCaro akhesaCaro marked this pull request as ready for review November 18, 2021 11:36
@akhesaCaro akhesaCaro requested a review from gdeest November 18, 2021 11:36
@alpmestan
Copy link
Contributor

Could we perhaps allow aeson < 2 and >= 2, with a bit of CPP?

#if MIN_VERSION_aeson(2,0,0)                                                                                    
import qualified Data.Aeson.KeyMap as KM                                                                        
#else                                                                                                           
import qualified Data.HashMap.Strict as KM                                                                      
#endif

... KM.lookup ... 

@akhesaCaro akhesaCaro force-pushed the aeson_2 branch 2 times, most recently from 201b8f2 to 7893659 Compare November 18, 2021 13:23
Copy link
Contributor

@gdeest gdeest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@tchoutri tchoutri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@alpmestan alpmestan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

(Just make sure we don't merge the "messy" commit history of this branch.)

@akhesaCaro akhesaCaro changed the title min bound aeson 2 support Aeson 2 Nov 26, 2021
@akhesaCaro akhesaCaro merged commit 9a39799 into haskell-servant:master Nov 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support aeson-2.0.x
7 participants