STITCH-2510 Java SDK: Alter AuthListener to better represent User lifecycle #97

jsflax · 2019-02-08T14:11:03Z

No description provided.

…into STITCH-2510

adamchel

Solid start, mainly had some naming feedback to bring the interface to parity with JavaScript, and logic changes in CoreStitchAuth. I think some of this may have been written before we decided to have onActiveUserChanged be called everywhere the active user changes, including login and logout.

android/core/src/main/java/com/mongodb/stitch/android/core/auth/StitchAuthListener.java

android/core/src/main/java/com/mongodb/stitch/android/core/auth/internal/StitchAuthImpl.java

android/core/src/main/java/com/mongodb/stitch/android/core/auth/StitchAuthListener.java

core/sdk/src/main/java/com/mongodb/stitch/core/auth/internal/CoreStitchAuth.java

adamchel · 2019-02-11T22:58:42Z

...java/com/mongodb/stitch/core/services/mongodb/remote/internal/CoreRemoteMongoClientImpl.java

-      );
+          // reinitialize the DataSynchronizer entirely.
+          // any auth event will trigger this.
+          this.dataSynchronizer.reinitialize(


[minor] once you fix CoreStitchAuth to trigger onActiveUserChanged event in all the places where a logout and login can happen, you should only do this reinitialization on the ACTIVE_USER_CHANGED case, and have default be a no-op. Otherwise, the reinitialize will get triggered multiple times on a single login or logout.

adamchel · 2019-02-11T23:02:55Z

...java/com/mongodb/stitch/core/services/mongodb/remote/internal/CoreRemoteMongoClientImpl.java

-      );
+          // reinitialize the DataSynchronizer entirely.
+          // any auth event will trigger this.
+          this.dataSynchronizer.reinitialize(


[q] if there is no active user after the active user has changed, shouldn't we call a different method that just stops the data synchronizer rather than reinitializes it? I see right now that the data synchronizer just ends up being bound to an "unbound" data directory. I guess that's fine but wouldn't it be cleaner if the data synchronizer just didn't run at all when logged out?

It won't run anyway. This won't be an issue until we get to sync cadence. In the interim, this is fine.

server/core/src/main/java/com/mongodb/stitch/server/core/auth/internal/StitchAuthImpl.java

adamchel · 2019-02-11T23:11:03Z

...oid/coretest/src/androidTest/java/com/mongodb/stitch/android/core/StitchAppClientIntTests.kt

+import java.util.concurrent.TimeUnit

 @RunWith(AndroidJUnit4::class)
 class StitchAppClientIntTests : BaseStitchAndroidIntTest() {


[task] I think you should probably test more cases auth listener cases.

check out the JS PR, I added a new file of StitchAuthListenerIntTests, and for each method of the listener, I have a test method that simulates the different ways that event can be triggered, and ensures that the event method is called a certain number of times.

it sounds like a lot but it should be pretty simple to write, and it's a lot cleaner than trying to mix in with existing integration tests

tkaye407 · 2019-02-11T23:28:29Z

android/core/src/main/java/com/mongodb/stitch/android/core/internal/StitchAppClientImpl.java

+  @Override
+  public void onUserLoggedIn(final StitchAuth auth,
+                             final StitchUser loggedInUser) {
+    onRebindEvent(new AuthEvent.UserLoggedIn<>(loggedInUser));


Should you be rebinding on all of these?

Yes, it is up to the service to determine what to do with the information given.

jsflax · 2019-02-12T18:51:46Z

@adamchel PTAL

adamchel

A couple nits, and you should also add tests to the server SDK, but LGTM otherwise!

.../coretest/src/androidTest/java/com/mongodb/stitch/android/core/StitchAuthListenerIntTests.kt

...java/com/mongodb/stitch/core/services/mongodb/remote/internal/CoreRemoteMongoClientImpl.java

server/core/src/main/java/com/mongodb/stitch/server/core/auth/internal/StitchAuthImpl.java

coveralls · 2019-02-13T13:31:54Z

Coverage decreased (-1.5%) to 65.969% when pulling d6dad19 on jsflax:STITCH-2510 into 94c5ed6 on mongodb:master.

adamchel

This is great! Much needed cleanup that will make this code a lot more stable.

Had a few minor questions/concerns but nothing that should block the merge. Would appreciate some answers or the chance to discuss before we merge. Might just require some future work.

adamchel · 2019-02-13T22:07:13Z

core/sdk/src/main/java/com/mongodb/stitch/core/internal/net/EventStreamReader.java

        doneOnce = true;
        line = "";
+      } catch (IOException e) {
+        return null;


[q] why not throw an exception? if we're keeping this, update the comment for this method to explain when this may be null, and add a @Nullable annotation

adamchel · 2019-02-13T22:14:50Z

...mongodb/stitch/core/services/mongodb/remote/sync/internal/NamespaceChangeStreamListener.java

      while (runnerThread.isAlive()) {
        runnerThread.interrupt();
        try {
+          System.out.println("killing runner");


[q] was this for debugging? delete if so. if this would be useful for users, make this an actual log item

adamchel · 2019-02-13T22:18:30Z

...va/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/SyncMongoClientFactory.java

    }

-    final String userId = appInfo.getAuthMonitor().isLoggedIn()
+    final String userId = appInfo.getAuthMonitor().tryIsLoggedIn()


[q] does this need to be a tryIsLoggedIn? I'm concerned that this would cause a real user to end up having their data be in an "unbound" data directory

In the current state of our codebase, it would not be possible for this to happen. However, maybe I'll make a note here for "in the future".

adamchel · 2019-02-13T22:19:32Z

.../stitch/core/services/mongodb/remote/sync/internal/NamespaceChangeStreamListenerUnitTests.kt

        ctx.isLoggedIn = true
        assertFalse(namespaceChangeStreamListener.openStream())
-        verify(nsConfigMock, times(1)).synchronizedDocumentIds
+        verify(nsConfigMock, times(3)).synchronizedDocumentIds


[q] what changed to make this happen? this isn't a performance hit right?

Hmm... it is a slight performance hit. I moved getSynchronizedDocumentIds to the top of openStream to pre-read them before taking the lock. But that may not be necessary anymore.

adamchel · 2019-02-13T22:23:42Z

server/core/src/main/java/com/mongodb/stitch/server/core/internal/StitchAppClientImpl.java

  @Override
-  public boolean isLoggedIn() {
-    return getAuth().isLoggedIn();
+  public boolean isLoggedIn() throws InterruptedException {


[q] If a user ever calls isLoggedIn() method on a StitchAppClient, is there a case where that can throw? should we mark the method as throws in the interface and explain when it could happen?

No, all of this is internal.

ah you're right, there's no isLoggedIn on the app client itself

adamchel · 2019-02-13T22:27:18Z

core/sdk/src/main/java/com/mongodb/stitch/core/auth/internal/CoreStitchAuth.java

+      }
+    } catch (InterruptedException e) {
+      System.err.println("Stream authentication was interrupted.");
+      return null;


[q] are we sure we want to return this? why not just throw the exception? This seems like a null pointer exception waiting to happen somewhere upstream. What happens if the user calls watch and the stream got interrupted? I think it would be better for an error to happen rather than getting a null result.

At the moment, that would have to happen on the users accord. We should probably throw an exception.

adamchel · 2019-02-13T22:27:52Z

core/sdk/src/main/java/com/mongodb/stitch/core/auth/internal/CoreStitchAuth.java

+        throw new StitchClientException(StitchClientErrorCode.MUST_AUTHENTICATE_FIRST);
+      }
+    } catch (InterruptedException e) {
+      System.err.println("Stream authentication was interrupted.");


make this a log statement or get rid of the print

jsflax added 4 commits February 8, 2019 14:10

Java SDK: Alter AuthListener to better represent User lifecycle

e509edd

Update for Adams listener changes

57129ad

Merge branch 'master' of https://github.com/mongodb/stitch-android-sdk …

061a684

…into STITCH-2510

Start unit tests

cb374d9

jsflax requested review from adamchel and tkaye407 February 11, 2019 22:05

jsflax added 2 commits February 11, 2019 22:07

Final touches

a5a27cf

Add catch for admin db

128b01a

adamchel suggested changes Feb 11, 2019

View reviewed changes

tkaye407 reviewed Feb 11, 2019

View reviewed changes

jsflax added 4 commits February 12, 2019 14:19

Address not test comments

bf75f79

More tests

3f99f46

Add rest of tests

ac543f9

Address changes

4c90c0e

adamchel approved these changes Feb 12, 2019

View reviewed changes

jsflax added 4 commits February 13, 2019 10:21

Appease linter

a711734

Add back in missing lock

eb1e138

Fix auth synchronization; appease linter

3400d44

Fix sync synchronicity errors

dca9ede

jsflax added 9 commits February 13, 2019 13:41

Add server tests

14217b4

Read synchronized ids outside of lock, open stream

fe9ddea

Rename switched to active user changed

bab8c96

Add disable listeners feature for unit tests

4c2fca7

Appease linter

ca267f3

Appease linter

508e6f6

Add extra lock on openStream

36aa4c0

Lessen lock

c970830

Add interruptible streams

c045cdc

Add back in closed, fix broken unit tests

660aacd

adamchel reviewed Feb 13, 2019

View reviewed changes

jsflax added 2 commits February 14, 2019 11:03

Added throws, removed stray prints

17fae04

Ensure nsrunner is torn down properly

d6dad19

jsflax merged commit ddd55d7 into mongodb:master Feb 14, 2019

STITCH-2510 Java SDK: Alter AuthListener to better represent User lifecycle #97

STITCH-2510 Java SDK: Alter AuthListener to better represent User lifecycle #97

Uh oh!

Conversation

jsflax commented Feb 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamchel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

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!

jsflax commented Feb 12, 2019

Uh oh!

adamchel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

adamchel left a comment

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!

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!

jsflax commented Feb 8, 2019 •

edited

Loading

coveralls commented Feb 13, 2019 •

edited

Loading