mock gundeck #554

fisx · 2018-12-12T21:31:39Z

Notes for the reviewer:

please read commit-by-commit first, ignoring everything about the new modules MockGundeck, Push in the gundeck unit tests. then the overall changes in those two modules.
i am particular fond of the test gundeck's old pushAny against pushAll stuff. this gives strong evidence that the bulk-push code does not change behavior in any undesired way. :)
i changed some types from opaque to exposing constructors. this was mostly for convenience, i think. let me know if you want to reduce the changes there (and why).

Related: #518, #531, #546, #549

fisx · 2018-12-20T13:02:32Z

updated the description. this is ready for review, but i will improve haddocks soon, feel free to wait!

fisx · 2018-12-21T11:44:27Z

all tests passed locally. ready for review!

libs/gundeck-types/src/Gundeck/Types/Push/V2.hs

libs/types-common/src/Data/Id.hs

services/gundeck/src/Gundeck/Push.hs

services/gundeck/test/unit/MockGundeck.hs

neongreen · 2019-01-07T02:36:37Z

I've attempted to review the actual logic here, but failed miserably – probably because I don't know well what the original Gundeck does either.

I will take another stab at it later and ask more questions.

fisx · 2019-01-07T10:46:57Z

thanks for your feedback! i processed everything but commented only where it wasn't obvious what i'm supposed to do. new commit coming up!

services/gundeck/test/unit/MockGundeck.hs

fisx · 2019-01-11T21:12:41Z

there is still an error uncovered by the last test coverage expansion:

TASTY_PATTERN='$NF ~ /native pushes/'  stack test . --pedantic --test --bench --no-run-benchmarks --local-bin-path=dist --fast --test-arguments=--quickcheck-replay=186970 --ghc-options=-Wwarn

pushAny correctly does not send anything; pushAll pushes to a device that shouldn't receive. if i implement connWhitelistSieve as id in mockOldSimpleWebPush, it's the opposite.

hypothesis: something about "empty list means all devices" again. maybe pushAny calls mpyPush with input where empty means full, but pushAll... (where does pushAll call mpyPush? how can the behavior of mockOldSimpleWebPush change the behavior of pushAll?!

services/galley/src/Galley/API/Update.hs

fisx · 2019-01-14T22:19:32Z

TASTY_PATTERN='$NF ~ /native pushes/'  stack test . --pedantic --test --bench --no-run-benchmarks --local-bin-path=dist --fast --test-arguments=--quickcheck-replay=785105 --ghc-options=-Wwarn

device e is native reachable and ws reachable. it gets filtered out by pushConnections, is not sent to the websocket, and therefore gets pushed natively. that sounds quite insane, but both production implementations (with --bulk-push and without) behave that way.

is this a bug in production?

tiago-loureiro · 2019-01-15T08:56:10Z

libs/gundeck-types/src/Gundeck/Types/Push/V2.hs

@@ -222,7 +256,7 @@ data Push = Push
      -- REFACTOR: this make no sense any more since native push notifications have no more payload.
      -- https://github.com/wireapp/wire-server/pull/546
    , _pushNativeAps :: !(Maybe ApsData)
-      -- ^ APNs-specific metadata.
+      -- ^ APNs-specific metadata.  REFACTOR: can this be removed?


Yes it can!

ok, I will remove that sentence.

libs/types-common/src/Data/Id.hs

tiago-loureiro · 2019-01-15T09:19:27Z

services/gundeck/src/Gundeck/Push.hs

-            natives <- mpaNativeTargets psh alreadySent
-            mpaPushNative notif psh natives
+        forM_ resp $ \((notif, psh), alreadySent) -> unless (psh ^. pushTransient) $
+            mpaPushNative notif psh =<< nativeTargets psh alreadySent


Shouldn't this call mpaNativeTargets instead of nativeTargets ?

that doesn't exist any more. instead, pushAll has a constraint that allows mocking this.

fisx · 2019-01-15T09:26:54Z

TASTY_PATTERN='$NF ~ /native pushes/'  stack test . --pedantic --test --bench --no-run-benchmarks --local-bin-path=dist --fast --test-arguments=--quickcheck-replay=785105 --ghc-options=-Wwarn
device e is native reachable and ws reachable. it gets filtered out by pushConnections, is not sent to the websocket, and therefore gets pushed natively. that sounds quite insane, but both production implementations (with --bulk-push and without) behave that way.

is this a bug in production?

Discussed this with @tiago-loureiro, and agreed it's a bug in the production code. I will revert aa0d53c and fix the behavior of pushAny and pushAll.

…e)." This reverts commit aa0d53c.

fisx · 2019-01-15T12:58:24Z

@neongreen wanna have a "last" look? check that 469c78a makes sense and that i didn't break compilePushResps, and see if you want to approve?

thanks for your work on this, that was quite necessary!

neongreen · 2019-01-15T13:07:19Z

services/gundeck/src/Gundeck/Push.hs

@@ -290,6 +292,9 @@ nativeTargets p pres =
    eligibleClient _ RecipientClientsAll = True
    eligibleClient a (RecipientClientsSome cs) = (a^.addrClient) `elem` cs

+    whitelistedOrNoWhitelist a = p^.pushConnections == mempty


null should work too, since both elem and null come from Foldable.

neongreen · 2019-01-15T13:09:24Z

services/gundeck/test/unit/MockGundeck.hs

@@ -494,7 +493,9 @@ handlePushNative Push{..} = do
          isOriginDevice = origin == (uid, Just cid)
          isAllowedPerOriginRules =
            not isOriginUser || (_pushNativeIncludeOrigin && not isOriginDevice)
-      when (isNative && isReachable && isAllowedPerOriginRules) $
+      -- Condition 5: push to cid iff (a) listed in pushConnections or (b) pushConnections is empty.
+      let isWhitelisted = Set.null _pushConnections || fakeConnId cid `elem` _pushConnections


Seems better to have either null+elem or Set.null+Set.member, but not a mix of those.

neongreen · 2019-01-15T13:12:41Z

services/gundeck/src/Gundeck/Push.hs

+                & targetClients .~ case r^.recipientClients of
+                    RecipientClientsAll -> []
+                      -- clients are stored in cassandra as a list with a notification.  empty list
+                      -- is intepreted as "all clients" by 'Gundeck.Notification.Data.toNotif'.


Suggested change

-- is intepreted as "all clients" by 'Gundeck.Notification.Data.toNotif'.

-- is interpreted as "all clients" by 'Gundeck.Notification.Data.toNotif'.

neongreen · 2019-01-15T13:28:36Z

I haven't looked at bulk push and old web push closely, but I hope that Tiago has. If so, I'm in favor of merging this PR.

fisx force-pushed the fisx-mock-gundeck branch 4 times, most recently from c0f586b to b717a7d Compare December 19, 2018 17:22

fisx changed the title ~~[WIP] mock gundeck~~ mock gundeck Dec 19, 2018

fisx force-pushed the fisx-mock-gundeck branch from 2016223 to c9e4a09 Compare December 20, 2018 12:55

fisx force-pushed the fisx-mock-gundeck branch 2 times, most recently from a5650cf to 18edeaf Compare December 21, 2018 11:33

fisx requested a review from tiago-loureiro December 21, 2018 11:44

neongreen reviewed Jan 7, 2019

View reviewed changes

services/gundeck/test/unit/MockGundeck.hs Show resolved Hide resolved

neongreen reviewed Jan 7, 2019

View reviewed changes

services/gundeck/test/unit/MockGundeck.hs Outdated Show resolved Hide resolved

fisx added 14 commits January 7, 2019 22:41

Cleanup (whitespace).

3189238

Haddocks.

f2671de

Derive some std instances (needed later).

0714173

More generous module surfaces for gundeck.

a396799

Cleanup.

429d76f

Refactor: make gundeck more mockable (needed for upcoming tests).

0e40c35

Property-based tests for gundeck's main pushAll function.

17dde8d

Cleanup.

6e92e8d

Fix: lost push notifications with push-all enabled.

29f40c4

Cleanup (shuffle code).

41238e1

Property-based tests: test gundeck's old pushAny against pushAll.

ad51f7c

Mock gundeck: test cassandra storage.

1d3daad

Cleanup.

6cc2ef8

Extend test coverage.

24699a7

fisx added 3 commits January 11, 2019 13:32

Fixup

ef6d179

Fix test: write notifs with RecipientClientsAll to cass.

878820c

Fix test: make genPush reasonably exhaustive.

3afc558

neongreen reviewed Jan 14, 2019

View reviewed changes

services/galley/src/Galley/API/Update.hs Show resolved Hide resolved

fisx added 4 commits January 14, 2019 14:59

Merge branch 'develop' into fisx-mock-gundeck

0f9bbc8

Fixup

7ed9f7b

Fixup

83e02ad

Fixup

b52a139

fisx added 2 commits January 14, 2019 23:29

Fixup (even though this may need fixing in the production code).

aa0d53c

Cleanup.

7188045

fisx changed the title ~~[WIP] mock gundeck~~ mock gundeck Jan 14, 2019

tiago-loureiro reviewed Jan 15, 2019

View reviewed changes

libs/types-common/src/Data/Id.hs Show resolved Hide resolved

tiago-loureiro reviewed Jan 15, 2019

View reviewed changes

fisx added 6 commits January 15, 2019 12:07

Revert "Fixup (even though this may need fixing in the production cod…

b32dc81

…e)." This reverts commit aa0d53c.

Fix: do not push natively to devices if they are not on the whitelist.

469c78a

Improve REFACTOR plan.

f53a88c

Refactor some ClientId deserialization instances.

073df93

Fixup

3a1c4bd

Fixup

939e522

neongreen approved these changes Jan 15, 2019

View reviewed changes

Fixup

507452e

fisx merged commit 5d5bf0e into develop Jan 15, 2019

fisx deleted the fisx-mock-gundeck branch January 15, 2019 16:03

fisx mentioned this pull request Jan 24, 2019

Release/2019-01-24 #596

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mock gundeck #554

mock gundeck #554

fisx commented Dec 12, 2018 •

edited

fisx commented Dec 20, 2018

fisx commented Dec 21, 2018

neongreen commented Jan 7, 2019

fisx commented Jan 7, 2019

fisx commented Jan 11, 2019 •

edited

fisx commented Jan 14, 2019

tiago-loureiro Jan 15, 2019

fisx Jan 15, 2019

tiago-loureiro Jan 15, 2019

fisx Jan 15, 2019

fisx commented Jan 15, 2019

fisx commented Jan 15, 2019

neongreen Jan 15, 2019

neongreen Jan 15, 2019

neongreen Jan 15, 2019

neongreen commented Jan 15, 2019

	-- is intepreted as "all clients" by 'Gundeck.Notification.Data.toNotif'.
	-- is interpreted as "all clients" by 'Gundeck.Notification.Data.toNotif'.

mock gundeck #554

mock gundeck #554

Conversation

fisx commented Dec 12, 2018 • edited

fisx commented Dec 20, 2018

fisx commented Dec 21, 2018

neongreen commented Jan 7, 2019

fisx commented Jan 7, 2019

fisx commented Jan 11, 2019 • edited

fisx commented Jan 14, 2019

tiago-loureiro Jan 15, 2019

Choose a reason for hiding this comment

fisx Jan 15, 2019

Choose a reason for hiding this comment

tiago-loureiro Jan 15, 2019

Choose a reason for hiding this comment

fisx Jan 15, 2019

Choose a reason for hiding this comment

fisx commented Jan 15, 2019

fisx commented Jan 15, 2019

neongreen Jan 15, 2019

Choose a reason for hiding this comment

neongreen Jan 15, 2019

Choose a reason for hiding this comment

neongreen Jan 15, 2019

Choose a reason for hiding this comment

neongreen commented Jan 15, 2019

fisx commented Dec 12, 2018 •

edited

fisx commented Jan 11, 2019 •

edited