-
Notifications
You must be signed in to change notification settings - Fork 521
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
O3-310: allow Put operations on an application-writable config.json file to openmrs/data/frontends/config.json #595
base: master
Are you sure you want to change the base?
Conversation
.../module/webservices/rest/web/v1_0/controller/openmrs2_4/FrontendJsonConfigController2_4.java
Outdated
Show resolved
Hide resolved
.../module/webservices/rest/web/v1_0/controller/openmrs2_4/FrontendJsonConfigController2_4.java
Outdated
Show resolved
Hide resolved
.../module/webservices/rest/web/v1_0/controller/openmrs2_4/FrontendJsonConfigController2_4.java
Outdated
Show resolved
Hide resolved
.../module/webservices/rest/web/v1_0/controller/openmrs2_4/FrontendJsonConfigController2_4.java
Outdated
Show resolved
Hide resolved
.../module/webservices/rest/web/v1_0/controller/openmrs2_4/FrontendJsonConfigController2_4.java
Outdated
Show resolved
Hide resolved
.../module/webservices/rest/web/v1_0/controller/openmrs2_4/FrontendJsonConfigController2_4.java
Outdated
Show resolved
Hide resolved
.../module/webservices/rest/web/v1_0/controller/openmrs2_4/FrontendJsonConfigController2_4.java
Outdated
Show resolved
Hide resolved
.../module/webservices/rest/web/v1_0/controller/openmrs2_4/FrontendJsonConfigController2_4.java
Outdated
Show resolved
Hide resolved
@jnsereko shouldn't we also have the option to locally download the config.json file from the server storage via a GET HTTP request? |
.../module/webservices/rest/web/v1_0/controller/openmrs2_4/FrontendJsonConfigController2_4.java
Outdated
Show resolved
Hide resolved
.../module/webservices/rest/web/v1_0/controller/openmrs2_4/FrontendJsonConfigController2_4.java
Outdated
Show resolved
Hide resolved
} | ||
} | ||
|
||
private boolean isValidAuthFormat(HttpServletResponse response, String basicAuth) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we even have an authorization check here? This can be handled by the filter in this module and we should be able to just reject things if there's no user or the user isn't an administrator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some doubts regarding our current authorisation filters.
When i execute a request with/without an Auth header, it fails the first time but succeeds on subsequent tries, hence i resorted to retaining the authorisation confirmation logic. However, i am going to remove it.
.../module/webservices/rest/web/v1_0/controller/openmrs2_4/FrontendJsonConfigController2_4.java
Outdated
Show resolved
Hide resolved
.../module/webservices/rest/web/v1_0/controller/openmrs2_4/FrontendJsonConfigController2_4.java
Outdated
Show resolved
Hide resolved
if (!folder.isDirectory()) { | ||
log.warn("SPA frontend repository is not a directory hence creating it at: " + folder.getAbsolutePath()); | ||
if (!folder.mkdirs()) { | ||
throw new ModuleException("Failed to create the SPA frontend repository folder at: " + folder.getAbsolutePath()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why ModuleException
? That shouldn't be used here. Also, it's not great to echo a file path back in response to a web request...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think throwing an IOException
is better
|
||
private static final Logger log = LoggerFactory.getLogger(FrontendJsonConfigController1_9.class); | ||
|
||
@RequestMapping(method = RequestMethod.GET) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The formatting here looks ... 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you address the formatting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed the extra spaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And the indention?
There should be a `config.json` file that the application is allowed to manage, which lives in the application data directory.
.../module/webservices/rest/web/v1_0/controller/openmrs1_9/FrontendJsonConfigController1_9.java
Outdated
Show resolved
Hide resolved
BufferedReader reader = request.getReader(); | ||
StringBuilder stringBuilder = new StringBuilder(); | ||
String line; | ||
while ((line = reader.readLine()) != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we have a library like IOUtils or any other to read this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And are we doing all this simply for the sake of validating that it is a JSON file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new ObjectMapper().readTree(requestBody);
this is the only line that validates the JSON file. Is it so expensive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then why is the entire method block surrounded with a try catch for JsonProcessingException
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then why is the entire method block surrounded with a try catch for
JsonProcessingException
?
I've made the try block specific to parsing
.../module/webservices/rest/web/v1_0/controller/openmrs1_9/FrontendJsonConfigController1_9.java
Outdated
Show resolved
Hide resolved
.../module/webservices/rest/web/v1_0/controller/openmrs1_9/FrontendJsonConfigController1_9.java
Outdated
Show resolved
Hide resolved
@jnsereko can you respond to all my comments? |
Sorry i had missed some of them. This PR has quite many comments. I hope i have addressed all of them now |
private void saveJsonConfigFile(HttpServletRequest request, HttpServletResponse response) throws IOException { | ||
File jsonConfigFile = getJsonConfigFile(); | ||
InputStream inputStream = request.getInputStream(); | ||
String requestBody = IOUtils.toString( inputStream , "UTF-8"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the above line also for validation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, this is just for reading the request body. Line 86 does the validation
private static final Logger log = LoggerFactory.getLogger(FrontendJsonConfigController1_9.class); | ||
|
||
@RequestMapping(method = RequestMethod.GET) | ||
public void getFrontendConfigFile(HttpServletRequest request, HttpServletResponse response) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's some inconsistency in spacing here. This line uses a tab character, but most lines are indented by spaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am wondering, what brought that tab, locally it doesn't exist. I will re-push
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IDEs have a nasty habit of doing this on auto-indent.
log.error("Error reading Configuration file: " + jsonConfigFile.getAbsolutePath(), e); | ||
response.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, "Error reading Configuration file: " + jsonConfigFile.getPath()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the other line I see indented by a tab.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as here #595 (comment)
response.setHeader("Content-Disposition", "attachment; filename=" + jsonConfigFile.getName()); | ||
response.setStatus(HttpServletResponse.SC_OK); | ||
} catch (IOException e) { | ||
log.error("Error reading Configuration file: " + jsonConfigFile.getAbsolutePath(), e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log.error("Error reading Configuration file: " + jsonConfigFile.getAbsolutePath(), e); | |
log.error("Error reading Configuration file {}", jsonConfigFile.getAbsolutePath(), e); |
OpenmrsUtil.copyFile(inputStream, outStream); | ||
|
||
response.setContentType("application/json"); | ||
response.setHeader("Content-Disposition", "attachment; filename=" + jsonConfigFile.getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding Content-Disposition: attachment
makes it impossible to use this on the frontend because you're not returning the JSON file as a REST response. You're returning a response instructing the user agent to download that as a file. Without some kind of endpoint that allows us to make a get request and get the JSON document back as part of the response without the Content-Disposition
we cannot make use of this configuration file. (I'd suggest adding an optional parameter, e.g., ?download
so that:
GET /openmrs/ws/rest/v1/frontend/config.json
returns like requesting any other JSON file and GET /openmrs/ws/rest/v1/frontend/config.json?download
sends it as an attachment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i have added the Content-Disposition: attachment
for both GET /openmrs/ws/rest/v1/frontend/config.json?download
and GET /openmrs/ws/rest/v1/frontend/config.json?download=true
. As an addition, i have Set their content type to application/x-download
Thank you for this clarification
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd keep the content type! It's still a JSON document.
public void getFrontendConfigFile(HttpServletRequest request, HttpServletResponse response) throws IOException { | ||
File jsonConfigFile = getJsonConfigFile(); | ||
if (!jsonConfigFile.exists()) { | ||
log.debug("Configuration file does not exist"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log.debug("Configuration file does not exist"); | |
log.warn("Configuration file does not exist"); |
// verify that is in a valid JSON format | ||
new ObjectMapper().readTree(requestBody); | ||
} catch (JsonProcessingException e) { | ||
log.error("Invalid JSON format", e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log.error("Invalid JSON format", e); | |
log.error("Invalid JSON format when reading: {}", requestBody, e); |
OutputStream outStream = Files.newOutputStream(jsonConfigFile.toPath()); | ||
OpenmrsUtil.copyFile(inputStream, outStream); | ||
|
||
if (jsonConfigFile.exists()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check is unnecessary. copyFile()
would've thrown an exception (which should potentially be handled here) if there was a failure.
try { | ||
// verify that is in a valid JSON format | ||
new ObjectMapper().readTree(requestBody); | ||
} catch (JsonProcessingException e) { | ||
log.error("Invalid JSON format", e); | ||
response.sendError(HttpServletResponse.SC_BAD_REQUEST, "Invalid JSON format"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try { | |
// verify that is in a valid JSON format | |
new ObjectMapper().readTree(requestBody); | |
} catch (JsonProcessingException e) { | |
log.error("Invalid JSON format", e); | |
response.sendError(HttpServletResponse.SC_BAD_REQUEST, "Invalid JSON format"); | |
} | |
try { | |
// verify that is in a valid JSON format | |
new ObjectMapper().readTree(requestBody); | |
} catch (JsonProcessingException e) { | |
log.error("Invalid JSON format", e); | |
response.sendError(HttpServletResponse.SC_BAD_REQUEST, "Invalid JSON format"); | |
} | |
if (jsonConfigFile.exists()) { | ||
log.debug("file: '{}' written successfully", jsonConfigFile.getAbsolutePath()); | ||
response.setStatus(HttpServletResponse.SC_OK); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never end a function like this with an if
without an else
. What response would the consumer get if the jsonConfigFile.exists()
check returned false
? (This is just intended as a pedagogic remark; as I said above, the entire if
is unnecessary
if (!folder.isDirectory()) { | ||
log.debug("Unable to find the OpenMRS SPA module frontend directory hence creating it at: " + folder.getAbsolutePath()); | ||
if (!folder.mkdirs()) { | ||
throw new IOException("Failed to create the OpenMRS SPA module frontend directory at: " + folder.getPath()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine, but:
- The error should be logged
- The consuming code should probably catch this exception and return something without the full path
It'd be great to have some tests for this. |
@jnsereko Do you think you could add some tests that verify that this works as expected? |
Description of what I changed
This is a continuation of openmrs/openmrs-module-spa#57 cc @dkayiwa
Issue I worked on
see https://issues.openmrs.org/browse/O3-310
Checklist: I completed these to help reviewers :)
My IDE is configured to follow the code style of this project.
No? Unsure? -> configure your IDE, format the code and add the changes with
git add . && git commit --amend
I have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)
No? -> write tests and add them to this commit
git add . && git commit --amend
I ran
mvn clean package
right before creating this pull request andadded all formatting changes to my commit.
No? -> execute above command
All new and existing tests passed.
No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
My pull request is based on the latest changes of the master branch.
No? Unsure? -> execute command
git pull --rebase upstream master