openliberty / guide-cdi-intro Goto Github PK

It looks pretty good (I can't judge the technical coverage but as far as the technology is there I think the guide describes it pretty well). I like the explanations in general - they seem to have the right level of detail. And the length of the guide is much better.

Following on from what we talked about yesterday regarding creating/updating files, yes, I think you need to do the same here with creating the classes then describing them. So have them create the class file (see the intro REST guide for the style) and then explain the relevant bits of the class (or specifically a bean in this case). Where you have the two beans in the scopes section, get them to create the first bean, then describe the relevant bits of it, then ask them to create the second bean and do the same. Start each explanation giving a brief overview of what the point of that particular bean is.

I think I understand why you’ve done it the way you have - because you’re trying to explain scopes and contexts separately from dependency injection? I think you're right to separate the discussion of each (collapsing them together would be messy and confusing) but I think you can just refer back to the code of the bean when you talk about the @Inject annotation in the next section. And, so that someone reading the first section isn't left wondering "but what about that @Inject annotation that hasn't been explained, what's that for?", I think you can just flag up that that will be described in the next section.

So, you could add a sentence at the end of the ‘managing scopes and contexts’ section saying something like “The InventoryManager bean also contains another annotation, @Inject, which will be explained in the next section.” (there might be a better way to say that). Then straight after the ‘Injecting a dependency’ heading, add something like “The InventoryResource bean depends on the InventoryResource bean. Instead of doing xxxx (the thing you’d do without the @Inject annotation), the InventoryManager bean uses the @Inject annotation to automatically call that dependency (or whatever the correct terminology is). Take a look at the @Inject line in the InventoryManager bean code….” Something like that? Give it a try and we can see how it looks/reads.

I also made a note of some style things as I went (though I didn't do a full review for that) and you'll probably get them again from ID review but I've included them here in case you want to fix them first (or talk to the ID person about how to fix them if you're not sure):

• Is the phrase “well-defined contexts” understandable in “You will use scopes to bound objects in this application to their well defined-contexts.”? (maybe, I don’t know)
• Don’t capitalise Contexts etc. They're just words, not names.
• Reduce the amount of future tense (eg only do that in the What you’ll learn section and say “you will work with” instead of “you will be working with” to avoid clunkiness; don't use future tense when describing how the app works; don't use it in sections further down the guide)
• When you mention ‘one function’ and ’the second function’, give the name of the function so its easy to see in the code. Ah, or is (get) and (list) for that purpose? I think they just need highlighting properly (with backticks).
• Remove “Note that….” (“Note that while this bean can also be application-scoped, request scope is short-lived and is therefore ideal for HTTP requests.”) - it’s redundant words. Just explain that while the bean can be app-scoped, it is more appropriate to use the request scope etc.
• I’m not sure about “we’re giving you ”. “You can find a client….”?
• Shorten “This class is responsible for communicating…” to “This class communicates with…”
• Javadocs mentions could do with a link to where to find those javadocs.

Thanks for doing this - it's looking good.

I was wondering why is the SystemClient class in the example https://openliberty.io/guides/cdi-intro.html defined with scope @RequestScoped. It is injected in the InventoryManager which is @ApplicationScoped.

I would expect the same for it as well...

Thank you very much in advance, Fabio

https://stackoverflow.com/questions/55226545/openliberty-tutorial-injecting-dependencies-into-microservices

Sectional Feedback

Introduction

• Add scopes after manage to make sure the introduction is consistent to the first paragraph in What you'll learn section

What you'll learn

• Change the flow of the first paragraph

Handling dependencies in the application

• Minor grammar issues

Multipane Issue

• The hotspot for testSuite() method is yet to be created, as well as for @Test annotation
• For highlighting, exclude annotations when discussing the method to avoid distractions.
• Change instructions in Action Arrow. For example: Change Inventory Manager class file to InventoryManager class

This guide creates a single deployable package (war) for 2 microservices.

The guide uses HTTP to communicate between these 2 services, to prove a point but this is not practical in real world usage. Typically, since all of the classes are in the same class path, you would just execute various methods on the injected classes directly, rather than making HTTP calls to get data between the 'services'.

It might be helpful to simply add a disclaimer note on the guide at a few points to point this fact out.

"This is the staring point of the inventory service and it displays the current contents of the inventory"

Should this not say starting instead of staring?

It looks good to me. The steps are clear.

• The start/pom.xml in line 9 <artifactId>io.openliberty.guides.rest-cdi</artifactId> needs to be changed to look like the finish/pom.xml <artifactId>io.openliberty.guides.cdi-intro</artifactId>.

While reworking the example, I've tried to use the connectTimeout and readTimeout methods on ClientBuilder. These methods have been introduced with JAX-RS 2.1. Curiously, they are not accessible from my IDE (Eclipse) which just sees the JAX-RS 2.0 version.
It seems there is conflict on javax.ws-rs-api versions in the pom.xml :

• cxf-rt-rs-client:3.1.11 depends on javax.ws.rs-api:2.0.1
• jaxrs-2.1:18.0.0.3 depends on javax.ws.rs-api:2.1 but is omitted for conflict with 2.0.1
  As a workaround, I've excluded javax.ws.rs-api dependency from cxf-rt-rs-client in the pom.xml and everything works fine.

Anna was requested to do the user review. She found these issues:

    1. If I copy the first `git clone` and `cd ... ` commands from the site using 'copied to clipboard' button and then paste into terminal, it asks me for a password to the repo. Maybe it doesn't copy the commands correctly. If I do the same thing manually, i.e. get the ssh url from girhub, do `git clone` in terminal manually, then cd the cloned repo, then no problem.
     
    => Clarified. 
       
    2. After going through the steps of building the app in the start folder and running `mvn install` in the directory, I get the following error:
     
    [INFO] ------------------------------------------------------------------------
    [INFO] BUILD FAILURE
    [INFO] ------------------------------------------------------------------------
    [INFO] Total time: 7.387 s
    [INFO] Finished at: 2018-03-06T14:18:05-05:00
    [INFO] Final Memory: 22M/301M
    [INFO] ------------------------------------------------------------------------
    [ERROR] Failed to execute goal org.apache.maven.plugins:maven-failsafe-plugin:2.18.1:integration-test (integration-test) on project io.openliberty.guides.cdi-intro: Execution integration-test of goal org.apache.maven.plugins:maven-failsafe-plugin:2.18.1:integration-test failed: There was an error in the forked process
    [ERROR] java.lang.RuntimeException: Unable to create test class 'it.io.openliberty.guides.inventory..gitkeep'
    [ERROR]     at org.apache.maven.surefire.util.DefaultScanResult.loadClass(DefaultScanResult.java:135)
    [ERROR]     at org.apache.maven.surefire.util.DefaultScanResult.applyFilter(DefaultScanResult.java:95)
    [ERROR]     at org.apache.maven.surefire.junit4.JUnit4Provider.scanClassPath(JUnit4Provider.java:222)
    [ERROR]     at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:107)
    [ERROR]     at org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:203)
    [ERROR]     at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:155)
    [ERROR]     at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:103)
    [ERROR] Caused by: java.lang.ClassNotFoundException: it.io.openliberty.guides.inventory..gitkeep
    [ERROR]     at java.net.URLClassLoader.findClass(URLClassLoader.java:381)
    [ERROR]     at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
    [ERROR]     at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:335)
    [ERROR]     at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
    [ERROR]     at org.apache.maven.surefire.util.DefaultScanResult.loadClass(DefaultScanResult.java:131)
    [ERROR]     ... 6 more
     
    It seems that the test/java/it/io/openliberty/guides/inventory path in 'start' has .gitkeep file that does not exist in the corresponding path of the 'finish'.
     
    => Working on fix. 
     
    3. Further on, the guide states to create 'EndpointTest.java', but the finish folder has 'InventoryEndpointTest.java'. Maybe they should be named the same for consistency. So when I build, I get the following error:
     
    [INFO] BUILD FAILURE
    [INFO] ------------------------------------------------------------------------
    [INFO] Total time: 1.261 s
    [INFO] Finished at: 2018-03-06T14:27:13-05:00
    [INFO] Final Memory: 18M/304M
    [INFO] ------------------------------------------------------------------------
    [ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.1:testCompile (default-testCompile) on project io.openliberty.guides.cdi-intro: Compilation failure
    [ERROR] /Users/annasafonov/Desktop/draft-guide-rest-cdi/start/src/test/java/it/io/openliberty/guides/inventory/EndpointTest.java:[18,8] class InventoryEndpointTest is public, should be declared in a file named InventoryEndpointTest.java
     
    => Fixed.
     
    4. I cannot really run the tests, because I cannot compile the app (point #2).
     
    It took me 20 min to work through the guide, but I was writing these comments as I worked through it. If everything ran smoothly, 12-15 min would probably be enough.
     
    => Thank you!

I don't think buildUrl() and buildClientBuilder() handle exceptions well. They just print two exception names when any Exception is caught. These print statements should have some meaningful text in them and should be unique to each exception.

For example, exception handling in getPropertiesHelper() is done much better, with error messages for different types of exceptions.

• "You will use scopes to bound objects in this application".
  "Bound" should be "bind" here?

• Remind the users where they should run the mvn commands

Get rid of "Now, navigate back to the start directory to begin." under "Try what you'll build" section.

Move the sentence with this context "navigate to the start directory" to the section where user starts working with the implementation.

/github open OpenLiberty/guide-cdi-intro

• Should rename INVENTORY_HOSTS to INVENTORY_SYSTEMS to better reflect the actual value stored in the variable.

• Extra print statement on line 99

Injecting a dependency

• Should InventoryManager bean and InventoryResource class be hotspotted?
• I think SystemClient should be displayed in the right pane when it is being talked about in the final paragraph of this section

Testing the inventory application

• "This client must also be registered with a JSON-P provider (JsrJsonpProvider) to process JSON resources." -> JsrJsonpProvider should hotspot to the line where the client is registered with a JSON-P provider
• "See the following descriptions of the test cases:" -> The test cases following this line should be put in a bulleted list. It looks awkward starting sentences with hotspotted method names
• "Finally, a system test class in the.." -> This should be hotspotted and display the test class in the right pane

Ref: https://ibm.ent.box.com/file/281716465645

It seems that method get() in the InventoryManager is not Thread-safe since it uses a shared SystemClient object.
Also, SystemClient should probably be injected also (with a Request scope, but I wonder how that could be done in a class with Application scope). The tutorial would benefit from that since it would emphasize the benefits of DI

• Check appearance (contents, headings, paragraphs, code snippets, outputs, links, hotspots) of the guide on test site, ie, qa-guides
• Read through the guide
• Clone repo and test finish directory with no error
• Run diff -r start/ finish/
• Follow the instruction in the guide to test that it is working
• Use the copy buttons instead of typing
• Links open in new tab
• Action arrows do not highlight any code
• Update action arrows have hotspotted text underneath
• Full files mentioned in the guide as hotspots should not highlight any code
• Hovering over code commands should switch to the right file

For example, https://github.com/OpenLiberty/guides-common/blob/master/gitclone.adoc has

== Getting started title

The title should not be shown otherwise this will cause a duplicate of titles. Please make sure you remove any title that you reference from guide-common links

mention others if they have the same thing

Replaced 'contexts' with 'scopes' in line 56.

I'm working on an article on CDI's and using this resource as a reference. Just noticed that the term 'contexts' and would like to suggest that 'scopes' might be a more appropriate term here. Please advise.

Still have io.openliberty.guides.cdi-intro for artifactID
it should be cdi-intro
OpenLiberty/guides-common#84

• You need to uncomment common includes (ex. Getting started, Building and running the application, Running the tests, etc.). I didn't add these for the sake of presenting a fully generated html during our prototyping meetings.

What you'll learn

• "You will learn how to use CDI in a simple inventory management application to manage scopes and inject dependencies." --> "You will learn how to use Contexts and Dependency Injection (CDI) in a simple inventory management application to manage scopes and inject dependencies."
  => Not changed since CDI is spelled out in the section under the title.
• "The implementation of the application and its services are provided for you" --> "The implementation of the inventory application and its services are provided for you under the start/src directory. The system service can be found under start/src/main/java/io/openliberty/guides/system and the inventory service can be found under start/src/main/java/io/openliberty/guides/inventory. All common files are located under start/src/main/java/io/openliberty/guides/common."
  => Fixed
• "If you want to learn how to build a RESTful web service" --> "If you want to learn more about RESTful web services and how to build them, follow ..."
  => Fixed
• "You will use CDI to inject dependencies into the application and you will learn how to manage the life cycles of your objects." --> remove this sentence, its a repeat of the first sentence in this section.
  => Reworked
• "The system and Inventory services..." --> remove this sentence, it would be a repeat of the information above (if you choose to add it).
  => Reworked

Handling dependencies in the application

• "You will use scopes and contexts in this application to manage the life cycles of your objects. You will also inject a dependency from one class into another class." --> scopes define contexts from what I understand, so its confusing to say "you will use scopes and contexts" because it makes it seem like they are two different things. I would change this sentences to something like "Objects can be bound to well defined-contexts using scopes. CDI provides a variety of scopes for you to work with and while you will not use all of them in this guide, there is one for almost every scenario that you may encounter. Scopes are defined using CDI annotations." Then add a new paragraph: "Dependency injection enables you to inject a bean in its specified context without having to instantiate it yourself. Use dependency injection to inject one bean into another to make use of its functionalities."
  => Fixed

Managing scopes and contexts

• Change this section title to "Binding stateful components to lifecycle contexts"
  => "Binding stateful components to lifecycle contexts" as a section title seems bit too descriptive, and it's not consistent with the other section title. No change.

• I think here is where you can make the changes we talked about; ie. say something like "Take a look at src/main/java/io/openliberty/guides/inventory/InventoryManager.java. This bean manages the inventory, adds new entries, removes old ones, etc. It also contains the actual storage for all of the entries, essentially making it our database as well. Therefore, it must be persistent between all the clients. To do this, add the @ApplicationScoped annotation on the class. This annotation indicates that this particular bean is to be initialized once per the application scope. By making it application-scoped, the container ensures that the same instance of the bean is used whenever
  its injected in the application." Then list the class. You probably also won't need to mention the ConcurrentMap as a result. Repeat for the other classes.
  => Reworked to have user create the class instead.

• you need to say where the utility components can be found. Use what I had before: "Finally, we are giving you two utility components: a InventoryUtil class and the JsonMessages enum.
  These can be found under src/main/java/io/openliberty/guides/common/inventory/util and src/main/java/io/openliberty/guides/common/common respectively.
  => Fixed.

• The InventoryUtil class is responsible for communicating with the system service to retrieve JVM
  system properties for a particular host that exposes them.

• The JsonMessages enum is used to build JSON Java objects for various error cases. Currently its
  only used to build an error JSON whenever a particular host does not exist or is not running the system service.

Both of these contain detailed Javadocs, so feel free to read them for reference."

Injecting dependency

• The title should be "Injecting a dependency"
  => Fixed.
• Don't say "add the @Inject annotation" because the annotation is already there. Just explain what the annotation is without showing it again. Something like "Recall the InventoryResource class. The @Inject annotation indicates a dependency injection. In this case, you are injecting your InventoryManager bean into the InventoryResource class. This injects the bean in its specified context (application-scoped) and makes all of its functionalities available without the need of instantiating it yourself. The injected bean can then be invoked directly via the manager.get(hostname) and manager.list() function calls."
  => Fixed.

Everything else looks good otherwise. The start and finish are consistent, and the guide is fairly easy to follow. The structure should be good, but it would be better if you make the changes we talked about this morning.
=> Thanks a lot for your review and feedback.

Are the following imports used in InventoryManager.java? I tried removing them and could still access the endpoints.

import io.openliberty.guides.inventory.client.SystemClient;
import javax.inject.Inject;

Similar to #82, I can't see any reason why any of the @RequestScoped classes couldn't be @ApplicationScoped instead as none of them appear to hold any state.

In general, having stateless classes is good, but if they are stateless, they should be @ApplicationScoped.

Note: I had someone in a workshop question why the resource class was RequestScoped.

The init methods in the SystemClient class are behaving like constructors and hence should be replaced by constructors. This is what the SystemClient class should look like:

  public SystemClient(String hostname) {
      this.init(hostname, DEFAULT_PORT);
  }
  
  public SystemClient(String hostname, int port) {
      this.init(hostname, port);
  }

  // Helper method to set the attributes.
  private void init(String hostname, int port) {
    this.url = buildUrl(PROTOCOL, hostname, port, SYSTEM_PROPERTIES);
    this.clientBuilder = buildClientBuilder(this.url);
  }

Then the SystemClient can be initialized whenever its needed (inside methods not as a instance variable) like so:

SystemClient systemClient = new SystemClient(hostname, 9080);

Here's a screenshot:

What you’ll Learn

• "Learn" shouldn't be capitalized
  => fixed. It was inherited from the previous version
• your language is a bit quirky in the section, sentences dont really flow that well. For example, the majority of the inventory application is built in this guide so why are you saying "This guide will complete building this application using CDI services."? It would also sound much better if it instead said something like "You will add CDI to this application to complete it".
  => not relevant anymore as building the inv app part of the guide is removed to shift focus on using cdi only.
• you should also explain what the application does first rather than saying right away thats its made up of two services
  => changed accordingly.
• All in all, I dont think this section needed to be changed at all. It already had CDI focus.
  => same as point 2 above.

Try what you’ll build

• the future tense you added here is not necessary
  => I think present tense is ok too.
• also, I believe only local machines work. Say the team in UK, we won't be able to access their system properties even if they run the system service.
  => I believe as long as the network allows, the system service can run anywhere. I'll double check with team to make sure.

Creating the inventory management application

• I'd rather keep the old section here. There is too many issues with this section now. The whole point of this section was to roughly outline what the user is building, yet you're already saying "inventory manager bean" and "inventory resource". The users are not familiar with these terms yet. Originally, I wanted to explain it at a very high level so its easy for anyone to understand.
  => not relevant anymore as building the inv app part of the guide is removed to shift focus on using cdi only.
• this sentence "CDI scopes and contexts are used in this component to handle the interactions in order to establish a loosely coupled infrastructure." makes no sense. Handle what interactions? What do you mean by loosely coupled infrastructure? See this is the main problem, its too low level. We don't want to go into detail here. The details come later when these classes are actually built.
  => fixed

Creating the inventory manager bean

• "an InventoryUtil class and the JsonMessages enum` class" is incorrect, you don't say "an class", you say "a class" and "an enum", it should be the way it was.
  => didn't think this sentence was changed from previous version of the guide. but it's revised in the latest version already.

Injecting dependency into the inventory resource

• i don't like the title of this section. The users haven't yet written the inventory resource. Perhaps it should isntead be "Creating the inventory resource and injecting dependencies"
  => again, not relevant anymore as building the inv app part of the guide is removed to shift focus on using cdi only.

Testing the inventory application

• "There are a few test case methods to test the functionality of your inventory application." -> "Let's break down the test cases:"
  => good suggestion, changed.
• remove "the" and "method" from test case bullet points. Example: "testEmptyInventory() verifies that the inventory is initially empty when the server first starts up."
  => fixed
• "There are some helper methods that are used throughout the tests. " -> "Finally, notice that you're using a few helper method throughout the tests. You can find complete javadocs for these under the finish directory for reference..."
  => fixed

All in all I don't like the direction you turned this guide (ie. completing a restful web service). This service wouldn't work without CDI, so the user's aren't really completing it, they are fixing it. The guide is also very awkward now, the Application class is provided but everything else is to be written? Then the users are really writing the whole app not just adding cdi into it. I really think its best for them to build everything from scratch the way it was. This way they understand the app better. Especially considering that many of the future guides will build on top.

When I check the InventoryManager class and its mention on the guide, I see that there are differences. Does that mean the code is out of sync or I am missing something?

@Inject
  private SystemClient systemClient;

This part is not available in the code but mentioned InventoryManager class in the guide

Hotspot includes an extra line.