Polysemy Spar: Laws for DefaultSsoCode, Now, IdP and ScimExternalIdStore by isovector · Pull Request #1940 · wireapp/wire-server

isovector · 2021-11-23T20:39:00Z

This PR adds polysemy-check laws for Now, DefaultSsoCode, IdP and ScimExternalIdStore. Now is just a check that the function is monotonic, and the other three are the usual lens laws (+ delete).

The remaining "store" effects in spar have weird TTL semantics that I haven't yet worked out what the laws should be. I'll follow up with those when I figure the semantics out.

Checklist

The PR Title explains the impact of the change.
The PR description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.
A file with the changelog entry in one or more suitable sub-sections. The sub-sections are marked by directories inside changelog.d.

fisx · 2021-11-24T22:31:03Z

The remaining "store" effects in spar have weird TTL semantics that I haven't yet worked out what the laws should be. I'll follow up with those when I figure the semantics out.

you should interview your client on those. :) you can talk to either me or @smatting to learn more.

isovector · 2021-11-25T06:53:13Z

CI seems to be genuinely broken. I'll brave setting up the integration tests locally tomorrow! 😱

smatting · 2021-11-25T08:41:05Z

@isovector FYI, the README.md has a section on how to run them locally.

isovector · 2021-12-01T18:10:47Z

Turns out CI was flakey. Everything is fixed now. @pcapriotti, @fisx , PTAL.

fisx

Would be nice to run this against the cassandra implementation as well. I would like to have the specs in services/src/Spar/Sem/*/Spec.hs, and have a very small integration test suite that just sticks it all together. But this can be done in the a different PR.

But there are some tests in here that I don't understand. Maybe it's time to pair again?

fisx · 2021-12-02T21:16:54Z

services/spar/test/Test/Spar/Sem/NowSpec.hs

+  -- before and after its generators, and check their results for equality. We
+  -- can't use 'Now' as this effect, because 'E.get' won't return equivalent
+  -- results! And we can't keep it empty, because that triggers a crash in
+  -- @polysemy-check@. Thus @Input ()@, which isn't beautiful, but works fine.


if it's an oversight, is there a ticket in polysemy-check for allowing empty effect lists? do you have an idea how to fix it?

services/spar/test/Test/Spar/Sem/ScimExternalIdStoreSpec.hs

services/spar/test/Test/Spar/Sem/IdPSpec.hs

fisx · 2021-12-02T21:37:36Z

services/spar/test/Test/Spar/Sem/IdPSpec.hs

+          E.deleteConfig s
+          pure Nothing


I think this test may be pointless if you haven't been careful about the Arbitrary instance. Does it return the same UUID twice in a few calls with non-negligible probability?

You're right --- it almost certainly doesn't. What would you say are the odds of the generators respecting the size parameter?

I have no idea. :) experiment? I'm not sure that's the right approach, though. We want the keys to come from a small set so that collisions are common, but not anything else. If we didn't use quickcheck but registry, we could "easily" replace the underlying UUID and Issuer generators, but with quickcheck I'm not sure what to do.

(Note to self: the Issuer generator may have not enough entropy, instead of too much. With some of the generators I've been quite lazy.)

services/spar/test/Test/Spar/Sem/IdPSpec.hs

fisx · 2021-12-02T21:52:31Z

services/spar/test/Test/Spar/Sem/IdPSpec.hs

+            getReplacedBy replaced_id,
+          do
+            E.setReplacedBy replaced replacing
+            (Just replacing_id <$) <$> E.getConfig replaced_id


... and I'm not sure this is making too much sense. What are you trying to test here?

This shows that getReplacedBy actually gives you back what you replaced it by, right?

Yes, but why the getConfig? Why just pure $ Just replacing_id? (Sorry, my first question wasn't very helpful. It was late...)

Ah, yeah. Because this is a Maybe (Maybe IdPId) where the outer maybe depends on whether the thing exists in the first place.

services/spar/test/Test/Spar/Sem/IdPSpec.hs

Co-authored-by: fisx <mf@zerobuzz.net>

…-mem-specs

isovector · 2021-12-09T19:49:45Z

@fisx good catch --- coverage checking pointed out that we were hitting lots of pathological cases. I think I've the worrisome ones.

isovector · 2021-12-16T17:51:08Z

@fisx I'd like to get this merged.

fisx · 2021-12-16T20:01:51Z

services/spar/test/Test/Spar/Sem/IdPSpec.hs

-      if s ^. SAML.idpId == s' ^. SAML.idpId
-        then discard
-        else pure ()
+      when (s ^. SAML.idpId == s' ^. SAML.idpId) discard


didn't we say this has negligible probability?

fisx · 2021-12-16T20:10:41Z

changelog.d/5-internal/in-mem-specs

@@ -0,0 +1 @@
+Added laws for DefaultSsoCode, Now, IdP and ScimExternalIdStore


this may be a bit confusing for the casual reader of the release notes. i'll fix it on develop after merging.

isovector added 23 commits November 9, 2021 12:34

Now and AssIDStore interpreters

c3e1dbe

Add AReqIDStore.Mem

263f946

add boolTTL helper

a6235c5

Add BindCookieStore.Mem

db1e01f

Add SAMLUserStore.Mem

2072759

Make format

b948402

Add DefaultSsoCode.Mem

96995d0

Add ScimExternalIdStore.Mem

3db57a0

Add ScimTokenStore.Mem

c4b51ef

Add ScimUserTimesStore.Mem

7f5dbb8

make format

c8bd681

Changelog

2252951

Merge branch 'develop' into in-mem-interpreters

ae3d3ea

Pull out VerdictFormatStore

b983824

Remove stale comment

39737af

Also emit internal state

edfa1eb

DefaultSsoCodeSpec

7b94658

NowSpec

be84e41

Add interpreter names

fb048ae

Generalize IdPSpec

09954ef

IdPSpec

8fd3197

ScimExternalIdStoreSpec

eef3d07

Better names for ScimExternalIdStoreSpec

e2deed3

isovector requested review from fisx and pcapriotti November 23, 2021 20:39

isovector added 3 commits November 23, 2021 12:41

make format

ca0f5ed

changelog.d

4738048

Fix module name

962f2e8

Base automatically changed from in-mem-interpreters to develop November 24, 2021 22:56

isovector added 3 commits November 24, 2021 15:02

Merge branch 'develop' into in-mem-specs

1c0808c

Hi CI

d03760e

More CI

1c2ef7d

isovector added 2 commits December 1, 2021 00:28

CI

e9ee28c

Merge branch 'develop' into in-mem-specs

10e1515

fisx requested changes Dec 2, 2021

View reviewed changes

isovector and others added 10 commits December 2, 2021 16:19

Update services/spar/test/Test/Spar/Sem/IdPSpec.hs

348673e

Co-authored-by: fisx <mf@zerobuzz.net>

Upgrade IdPSpec to 0.9.0.0

2eb6688

Merge branch 'in-mem-specs' of github.com:wireapp/wire-server into in…

d50f114

…-mem-specs

Fix a bug from merge

8f03a63

Update IdPRawMetadataStoreSpec

278a1b9

Fixed DefaultSsoCodeSpec

c2d3e23

Add a prelude to delete/delete

79b43c1

Fix NowSpec

ca156e7

Fix ScimExternalIdStoreSpec

097b0aa

Merge branch 'develop' into in-mem-specs

3d21824

isovector added 2 commits December 9, 2021 23:07

make format

0b5190c

OCI

ee531ae

fisx approved these changes Dec 16, 2021

View reviewed changes

fisx reviewed Dec 16, 2021

View reviewed changes

fisx merged commit 4e1b8fb into develop Dec 16, 2021

fisx deleted the in-mem-specs branch December 16, 2021 20:11

pcapriotti mentioned this pull request Dec 20, 2021

Update polysemy-check version in cabal.freeze #2006

Merged

akshaymankar mentioned this pull request Jan 18, 2022

Release 2022-01-18 #2055

Merged

		@@ -0,0 +1 @@
		Added laws for DefaultSsoCode, Now, IdP and ScimExternalIdStore

Conversation

isovector commented Nov 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

fisx commented Nov 24, 2021

Uh oh!

isovector commented Nov 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smatting commented Nov 25, 2021

Uh oh!

isovector commented Dec 1, 2021

Uh oh!

fisx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fisx Dec 3, 2021 • 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!

Uh oh!

Uh oh!

isovector commented Dec 9, 2021

Uh oh!

isovector commented Dec 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

isovector commented Nov 23, 2021 •

edited

Loading

isovector commented Nov 25, 2021 •

edited

Loading

fisx Dec 3, 2021 •

edited

Loading