-
Notifications
You must be signed in to change notification settings - Fork 523
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?
Changes from 1 commit
a44a9d3
9c6d3ea
1abae4c
bf46700
6ffb3dd
a78f76d
b426de6
b6ef2eb
3abd011
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,136 @@ | ||||||
/** | ||||||
* This Source Code Form is subject to the terms of the Mozilla Public License, | ||||||
* v. 2.0. If a copy of the MPL was not distributed with this file, You can | ||||||
* obtain one at http://mozilla.org/MPL/2.0/. OpenMRS is also distributed under | ||||||
* the terms of the Healthcare Disclaimer located at http://openmrs.org/license. | ||||||
* | ||||||
* Copyright (C) OpenMRS Inc. OpenMRS is a registered trademark and the OpenMRS | ||||||
* graphic logo is a trademark of OpenMRS Inc. | ||||||
*/ | ||||||
package org.openmrs.module.webservices.rest.web.v1_0.controller.openmrs1_9; | ||||||
|
||||||
import java.io.BufferedReader; | ||||||
import java.io.ByteArrayInputStream; | ||||||
import java.io.IOException; | ||||||
import java.io.File; | ||||||
import java.io.FileInputStream; | ||||||
import java.io.InputStream; | ||||||
import java.io.OutputStream; | ||||||
import java.nio.file.Files; | ||||||
import java.nio.charset.StandardCharsets; | ||||||
import javax.servlet.http.HttpServletRequest; | ||||||
import javax.servlet.http.HttpServletResponse; | ||||||
|
||||||
import org.openmrs.User; | ||||||
import org.openmrs.api.AdministrationService; | ||||||
import org.openmrs.api.context.Context; | ||||||
import org.openmrs.module.webservices.rest.web.RestConstants; | ||||||
import org.openmrs.module.webservices.rest.web.v1_0.controller.BaseRestController; | ||||||
import org.openmrs.util.OpenmrsUtil; | ||||||
import org.slf4j.Logger; | ||||||
import org.slf4j.LoggerFactory; | ||||||
import org.springframework.http.HttpStatus; | ||||||
import org.springframework.stereotype.Controller; | ||||||
import org.springframework.web.bind.annotation.RequestMapping; | ||||||
import org.springframework.web.bind.annotation.RequestMethod; | ||||||
import org.springframework.web.bind.annotation.ResponseStatus; | ||||||
import org.codehaus.jackson.JsonProcessingException; | ||||||
import org.codehaus.jackson.map.ObjectMapper; | ||||||
|
||||||
@Controller | ||||||
@RequestMapping(value = "/rest/" + RestConstants.VERSION_1 + "/frontend/config.json") | ||||||
public class FrontendJsonConfigController1_9 extends BaseRestController { | ||||||
|
||||||
private static final String DEFAULT_FRONTEND_DIRECTORY = "frontend"; | ||||||
private static final String GP_LOCAL_DIRECTORY = "spa.local.directory"; | ||||||
private static final String JSON_CONFIG_FILE_NAME = "config.json"; | ||||||
|
||||||
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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. IDEs have a nasty habit of doing this on auto-indent. |
||||||
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 commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
response.sendError(HttpServletResponse.SC_BAD_REQUEST, "Configuration file does not exist"); | ||||||
jnsereko marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
} | ||||||
try { | ||||||
InputStream inputStream = new FileInputStream(jsonConfigFile); | ||||||
OutputStream outStream = response.getOutputStream(); | ||||||
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 commentThe reason will be displayed to describe this comment to others. Learn more. Adding
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i have added the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd keep the content type! It's still a JSON document. |
||||||
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 commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. same as here #595 (comment) |
||||||
|
||||||
@RequestMapping(method = RequestMethod.POST) | ||||||
@ResponseStatus(HttpStatus.OK) | ||||||
public void saveFrontendConfigFile(HttpServletRequest request, HttpServletResponse response) throws IOException { | ||||||
User user = Context.getAuthenticatedUser(); | ||||||
if (user == null || !user.isSuperUser()) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should probably invent a privilege for this purpose so we don't require people to be super users. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dkayiwa should i go ahead and create a ticket or talk post for this new requirement? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You do not have to, because that would be metadata which does not require a core change. |
||||||
log.error("Authorization error while creating a config.json file"); | ||||||
response.sendError(HttpServletResponse.SC_FORBIDDEN, "Authorization error while creating a config.json file"); | ||||||
jnsereko marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
return; | ||||||
} | ||||||
saveJsonConfigFile(request, response); | ||||||
} | ||||||
|
||||||
private void saveJsonConfigFile(HttpServletRequest request, HttpServletResponse response) throws IOException { | ||||||
File jsonConfigFile = getJsonConfigFile(); | ||||||
try { | ||||||
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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I've made the try block specific to parsing |
||||||
stringBuilder.append(line); | ||||||
} | ||||||
String requestBody = stringBuilder.toString(); | ||||||
|
||||||
// verify that is in a valid JSON format | ||||||
new ObjectMapper().readTree(requestBody); | ||||||
|
||||||
InputStream inputStream = new ByteArrayInputStream(requestBody.getBytes(StandardCharsets.UTF_8)); | ||||||
OutputStream outStream = Files.newOutputStream(jsonConfigFile.toPath()); | ||||||
OpenmrsUtil.copyFile(inputStream, outStream); | ||||||
|
||||||
if (jsonConfigFile.exists()) { | ||||||
log.debug("file: '{}' written successfully", jsonConfigFile.getAbsolutePath()); | ||||||
response.setStatus(HttpServletResponse.SC_OK); | ||||||
} | ||||||
} catch (JsonProcessingException e) { | ||||||
log.error("Invalid JSON format", e); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
response.sendError(HttpServletResponse.SC_BAD_REQUEST, "Invalid JSON format"); | ||||||
} | ||||||
} | ||||||
|
||||||
private File getJsonConfigFile() throws IOException { | ||||||
File folder = getSpaStaticFilesDir(); | ||||||
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 commentThe reason will be displayed to describe this comment to others. Learn more. This is fine, but:
|
||||||
} | ||||||
} | ||||||
return new File(folder.getAbsolutePath(), JSON_CONFIG_FILE_NAME); | ||||||
} | ||||||
|
||||||
private File getSpaStaticFilesDir() { | ||||||
AdministrationService as = Context.getAdministrationService(); | ||||||
String folderName = as.getGlobalProperty(GP_LOCAL_DIRECTORY, DEFAULT_FRONTEND_DIRECTORY); | ||||||
jnsereko marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
// try to load the repository folder straight away. | ||||||
File folder = new File(folderName); | ||||||
|
||||||
// if the property wasn't a full path already, assume it was intended to be a | ||||||
// folder in the application directory | ||||||
if (!folder.exists()) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Check |
||||||
folder = new File(OpenmrsUtil.getApplicationDataDirectory(), folderName); | ||||||
} | ||||||
return folder; | ||||||
} | ||||||
} |
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?