Skip to content
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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
/**
* 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.apache.commons.io.IOUtils;
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)
Copy link
Member

@dkayiwa dkayiwa Jan 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The formatting here looks ... 😊

Copy link
Member

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?

Copy link
Author

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And the indention?

public void getFrontendConfigFile(HttpServletRequest request, HttpServletResponse response) throws IOException {
Copy link
Member

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.

Copy link
Author

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

Copy link
Member

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.

File jsonConfigFile = getJsonConfigFile();
if (!jsonConfigFile.exists()) {
log.debug("Configuration file does not exist");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
log.debug("Configuration file does not exist");
log.warn("Configuration file does not exist");

response.sendError(HttpServletResponse.SC_NOT_FOUND, "Configuration file does not exist");
}
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());
Copy link
Member

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.

Copy link
Author

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

Copy link
Member

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.

response.setStatus(HttpServletResponse.SC_OK);
} catch (IOException e) {
log.error("Error reading Configuration file: " + jsonConfigFile.getAbsolutePath(), e);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
log.error("Error reading Configuration file: " + jsonConfigFile.getAbsolutePath(), e);
log.error("Error reading Configuration file {}", jsonConfigFile.getAbsolutePath(), e);

response.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, "Error reading Configuration file: " + jsonConfigFile.getPath());
}
}
Copy link
Member

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.

Copy link
Author

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)


@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()) {
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Author

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

The 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. Admin privileges required to save the config.json file");
return;
}
saveJsonConfigFile(request, response);
}

private void saveJsonConfigFile(HttpServletRequest request, HttpServletResponse response) throws IOException {
File jsonConfigFile = getJsonConfigFile();
InputStream inputStream = request.getInputStream();
String requestBody = IOUtils.toString( inputStream , "UTF-8");
Copy link
Member

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?

Copy link
Author

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
String requestBody = IOUtils.toString( inputStream , "UTF-8");
String requestBody = IOUtils.toString(inputStream , "UTF-8");

try {
// verify that is in a valid JSON format
new ObjectMapper().readTree(requestBody);
} catch (JsonProcessingException e) {
log.error("Invalid JSON format", e);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
log.error("Invalid JSON format", e);
log.error("Invalid JSON format when reading: {}", requestBody, e);

response.sendError(HttpServletResponse.SC_BAD_REQUEST, "Invalid JSON format");
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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");
}

inputStream = new ByteArrayInputStream(requestBody.getBytes(StandardCharsets.UTF_8));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we just use the requestBody string we already have?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not so sure if i get you correctly but i was advised to use OpenMRSUtil's copyFile

OutputStream outStream = Files.newOutputStream(jsonConfigFile.toPath());
OpenmrsUtil.copyFile(inputStream, outStream);

if (jsonConfigFile.exists()) {
Copy link
Member

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.

log.debug("file: '{}' written successfully", jsonConfigFile.getAbsolutePath());
response.setStatus(HttpServletResponse.SC_OK);
}
Copy link
Member

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

}

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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine, but:

  1. The error should be logged
  2. The consuming code should probably catch this exception and return something without the full path

}
}
return new File(folder.getAbsolutePath(), JSON_CONFIG_FILE_NAME);
}

private File getSpaStaticFilesDir() {
String folderName = Context.getAdministrationService().getGlobalProperty(GP_LOCAL_DIRECTORY, DEFAULT_FRONTEND_DIRECTORY);

// 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()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check folder.isAbsolute() rather than folder.exists() for this.

folder = new File(OpenmrsUtil.getApplicationDataDirectory(), folderName);
}
return folder;
}
}
Loading