User Lookup #143

lakhina · 2017-08-07T04:42:37Z

No description provided.

TimMoore · 2017-08-07T22:46:07Z

@lakhina could you provide a little description of the reason for this change please? How does it fit into the bigger picture? Who will be subscribing to these events and how will they be used?

lakhina · 2017-08-08T01:38:14Z

@TimMoore As you mentioned in issue #120
We could publish user events to Kafka, and have the web-gateway listen to these events and maintain its own internal copy of user display names.

I am just trying to do this for user Lookup. If there is any other way to create user lookup that would be easier than this than you can suggest it and I will then work accordingly.

TimMoore · 2017-08-08T06:40:07Z

@lakhina ... I see. That was one part of a larger comment. Sorry that it was unclear. I've raised #145 with a more detailed description of what I was expecting.

In #120, I mentioned that publishing user events to Kafka and subscribing to them in web-gateway is a possible optimization that could speed it up and make it more resilient if the user service is down for some reason, but it is a more complex solution, and I'm not sure the extra complexity is worth it.

…into lookup

lakhina · 2017-08-10T17:29:28Z

@TimMoore Can you please review? There are still some errors on running.

ignasi35 · 2017-08-11T11:48:03Z

user-impl/src/main/java/com/example/auction/user/impl/UserServiceImpl.java

 import com.lightbend.lagom.javadsl.persistence.PersistentEntityRef;
 import com.lightbend.lagom.javadsl.persistence.PersistentEntityRegistry;

 import javax.inject.Inject;
 import java.time.Instant;
 import java.util.Optional;
 import java.util.UUID;
+import java.util.concurrent.CompletableFuture;



I think there's some unused imports here. I understand in previous commits those were used but after removing the Kafka client from the web-gateway they may be not necessary anymore.

@ignasi35 Thanks for reminding. I will remove it.

ignasi35 · 2017-08-11T12:22:49Z

web-gateway/src/main/java/controllers/AbstractController.java

-                        return Optional.of(u);
-                    }
+        CompletionStage<User> u = userService.getUser(userId.get())
+                .handleRequestHeader(authenticate(userId.get())).invoke();


If userId is empty this will fail. Maybe invoke the remote service only if userId.isPresent. When not present there's no need to combine the users and u CompletionStages.

ignasi35 · 2017-08-11T12:22:50Z

web-gateway/src/main/java/controllers/AbstractController.java

                }
+
                return Optional.empty();
            });


With the change described above you can be sure that userId is present and use get here instead of flatMap.
Also, I don't understand in what case would user.getId().equals(uuid) return false.

ignasi35 · 2017-08-11T12:23:27Z

web-gateway/src/main/java/controllers/AbstractController.java

+        CompletionStage<User> u = userService.getUser(userId.get())
+                .handleRequestHeader(authenticate(userId.get())).invoke();
+        CompletionStage<PaginatedSequence<User>> users = userService.getUsers(Optional.of(0), Optional.of(10))
+                .handleRequestHeader(authenticate(userId.get())).invoke();


When no user has logged in yet, the list of users will not be accessible. This authentication is not required.

ignasi35 · 2017-08-11T12:34:27Z

web-gateway/src/main/java/controllers/Nav.java

+            default: {
+                key = "durationSeconds";
+                break;
+            }


I think this is only sources formatting

Yes, please revert the change in this file, since it isn't part of the changes for this pull request.

ignasi35 · 2017-08-11T12:34:58Z

web-gateway/src/main/java/controllers/TransactionController.java

+                                    .handleRequestHeader(authenticate(user)).invoke();
+                            CompletionStage<PaginatedSequence<User>> users = userService.getUsers(Optional.of(0), Optional.of(10))
+                                    .handleRequestHeader(authenticate(user)).invoke();
+                            transactionFuture.handle((transaction, exception) -> {


No need to reload u and users again. Use the value in nav (line 79).

lakhina · 2017-08-14T03:10:20Z

@ignasi35 @TimMoore Please review.

TimMoore · 2017-08-14T05:19:49Z

user-impl/src/main/java/com/example/auction/user/impl/UserServiceImpl.java

@@ -67,5 +66,4 @@ public UserServiceImpl(PersistentEntityRegistry registry, UserRepository userRep

    private PersistentEntityRef<PUserCommand> entityRef(String id) {
        return registry.refFor(PUserEntity.class, id);
-    }
-}
+    }}


Looks like an accidental formatting change?

TimMoore · 2017-08-14T05:20:21Z

web-gateway/src/main/java/controllers/AbstractController.java

 public abstract class AbstractController extends Controller {
    private final MessagesApi messagesApi;
-    private final UserService userService;
+    public final UserService userService;


It should be protected if it is only used by subclasses.

TimMoore · 2017-08-14T05:22:41Z

web-gateway/src/main/java/controllers/AbstractController.java

+        u = CompletableFuture.completedFuture(Optional.empty()); }
+         CompletionStage<PaginatedSequence<User>> users = userService.getUsers(Optional.of(0), Optional.of(10)).invoke();
+    return users.thenCombineAsync( u, (userPaginatedSequence, maybeLoggedUser) -> new Nav(messagesApi.preferred(Collections.emptyList()), userPaginatedSequence.getItems(), maybeLoggedUser)); }
+


Some unusual formatting in this file. Try using IDEA's auto-formatter.

I think it would also be helpful to refactor it to extract a getUser helper method like this:

private CompletionStage<Optional<User>> getUser(userId) { if (userId.isPresent()) { return userService.getUser(userId.get()) .handleRequestHeader(authenticate(userId.get())) .invoke() .thenApply(resp -> Optional.of(resp)); } else { return CompletableFuture.completedFuture(Optional.empty()); } }

You can then use it like this:

CompletionStage<Optional<User>> u = getUser(userId);

It might also be good to rename u to something more descriptive, such as currentUser.

This makes it more clear what the intent of this method is, and separates out the details of how the current user is retrieved into a separate method.

I would also extract the code that creates the Nav to a separate method that doesn't deal with any CompletionStage:

private Nav getNav(PaginatedSequence<User> users, Optional<User> currentUser) { return new Nav(messagesApi.preferred(Collections.emptyList()), users.getItems(), currentUser); }

Then, you can use it with a method reference rather than a lambda:

return users.thenCombineAsync(u, this::getNav);

This has two benefits:

You don't need to have two different names for the same thing (users & userPaginatedSequence, u or currentUser & maybeLoggedUser)

Each method stays focused on a single level of abstraction and you don't have to mix CompletionStage-handling code with value-handling code

TimMoore · 2017-08-14T05:34:03Z

web-gateway/src/main/java/controllers/ItemController.java

@@ -126,7 +127,7 @@ private ItemData fromForm(ItemForm itemForm) {
                                                        formFactory.form(ItemForm.class).fill(itemForm),
                                                        item.getStatus(),
                                                        Optional.empty(),
-                                                        nav)
+                                                        (Nav) nav)


I think this cast is unnecessary.

TimMoore · 2017-08-14T05:48:02Z

web-gateway/src/main/java/controllers/ItemController.java

-                if (item.getStatus() == ItemStatus.CREATED && !item.getCreator().equals(user)) {
-                    return forbidden();
-                }
+                    }


The logic above isn't quite correct:

What the code should do is look up the user data for the item's seller and winner (if there is a winner) by calling userService.getUser(item.getCreator()) and userService.getUser(item.getAuctionWinner().get()) (if item.getAuctionWinner().isPresent()).

What this code actually does is set's the seller to the current user, or the winner to the current user, only if the seller or the winner is the current user.

For an example of why that's not right, try creating seller and buyer users, then create an item as seller, and view the item as buyer. You get a NullPointerException, because the seller is set to null.

With this change, the user1 variable will not be needed.

TimMoore · 2017-08-14T05:48:57Z

web-gateway/src/main/java/controllers/Nav.java

+            default: {
+                key = "durationSeconds";
+                break;
+            }


Yes, please revert the change in this file, since it isn't part of the changes for this pull request.

TimMoore · 2017-08-14T05:50:07Z

web-gateway/src/main/java/controllers/TransactionController.java

+
+                                            winner = Optional.of(user1);
+
+                                        }


Similar to my comment above: this should be retrieving the seller and winner users, not just comparing them to the current user.

lakhina · 2017-08-14T07:19:16Z

@TimMoore I will make the suggested changes asap.

…into lookup

ignasi35 · 2017-08-18T15:38:13Z

web-gateway/src/main/java/controllers/ItemController.java

+                        } catch (ExecutionException e) {
+                            e.printStackTrace();
+                        }
+


This formatting is not right.

ignasi35 · 2017-08-18T16:02:49Z

web-gateway/src/main/java/controllers/ItemController.java

+                    Optional<User> winner = Optional.empty();
+
+                    try {
+                        seller = userService.getUser(item.getCreator()).invoke().toCompletableFuture().get();


CompletableFuture.get() is a blocking call. All this code requires a (rather complex) composition of futures.

I've been putting some thought on these two calls (load seller and winner) and I think that for the moment we can extract each of them into a separate blocking method.

Java (the language and the API) are not great for composition of futures (CompletionStage) and even less when the call depends on the result of a previous CompletionStage (like in this case where the calls to UserService depend on item).

So, for the time being I think we can:

add an issue in https://github.com/lagom/online-auction-java stating this blocking calls should be removed

extract these two blocks into separate private methods that return User or null (similar to what we had before)

Add a TODO in this code stating: "CompletableFuture.get() is a blocking call, this code should be reviewed with CompletionStage composition."

There are further issues with the code as is: if UserService is down, then these two call will fail, we should work on a fallback. It's probably a good idea to enrich Item with one or two fields with the seller's and winners usernames. This way, when these calls to UserService fail and we can't display the name and surname, we could fallback to username. I would also make this on a separate PR.

Since AbstractController requires the UserService for every request (to lookup the current user and the list of users) there would not be much point adding a fallback here until that is addressed somehow. I don't think that dependencies are something that can or should be eliminated entirely. We also have to be concerned about the complexity of the implementation.

@TimMoore you are right, we can address the UserService-is-down fallback in a separate PR.

ignasi35 · 2017-08-20T19:29:19Z

web-gateway/src/main/java/controllers/TransactionController.java

+
+                                   } catch (ExecutionException e) {
+
+                                   }


This code has the same problems than ItemController. I'm beginning to think that this needs a deeper review: maybe extract common code to AbstractController, maybe creating a UserLoader which may load many users in parallel ( UserLodaer.load(UUID... uuid) -> Map<UUID,User>), or some other way to reuse this code. There are 30 lines of code polluting our main logic and they just load 2 fields.

ignasi35 · 2017-08-23T12:06:49Z

web-gateway/src/main/java/controllers/TransactionController.java

-                        } else {
-                            String msg = exception.getCause().getMessage();
-                            return ok(views.html.transaction.render(showInlineInstruction, Optional.empty(), user, Optional.empty(), Optional.empty(), Optional.empty(), Optional.of(msg), nav));
+                                return null;


This is wrong. This makes the compiler happy because the java compiler will fail to check the returned type on all possible branches matches the expectation so it won't complain.

Your Happy branch ( if (exception == null)) returns a CompletionStage<Result> while the else branch returns a Result. That won't compile.

The problem here is that there is no combinator in CompletionStage<T> that will accept Function2<T, Throwable, CompletionStage<U>> as an argument. That implies that you can't implement the happy path and the recover in the same lambda.
I think a possible solution to this situation is this code:

return transactionFuture.thenCompose(transaction -> { //... CompletionStage<Result> resultCompletionStage = getUser(sellerId).thenCombine( getUser(winnerId), (seller, winner) -> { Currency currency = //... return ok(views.html.transaction.render(// ... } ); // the happy path branch returns a CompletionStage<Result> that is unwrapped in the // `thenCompose` above so the returned type of `transactionFuture.thenCompose(...` // is not `CompletionStage<CompletionStage<Result>>` but // `CompletionStage<Result>` return resultCompletionStage; }).exceptionally(exception -> { // this `exceptionally` is slightly different than before, since previous code only captured // exceptions on `transactionFuture`, while this captures exceptions in the whole // `transactionFuture.thenCompose(...)`. This means that before, if there was an exception // `getUser` it was't captured while this code will String msg = exception.getCause().getMessage(); return ok(views.html.transaction.render(//... } );

…into lookup

lakhina · 2017-08-27T19:13:01Z

@ignasi35 Please review

lakhina · 2017-08-28T15:12:51Z

@TimMoore @ignasi35 Please review

Publish events to Kafka in user service

29ad6c7

TimMoore mentioned this pull request Aug 8, 2017

NoSuchElementException after creating a user #144

Closed

Merge branch 'master' of https://github.com/lagom/online-auction-java …

fbacd0d

…into lookup

lakhina changed the title ~~Publish events to Kafka in user service~~ User Lookup Aug 9, 2017

Merge branch 'master' of https://github.com/lagom/online-auction-java …

4855081

…into lookup

lakhina force-pushed the lookup branch 4 times, most recently from 19e7f52 to 4f338dc Compare August 10, 2017 15:13

ignasi35 reviewed Aug 11, 2017

View reviewed changes

lakhina force-pushed the lookup branch from b3a4bb6 to e8e89b5 Compare August 11, 2017 16:23

TimMoore reviewed Aug 14, 2017

View reviewed changes

lakhina force-pushed the lookup branch from e8e89b5 to 622f06f Compare August 14, 2017 15:33

Modify controllers in web-gateway

bb4e313

lakhina force-pushed the lookup branch from 622f06f to bb4e313 Compare August 14, 2017 15:42

Modify ItemController and Abstract Controller

d199fbb

lakhina force-pushed the lookup branch from 689f564 to d199fbb Compare August 15, 2017 06:17

Merge branch 'master' of https://github.com/lagom/online-auction-java …

7d06d54

…into lookup

lakhina added 2 commits August 16, 2017 19:47

Modify Transaction Controller

4965a33

Modify controllers

a0556d9

lakhina requested a review from octonato August 17, 2017 10:26

ignasi35 reviewed Aug 18, 2017

View reviewed changes

ignasi35 reviewed Aug 20, 2017

View reviewed changes

lakhina added 2 commits August 22, 2017 17:25

Update controllers in webgateway

e83179e

Update Transaction Controller

cdf610f

ignasi35 approved these changes Aug 22, 2017

View reviewed changes

Update Item Controller

50d0928

ignasi35 reviewed Aug 23, 2017

View reviewed changes

Update Transaction Controller

301068c

lakhina force-pushed the lookup branch from ed4167f to 301068c Compare August 23, 2017 12:30

Merge branch 'master' of https://github.com/lagom/online-auction-java …

529e163

…into lookup

TimMoore approved these changes Aug 28, 2017

View reviewed changes

TimMoore merged commit 1745aeb into lagom:master Aug 28, 2017

TimMoore mentioned this pull request Aug 30, 2017

Change single-user lookups by ID in the web-gateway to use the UserService #145

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

User Lookup #143

User Lookup #143

lakhina commented Aug 7, 2017

TimMoore commented Aug 7, 2017

lakhina commented Aug 8, 2017

TimMoore commented Aug 8, 2017

lakhina commented Aug 10, 2017

ignasi35 Aug 11, 2017

lakhina Aug 11, 2017

ignasi35 Aug 11, 2017

ignasi35 Aug 11, 2017

ignasi35 Aug 11, 2017

ignasi35 Aug 11, 2017

TimMoore Aug 14, 2017

ignasi35 Aug 11, 2017

lakhina commented Aug 14, 2017

TimMoore Aug 14, 2017

TimMoore Aug 14, 2017

TimMoore Aug 14, 2017

TimMoore Aug 14, 2017

TimMoore Aug 14, 2017

TimMoore Aug 14, 2017

TimMoore Aug 14, 2017

TimMoore Aug 14, 2017

TimMoore Aug 14, 2017

lakhina commented Aug 14, 2017

ignasi35 Aug 18, 2017

ignasi35 Aug 18, 2017

ignasi35 Aug 20, 2017 •

edited

Loading

TimMoore Aug 20, 2017

ignasi35 Aug 21, 2017

ignasi35 Aug 20, 2017

ignasi35 Aug 23, 2017

ignasi35 Aug 23, 2017

lakhina commented Aug 27, 2017

lakhina commented Aug 28, 2017

User Lookup #143

User Lookup #143

Conversation

lakhina commented Aug 7, 2017

TimMoore commented Aug 7, 2017

lakhina commented Aug 8, 2017

TimMoore commented Aug 8, 2017

lakhina commented Aug 10, 2017

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

lakhina commented Aug 14, 2017

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

lakhina commented Aug 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ignasi35 Aug 20, 2017 • 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

lakhina commented Aug 27, 2017

lakhina commented Aug 28, 2017

ignasi35 Aug 20, 2017 •

edited

Loading