OpenLiberty/guide-microprofile-rest-client

SME Review

JunyiSun opened this issue · 2 comments

Feedback from Andy McCright:
Comments:

  • In the example, the baseURL includes the system path element (the baseURL is http://localhost:9080/system), so this line:
    When the getProperties() method gets invoked, the system client sends a GET request to the endpoint at <baseUrl>/system/properties.
    should be changed to:
    When the getProperties() method gets invoked, the system client sends a GET request to the endpoint at <baseUrl>/properties.

  • Not critical, but I would change these sentences:
    @RegisterProvider() adds any provider classes to this interface; you can add as many providers as necessary. In the system client, you add a response exception mapper as a provider to map the 404 NOT FOUND response code with the UnknownUrlException.
    to something like:
    The @RegisterProvider annotation will tell the framework to register provider classes to be used when invoking this interface; you can add as many providers as necessary. In the SystemClient, you add a response exception mapper as a provider to map the 404 NOT FOUND response code with the UnknownUrlException.

  • Likewise in the first sentence of the Handling exceptions through ResponseExceptionMappers section, you have system client, but it should probably be SystemClient.

  • Also, not critical, but it would save some space to remove the mostly blank javadoc for the UnknownUrlException constructor - specifically:
    /** * @param message */

  • I would change this sentence:
    The handles() method intercepts the HTTP response code, and the toThrowable method throws the mapped exception.
    to:
    The handles() method inspects the HTTP response code to determine if an exception should be thrown for this response, and the toThrowable method returns the mapped exception.

  • Same comment as 3 for the first paragraph under the Instantiating the RESTful client header - "system client" should be "SystemClient". Same thing under the Injecting the client with CDI and Config header.

  • Typo: src/main/webapp/META-IF/microprofile-config.properties should be src/main/webapp/META-INF/microprofile-config.properties.

  • In the InventoryManager class, the getPropertiesWithGivenHostName uses the RestClientBuilder to create an instance of the SystemClient interface, but it does not register the UnknownUrlExceptionMapper provider, so it will not throw the UnknownUrlException. This line (in InventoryManager.java):
    SystemClient customRestClient = RestClientBuilder.newBuilder().baseUrl(customURL).build(SystemClient.class);
    should be changed to:

                                                                                       .baseUrl(customURL)
                                                                                       .register(UnknownUrlExceptionMapper.class)
                                                                                       .build(SystemClient.class);

Overall, this looks really good! Thanks for putting this together!

Fixed by #15

Andy approved the changes.