Introduce VersionNumber newtype. by fisx · Pull Request #3075 · wireapp/wire-server

fisx · 2023-02-10T16:20:10Z

See /libs/wire-api/test/unit/Test/Wire/API/Routes/Version.hs for explanation.

Checklist

Add a new entry in an appropriate subdirectory of changelog.d
Read and follow the PR guidelines

See `/libs/wire-api/test/unit/Test/Wire/API/Routes/Version.hs` for explanation.

elland · 2023-02-13T08:06:55Z

.hlint.yaml

+#      . allVersionNumbers @=? cs . toByteString'
+#      <$> allVersionNumbers)
+# ```
+- ignore: { name: Functor law }


Maybe we don't want to ignore Functor law everywhere, but rather where hlint gets tripped up, which seems to be when using the Bilge Assert combinators? 🤔

I wish hlint made it easy for us to disable some stuff per module/target.

I'd prefer to turn off broken laws. It's not like either side of the law is wrong, it's merely an aesthetic preference. Anyone case to weigh in either way?

Maybe it's just me who sometimes write "dumb" functor code and waits for hlint to clean it for me. 🤔

When I comes to dumb mistakes, I'm a good candidate, too 😉

ok, i'm outweighed. :) will fix!

libs/wire-api/src/Wire/API/Routes/Version.hs

changelog.d/5-internal/play-with-version-types

supersven · 2023-02-13T08:33:25Z

libs/wire-api/src/Wire/API/Routes/Version.hs

+  deriving (Arbitrary) via (GenericUniform Version)
+
+versionString :: IsString a => Version -> a
+versionString V0 = "v0"


I think this could be implemented with fromEnum.

The benefit would be that you won't have to touch this function when a new version is introduced. The drawback is that Version constructors have to be sorted.

i used to agree, but been there! :)

Fix ToSchema Version instance. #2978 (review)

d2e52a1

This simple version is fine with me, too 😸

But surely it could be implemented in terms of versionInt.

supersven · 2023-02-13T08:33:57Z

libs/wire-api/src/Wire/API/Routes/Version.hs

+versionString V2 = "v2"
+versionString V3 = "v3"
+
+versionInt :: Integral i => Version -> i


Same here: Could be implemented with fromEnum.

supersven · 2023-02-13T08:44:55Z

libs/wire-api/src/Wire/API/Routes/Version/Wai.hs

-parseVersion :: Request -> Maybe (Request, Integer)
+data ParseVersionError = NoVersion | BadVersion Text
+
+parseVersion :: Request -> Either ParseVersionError (Request, Version)


I'm wondering if this function cannot be generalized regarding its Monad: Shouldn't a constraint MonadError m => ... be sufficient?

(This is nitpicking as far as nitpicking can get ... 😉 )

yes, that would be sufficient, but i intentionally decided against it. i like both the abstract interpretation and the precise type that offers exactly what's needed, and not more.

always happy to nit-pick, though! :)

libs/wire-api/test/unit/Test/Wire/API/Routes/Version.hs

libs/wire-api/src/Wire/API/Routes/Version.hs

supersven

Being able to state if we're referring to an API version or only its number seems to be an improvement to me. 👍

Please feel free to get into my nitpicks or ignore them.

elland

LGTM, just unsure about the hlint changes, although not a blocker.

Co-authored-by: Sven Tennie <sven.tennie@wire.com>

This reverts commit 31c9889.

…ypes' into play-with-version-types

pcapriotti

I would organise this differently:

I wouldn't use the Enum instance of Version to define the schemas, as that is bound to cause confusion. Instead, we can have a list allVersions :: [Version] somewhere and use that. In fact, we should probably remove the Enum instance.
versionString and versionInt can then be defined in terms of the schema instance, as they were (implicitly) before.

pcapriotti · 2023-02-14T08:29:28Z

libs/wire-api/src/Wire/API/Routes/Version.hs

+  deriving (Arbitrary) via (GenericUniform Version)
+
+versionString :: IsString a => Version -> a
+versionString V0 = "v0"


But surely it could be implemented in terms of versionInt.

fisx · 2023-02-15T13:22:28Z

I would organise this differently:

* I wouldn't use the `Enum` instance of `Version` to define the schemas, as that is bound to cause confusion. Instead, we can have a list `allVersions :: [Version]` somewhere and use that. In fact, we should probably remove the `Enum` instance.

* `versionString` and `versionInt` can then be defined in terms of the schema instance, as they were (implicitly) before.

I don't disagree, but I don't understand either: what's the confusion that is caused by the Enum instance?

I think versionString and versionInt are internal and only needed as a place where we can write down explicitly which Int belongs to which Version in order to support removing old versions.

fisx · 2023-03-06T09:26:17Z

federator>   Federator.Remote
federator>     can get response with valid certificate
federator>       when hostname=localhost and certificate-for=localhost:                                                       FAIL (0.03s)
federator>         Error message: Unexpected remote error: RemoteError (SrvTarget {srvTargetDomain = "localhost", srvTargetPort = 34749}) (FederatorClientHTTP2Exception (BadThingHappen ProtocolError "error:05800074:x509 certificate routines::key values mismatch"))
federator>         CallStack (from HasCallStack):
federator>           assertFailure, called at test/unit/Test/Federator/Remote.hs:76:15 in main:Test.Federator.Remote
federator>         Use -p '/when hostname=localhost and certificate-for=localhost/' to rerun this test only.

Happens reproducibly on CI, but I can't reproduce it locally.

$ make c test=1 package=federator TASTY_PATTERN='/when hostname=localhost and certificate-for=localhost/'

fisx · 2023-03-06T09:59:33Z

retried to reproduce locally (only the one test) 15878 times. got the sibling test to fail locally after 57 re-runs:

  Federator.Remote
    can get response with valid certificate
      when hostname=localhost and certificate-for=localhost:                                                       OK (0.10s)
      when hostname=localhost. and certificate-for=localhost:                                                      FAIL (0.03s)
        Error message: Unexpected remote error: RemoteError (SrvTarget {srvTargetDomain = "localhost.", srvTargetPort = 45027}) (FederatorClientHTTP2Exception (BadThingHappen ProtocolError "error:05800074:x509 certificate routines::key values mismatch"))
        
        CallStack (from HasCallStack):
          assertFailure, called at test/unit/Test/Federator/Remote.hs:76:15 in main:Test.Federator.Remote
        Use -p '$0=="Tests.Federator.Remote.can get response with valid certificate.when hostname=localhost. and certificate-for=localhost"' to rerun this test only.
      when hostname=localhost. and certificate-for=localhost.:                                                     OK (0.02s)

fisx · 2023-03-06T10:06:27Z

ok, we established the test is flaky and unrelated. i'll merge once i get lucky with the re-runs.

…types

…-test' into play-with-version-types

…types

(I ran this against develop to establish this tests the right thing.)

…types Conflicts: libs/wire-api/src/Wire/API/Routes/Version.hs tools/stern/src/Stern/Intra.hs

fisx · 2023-03-07T21:15:52Z

Failures:


  test-integration/Test/Spar/Scim/AuthSpec.hs:132:38: 

  1) WireIdPAPIV1.Spar.Scim.Auth, POST /auth-tokens, works with verification code

       uncaught exception: ErrorCall

       Error executing request: responseJsonUnsafeWithMsg: Value Error in $: parsing ASCII failed, expected String, but encountered Null

       CallStack (from HasCallStack):

         error, called at src/Bilge/Response.hs:145:7 in bilge-0.22.0-L77WXGV80XFJMqJLnvaA1h:Bilge.Response

         responseJsonUnsafeWithMsg, called at src/Bilge/Response.hs:134:22 in bilge-0.22.0-L77WXGV80XFJMqJLnvaA1h:Bilge.Response

         responseJsonUnsafe, called at test-integration/Test/Spar/Scim/AuthSpec.hs:163:10 in main:Test.Spar.Scim.AuthSpec

* Introduce VersionNumber newtype. See `/libs/wire-api/test/unit/Test/Wire/API/Routes/Version.hs` for explanation. Co-authored-by: Sven Tennie <sven.tennie@wire.com> Co-authored-by: Paolo Capriotti <paolo@capriotti.io> Co-authored-by: Stefan Matting <stefan@wire.com> Co-authored-by: Leif Battermann <leif.battermann@wire.com>

zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Feb 10, 2023

fisx added 2 commits February 11, 2023 16:15

Introduce VersionNumber newtype.

c9b99ed

See `/libs/wire-api/test/unit/Test/Wire/API/Routes/Version.hs` for explanation.

Tweak hlint.

31c9889

fisx force-pushed the play-with-version-types branch from 2017a86 to 31c9889 Compare February 11, 2023 15:18

Fixup

86c4b1f

fisx marked this pull request as ready for review February 11, 2023 20:56

hi ci

53758c3

fisx mentioned this pull request Feb 12, 2023

internal endpoints swagger (brig, cannon) #3003

Merged

2 tasks

elland reviewed Feb 13, 2023

View reviewed changes

libs/wire-api/src/Wire/API/Routes/Version.hs Outdated Show resolved Hide resolved

elland reviewed Feb 13, 2023

View reviewed changes

changelog.d/5-internal/play-with-version-types Outdated Show resolved Hide resolved

Tweak changelog entry and haddocs.

a8d8ed7

fisx requested a review from elland February 13, 2023 08:27

supersven reviewed Feb 13, 2023

View reviewed changes

libs/wire-api/test/unit/Test/Wire/API/Routes/Version.hs Show resolved Hide resolved

supersven reviewed Feb 13, 2023

View reviewed changes

libs/wire-api/src/Wire/API/Routes/Version.hs Outdated Show resolved Hide resolved

supersven approved these changes Feb 13, 2023

View reviewed changes

elland approved these changes Feb 13, 2023

View reviewed changes

fisx and others added 8 commits February 13, 2023 15:02

More haddocks.

d2e52a1

Update libs/wire-api/src/Wire/API/Routes/Version.hs

ccd4650

Co-authored-by: Sven Tennie <sven.tennie@wire.com>

Revert "Tweak hlint."

5d05349

This reverts commit 31c9889.

Tweak hlint *locally*.

e599619

D'OH!

ecf5249

Merge remote-tracking branch 'refs/remotes/origin/play-with-version-t…

6780480

…ypes' into play-with-version-types

hi ci

4aa8e48

hi ci

7a2bdde

pcapriotti reviewed Feb 14, 2023

View reviewed changes

fisx and others added 4 commits March 5, 2023 13:58

hi ci

7589b2d

Update golden tests

f02ae72

Add assertion for v4 to version test

d3b4f24

Use v2 welcome in end2end tests

d4d6ae4

hi ci

c21c33a

fisx and others added 10 commits March 6, 2023 11:16

Merge branch 'pcapriotti/finalise-v3' into play-with-version-types

d2a2c39

run-services: add status reponses to failure msg

45f30a1

Merge remote-tracking branch 'origin/develop' into play-with-version-…

030f805

…types

using proof in test with max expiration time until 2038

0bbbdd8

changelog

5d7b5f5

Merge remote-tracking branch 'origin/battermann/fix-dpop-access-token…

5cae797

…-test' into play-with-version-types

Merge remote-tracking branch 'origin/develop' into play-with-version-…

5199887

…types

Fix versionMiddleware.

5213043

Test versionMiddleware.

33aefc6

(I ran this against develop to establish this tests the right thing.)

Recover human-readable error messages (almost).

759750f

fisx force-pushed the play-with-version-types branch from 8e02002 to 759750f Compare March 7, 2023 15:58

fisx added 3 commits March 7, 2023 17:05

Merge remote-tracking branch 'origin/develop' into play-with-version-…

1fdee85

…types Conflicts: libs/wire-api/src/Wire/API/Routes/Version.hs tools/stern/src/Stern/Intra.hs

Fixup

c454df8

hi ci

e94dd83

fisx added 2 commits March 7, 2023 22:22

Fix: make version parser in middleware more selective.

1360860

bike shed.

18382ab

fisx merged commit 4456e29 into develop Mar 7, 2023

fisx deleted the play-with-version-types branch March 7, 2023 22:06

fisx mentioned this pull request Mar 28, 2023

Fix syntax for disabelling versions in docs and comments. #3197

Merged

zebot mentioned this pull request Apr 17, 2023

Release 2023-04-17 - (expected chart version 4.35.0) #3230

Merged

Conversation

fisx commented Feb 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

elland Feb 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

supersven left a comment

Choose a reason for hiding this comment

Uh oh!

elland left a comment

Choose a reason for hiding this comment

Uh oh!

pcapriotti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fisx commented Feb 15, 2023

Uh oh!

fisx commented Mar 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fisx commented Mar 6, 2023

Uh oh!

fisx commented Mar 6, 2023

Uh oh!

fisx commented Mar 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Comments

fisx commented Feb 10, 2023 •

edited

Loading

elland Feb 13, 2023 •

edited

Loading

fisx commented Mar 6, 2023 •

edited

Loading