SME Review
JunyiSun opened this issue · 2 comments
Feedback from Andy McCright:
Comments:
-
In the example, the
baseURL
includes thesystem
path element (the baseURL ishttp://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 beSystemClient
. -
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 besrc/main/webapp/META-INF/microprofile-config.properties
. -
In the
InventoryManager
class, thegetPropertiesWithGivenHostName
uses theRestClientBuilder
to create an instance of theSystemClient
interface, but it does not register theUnknownUrlExceptionMapper
provider, so it will not throw theUnknownUrlException
. 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!
Andy approved the changes.