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

Moving away from lens to provide classy optics #101

Open
Kleidukos opened this issue Jun 28, 2021 · 4 comments
Open

Moving away from lens to provide classy optics #101

Kleidukos opened this issue Jun 28, 2021 · 4 comments

Comments

@Kleidukos
Copy link

After a discussion with @frasertweedale, it has been decided to explore available options to end the dependency on lens for the API, while still retaining the classy optics that are not moving anywhere soon.
There is also a plan to get rid of TH for optics generation.

Points to discuss:

  • Control.Lens.Cons.Cons is used throughout the API. This will incur an API breakage, but won't be a show-stopper.
@frasertweedale
Copy link
Owner

@Kleidukos thanks for filing. I'm at end of my day but will return tomorrow and set out all the API breaks I can think of, for discussion on how to proceed, and some rough ideas about the order in which to do things.

@frasertweedale
Copy link
Owner

@Kleidukos sorry for the delay, I should manage to get to it in the next couple days.

@frasertweedale
Copy link
Owner

OK, sorry it took so long. Rough plan:

  1. Define lens-compatible optic type synonyms and helper functions in new module Crypto.JOSE.Lens. The module can be exposed. It's fine to only implement what jose actually uses.
  2. In the library only, replace uses of the lens types, synonyms and functions with our own. But do not change the definitions of any of the optics until later steps. Also defer Control.Lens.Cons stuff for a later step.
  3. Rip out all the lens TH and hand-roll the classes/instances/accessors. This will be the most toilsome part.
  4. For types using Cons or AsEmpty, convert as suggested below (feel free to discuss). Because this is a breaking API change, the tests and example program may need updates too.
  5. Update definitions of other optics, if any.
  6. Last of all (and optional) remove use of lens from the test suite and example program, replacing with the values defined in jose.

Commits should follow the plan above, more or less.

Cons and AsEmpty conversions:

  • Crypto.JOSE.JWS.signature: s ~ Data.ByteString.ByteString
  • Crypto.JOSE.JWS.signJWS: s ~ Data.Bytestring.Lazy.ByteString
  • Crypto.JOSE.JWS.verifyJWS, verifyJWS and verifyJWSWithPayload: s ~ Data.ByteString.Lazy.ByteString
  • Crypto.JOSE.JWK.fromOctets: s ~ Data.ByteString
  • Crypto.JOSE.Types.Internal.base64url: Either we split it into two prisms: one for strict ByteString and one for lazy. Alternatively, define as a method of a new type class that provides the conversions, e.g. class Base64URL dec enc where base64url :: Prism' dec enc with four instances (strict strict, strict lazy, lazy strict, lazy lazy). I think I prefer the second approach provides better (though imperfect) compatibility for existing code. Note the specific implementation for lazy, which we currently don't use: https://hackage.haskell.org/package/base64-bytestring-1.2.0.0/docs/Data-ByteString-Base64-URL-Lazy.html.
  • Crypto.JWT.stringOrUri: s ~ Data.Text.Text

Note that use of lazy ByteString where the payload is concerned does not match with how the JWS is defined. Specifically, the payload is stored as a Types.Base64Octets, which is a newtype wrapper around strict ByteString. We can deal with this mismatch now (e.g. by defining a different newtype for lazy bytestring or abstracting Base64Octets over the representation. Or feel free to punt on it and just do the conversions wherever necessary, and I will follow up later.

I probably missed some stuff and we will no doubt have more to discuss as the work gets tacked.

@Kleidukos
Copy link
Author

@frasertweedale I can't say I'm the most comfortable with the codebase, but if you're available for some pairing one day, I would love that. :)

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

No branches or pull requests

2 participants