Fix request handle un registration #63

deepueg · 2020-01-06T22:19:13Z

This PR fixes an issue with unregistering request handlers.

The old implementation was unregistering wrong request handlers due to a bad UUID mapping
Now, when a new request handler registration comes in the old one is automatically removed before registering the new one.

Added more tests.

friederbluemle

Nice one!

Just confirming and double checking a couple of things:

There can only be one registered handler for the request/response functionality (unlike "events", which can have multiple handlers). Is that assumption correct?
Previously, what would happen if multiple handlers were registered for request/response? I guess we did not have a test case for that..

friederbluemle · 2020-01-06T22:40:46Z

...ge/src/androidTest/java/com/walmartlabs/electrode/reactnative/bridge/RequestHandleTests.java

+import static junit.framework.TestCase.assertSame;
+import static junit.framework.TestCase.fail;
+
+public class RequestHandleTests extends BaseBridgeTestCase {


Should the class name be RequestHandlerTests?

Nice one!

Just confirming and double checking a couple of things:

There can only be one registered handler for the request/response functionality (unlike "events", which can have multiple handlers). Is that assumption correct?

Previously, what would happen if multiple handlers were registered for request/response? I guess we did not have a test case for that..

Yes, we can only have one request handler at any given point of time.

Previously, the mRequestNameByUUID map was not updated when a new request handler is registered. Due to this, when someone calls unregisterRequestHandler on an old UUID the implementation was removing the current one.

RequestHandle is the return type that stores the ID, hence kept this name. Guess it can be either.

OK.. Just keep it. We can consider some naming refactoring/simplification for v2.

friederbluemle · 2020-01-06T22:49:54Z

...tive-bridge/src/main/java/com/walmartlabs/electrode/reactnative/bridge/RequestRegistrar.java

@@ -59,6 +59,6 @@
     * @param name
     * @return {@link UUID} of the request handler
     */
-    @NonNull
+    @Nullable


Technically, this change is breaking and would require a new major version - So question: Is this required or can you keep it NonNull? If the change is required, other parts need updates as well, e.g. the getRequestHandlerId implementation in ElectrodeBridgeTransceiver - It is still marked as NonNull, but can now return null. That in turn might require other code updates and refactoring.

This was technically not used anywhere till now, So it should be okay to change the signature. I will update the ElectrodeBridgeTransceiver as well.

Also, if you look at the old implementation it was done incorrectly. A null value was returned if a match is not found.

Makes sense 👍

friederbluemle · 2020-01-06T22:50:34Z

...-bridge/src/main/java/com/walmartlabs/electrode/reactnative/bridge/RequestRegistrarImpl.java

 import java.util.Map;
 import java.util.UUID;
 import java.util.concurrent.ConcurrentHashMap;

 public class RequestRegistrarImpl<T> implements RequestRegistrar<T> {
+    private static final String TAG = RequestRegistrarImpl.class.getSimpleName();
+


Replace double blank line with single blank line.

friederbluemle · 2020-01-06T23:02:42Z

...idge/src/main/java/com/walmartlabs/electrode/reactnative/bridge/RequestHandlerProcessor.java

@@ -91,7 +91,7 @@ public void onSuccess(TResp obj) {

    @Override
    public boolean unregister() {
-        Logger.d(TAG, "Removing registered request handler %s with id %s", handler, id);
+        Logger.w(TAG, "Removing registered request handler %s with id %s", handler, id);


Is this now an unexpected ("warning") case, or can it stay "debug"?

oops. Good catch!. typo.

In fact, I will clean up this logic now.

friederbluemle · 2020-01-06T23:05:28Z

...-bridge/src/main/java/com/walmartlabs/electrode/reactnative/bridge/RequestRegistrarImpl.java

            return mRequestHandlerByRequestName.remove(requestName);
+        } else {
+            Logger.d(TAG, "Request handler(id: %s) already removed", requestHandlerUuid);


The other two added Logger usages have info level, and here it's debug (with a very similar message) - Keep it consistent? Either debug or info would be fine I think..

I will drop it to debug

deepueg · 2020-01-06T23:39:40Z

@friederbluemle Updated the PR.

friederbluemle

Thanks @deepueg 👍

Consider rewording commit message to fix typo before merge:
Move RequestHandle test cases to it's own class
->
Move RequestHandle test cases to its own class
OR simply
Move RequestHandle test cases to separate class

Also, remove period from commit message subject line.

Add more tests. The old implementation was unregistering wrong request handlers due to a bad UUID mapping Now, when a new request handler registration comes in the old one is automatically removed before registering the new one.

deepueg requested a review from friederbluemle January 6, 2020 22:19

friederbluemle reviewed Jan 6, 2020

View reviewed changes

deepueg force-pushed the fix-request-handle-un-registration branch 2 times, most recently from 4bf20bc to d629db1 Compare January 6, 2020 23:38

friederbluemle approved these changes Jan 6, 2020

View reviewed changes

deepueg force-pushed the fix-request-handle-un-registration branch 2 times, most recently from 1e9c6fa to 773fefd Compare January 6, 2020 23:42

deepueg added 2 commits January 6, 2020 15:46

Move RequestHandle test cases to separate class

65d1bfc

Fix request handler unregistration issue

d352e2c

Add more tests. The old implementation was unregistering wrong request handlers due to a bad UUID mapping Now, when a new request handler registration comes in the old one is automatically removed before registering the new one.

deepueg force-pushed the fix-request-handle-un-registration branch from 773fefd to d352e2c Compare January 6, 2020 23:47

deepueg merged commit 492d585 into electrode-io:master Jan 7, 2020

deepueg deleted the fix-request-handle-un-registration branch January 7, 2020 00:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix request handle un registration #63

Fix request handle un registration #63

deepueg commented Jan 6, 2020

friederbluemle left a comment

friederbluemle Jan 6, 2020

deepueg Jan 6, 2020 •

edited

Loading

deepueg Jan 6, 2020

friederbluemle Jan 6, 2020

friederbluemle Jan 6, 2020

deepueg Jan 6, 2020

deepueg Jan 6, 2020

friederbluemle Jan 6, 2020

friederbluemle Jan 6, 2020

friederbluemle Jan 6, 2020

deepueg Jan 6, 2020

deepueg Jan 6, 2020

friederbluemle Jan 6, 2020

deepueg Jan 6, 2020

deepueg commented Jan 6, 2020

friederbluemle left a comment

Fix request handle un registration #63

Fix request handle un registration #63

Conversation

deepueg commented Jan 6, 2020

friederbluemle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepueg Jan 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepueg commented Jan 6, 2020

friederbluemle left a comment

Choose a reason for hiding this comment

deepueg Jan 6, 2020 •

edited

Loading