REPORT-862:Create an HttpReportProcessor #196
Conversation
| @@ -0,0 +1,64 @@ | |||
| package org.openmrs.module.reporting.report.processor; | |||
| import org.springframework.stereotype.Component; | ||
|
|
||
| @Component | ||
| public class HttpReportProcessor implements ReportProcessor { |
| ret.add("connection url"); | ||
| ret.add("subject"); | ||
| ret.add("Add report"); | ||
| return ret; |
There was a problem hiding this comment.
These should not contain spaces in the names. Please make these camel case.
There was a problem hiding this comment.
Since you refer to these names below, you should also make them into Constants and then refer to the constants in each place.
There was a problem hiding this comment.
@mseaton kindly review ..Thanks
| URL url = new URL(configuration.getProperty("connection url")); | ||
| HttpURLConnection connection = (HttpURLConnection) url.openConnection(); | ||
| connection.setRequestMethod("POST"); | ||
| connection.setRequestProperty("Content-Type", "text/html; charset=UTF-8"); |
There was a problem hiding this comment.
Why limit this to text/html? Wouldn't it be better to get the report.getOutputContentType(), and set this content type?
| connection.connect(); | ||
|
|
||
| if (report.getRenderedOutput() != null && "true".equalsIgnoreCase(configuration.getProperty("Add report"))) { | ||
| String addcontent = configuration.getProperty(("Add report") + configuration.getProperty("subject")); |
There was a problem hiding this comment.
I don't really understand what all of this is doing with the "Add report" and "subject" properties. Is this meant to be like saying "enabled=true/false"?
There is no javadoc, no examples, and no unit tests showing what is expected here. All of that needs to be added.
| String addcontent = configuration.getProperty(("Add report") + configuration.getProperty("subject")); | ||
| if (report.getOutputContentType().contains("text") || report.getOutputContentType().contains("html")) { | ||
| addcontent = new String(report.getRenderedOutput(),"UTF-8"); | ||
| } |
There was a problem hiding this comment.
Per my comment above, there is no reason to limit this to text/html, is there?
| connection.getOutputStream()); | ||
| writer.write(addcontent); | ||
| writer.flush(); | ||
| writer.close(); |
There was a problem hiding this comment.
Does this need to be in a finally block?
There was a problem hiding this comment.
No because the finally blocks can't be inserted in try-catch blocks
There was a problem hiding this comment.
With I/O, typically it's a good idea to close the connection in a finally block. You can have nested try/catch blocks if you need to. In this case, I would declare "HttpURLConnection" outside of the try block, assign it and use it inside the try block, and close it in the finally block:
HttpURLConnection connection;
OutputStreamWriter writer;
try {
connection = (HttpURLConnection) url.openConnection();
...
writer = new OutputStreamWriter(connection.getOutputStream());
...
}
catch (Exception e) {
throw new RuntimeException("Error occurred while sending report via HTTP POST", e);
}
finally {
IOUtils.closeQuietly(writer);
IOUtils.closeQuietly(connection);
}
There was a problem hiding this comment.
@mseaton the connection variable is as a result of casting the url variable to HttpURLConnection of which if am to declare "HttpURLConnection" outside of the try block then i will need to declare URL outside to, which requires also a try block to catch the MalformedURLException and if this is done,then the scope of the url variable used here url.openConnection() will be limited
| HttpURLConnection connection = (HttpURLConnection) url.openConnection(); | ||
| connection.setRequestMethod("POST"); | ||
| String outPutContentType = report.getOutputContentType(); | ||
| connection.setRequestProperty("Content-Type", "outPutContentType; charset=UTF-8"); |
There was a problem hiding this comment.
@mseaton i don't understand clearly what you mean here..please elaborate
| connection.getOutputStream()); | ||
| writer.write(addcontent); | ||
| writer.flush(); | ||
| writer.close(); |
There was a problem hiding this comment.
With I/O, typically it's a good idea to close the connection in a finally block. You can have nested try/catch blocks if you need to. In this case, I would declare "HttpURLConnection" outside of the try block, assign it and use it inside the try block, and close it in the finally block:
HttpURLConnection connection;
OutputStreamWriter writer;
try {
connection = (HttpURLConnection) url.openConnection();
...
writer = new OutputStreamWriter(connection.getOutputStream());
...
}
catch (Exception e) {
throw new RuntimeException("Error occurred while sending report via HTTP POST", e);
}
finally {
IOUtils.closeQuietly(writer);
IOUtils.closeQuietly(connection);
}
| connection.setRequestProperty("Content-Type", "outPutContentType; charset=UTF-8"); | ||
| connection.setDoOutput(true); | ||
| connection.connect(); | ||
| //when the parameter ADD_REPORT is set to true then the rendered report is added to the url connection |
There was a problem hiding this comment.
I still don't quite understand this. What happens if ADD_REPORT is set to false or left empty? Why are you opening a connection above at all in this case if you are not actually writing anything to it? If the intention is to send the subject to that URL but not the report, then this code isn't achieving that. If the intention is to just disable this processor altogether, then put this check at the very top before you do anything with a connection at all...
There was a problem hiding this comment.
Sir the intention to send the subject to the report.
Am going to add the ADD_REPORT to the very top because my intention is that when its not set or false then the processor is disabled.
i used connection.connect() because it opens a connection link to a resource referenced by the URL but i will go ahead remove it
link:https://issues.openmrs.org/browse/REPORT-862?src=confmacro