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.

Sign up for GitHub

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

Jump to bottom

Profile Manager Error Handling #14

Merged
merged 3 commits into from
Oct 25, 2015
Merged

Profile Manager Error Handling #14

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
43a6a05
In depth refactoring for better error handling and displaying on the …
llvasconcellos Oct 22, 2015
e343aa6
After some ajustments.
llvasconcellos Oct 23, 2015
c03e1a7
Rewieng some indentation problems and other minor fixes.
llvasconcellos Oct 25, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
155 changes: 103 additions & 52 deletions openmrs/omod/src/main/java/org/projectbuendia/openmrs/web/controller/ProfileManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,10 @@
import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
import org.openmrs.api.context.Context;
import org.openmrs.projectbuendia.webservices.rest.ConfigurationException;
import org.openmrs.projectbuendia.webservices.rest.GlobalProperties;
import org.springframework.stereotype.Controller;
import org.springframework.ui.ModelMap;
import org.springframework.web.bind.annotation.ModelAttribute;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RequestMethod;
import org.springframework.web.multipart.MultipartFile;
Expand All @@ -34,43 +34,78 @@
import java.nio.file.Files;
import java.nio.file.Paths;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.Comparator;
import java.util.Date;
import java.util.Iterator;
import java.util.List;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Properties;

import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

/** The controller for the profile management page. */
@Controller
public class ProfileManager {
static Log log = LogFactory.getLog(ProfileManager.class);
final File PROFILE_DIR = new File("/usr/share/buendia/profiles");
final String VALIDATE_CMD = "buendia-profile-validate";
final String APPLY_CMD = "buendia-profile-apply";
protected static Log log = LogFactory.getLog(ProfileManager.class);
private final String PROFILE_DIR_PATH = "/usr/share/buendia/profiles";
private final String VALIDATE_CMD = "buendia-profile-validate";
private final String APPLY_CMD = "buendia-profile-apply";
private File profileDir;

/** This is executed every time a request is made and determines the profileDir to be used. */
@ModelAttribute
public void getProfileDir() {
Properties config = Context.getRuntimeProperties();
String configProfileDir = config.getProperty("profile_manager.profile_dir", PROFILE_DIR_PATH);
profileDir = new File(configProfileDir);
}

@RequestMapping(value = "/module/projectbuendia/openmrs/profiles", method = RequestMethod.GET)
public void get(HttpServletRequest request, ModelMap model) {
List<FileInfo> files = new ArrayList<>();
for (File file : PROFILE_DIR.listFiles()) {
files.add(new FileInfo(file));
String currentProfile = getCurrentProfile();
model.addAttribute("currentProfile", currentProfile);
model.addAttribute("authorized", authorized());
model = queryParamsToModelAttr(request, model);
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this snippet does ? It would be more clear if you comment it.

        String currentProfile = Context.getAdministrationService().getGlobalProperty(GlobalProperties.CURRENT_PROFILE);
        model.addAttribute("currentProfile", currentProfile);
        model.addAttribute("authorized", authorized());
        model = queryParamsToModelAttr(request, model);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's very clear to me what it does and it wasn't me who did it. I just rearrange the sequence. I'll leave like this.

if (!profileDir.exists()) {
model.addAttribute("success", false);
model.addAttribute("message", "The Profile Manager directory does not exist. It must reside on " +
"/usr/share/buendia/profiles or on a directory specified in openmrs-runtime.properties " +
"file under profile_manager.profile_dir directive.");
} else {
model.addAttribute("profiles", listProfiles());
}
Collections.sort(files, new Comparator<FileInfo>() {
public int compare(FileInfo a, FileInfo b) {
return -a.modified.compareTo(b.modified);
}
});
}

model.addAttribute("user", Context.getAuthenticatedUser());
model.addAttribute("profiles", files);
model.addAttribute("currentProfile",
Context.getAdministrationService().getGlobalProperty(
GlobalProperties.CURRENT_PROFILE));
model.addAttribute("authorized", authorized());
/** This method sends data to jsp in a more consistent way. */
private ModelMap queryParamsToModelAttr(HttpServletRequest request, ModelMap model) {
Copy link
Contributor

Choose a reason for hiding this comment

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

private methods should be after the publics.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disagree on this one. Think methods are best placed next to where it is used.

Map params = request.getParameterMap();
Copy link
Contributor

Choose a reason for hiding this comment

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

A for loop does the job nicely here, and using the parameterized type Map<String, String[]> gives better type safety:

        for (Map.Entry<String, String[]> entry : params.entrySet()) {
            model.addAttribute(entry.getKey(), entry.getValue()[0]);
        }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I definitely tried that but this does not work. entrySet returns a Set.
So I thought that the while was more straightforward.

Iterator paramIterator = params.entrySet().iterator();
while (paramIterator.hasNext()) {
Entry param = (Entry) paramIterator.next();
String key = (String) param.getKey();
String[] value = (String[]) param.getValue();
model.addAttribute(key, value[0]);
}
return model;
}

private List<FileInfo> listProfiles() {
Copy link
Contributor

Choose a reason for hiding this comment

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

private methods should be after the publics.

List<FileInfo> files = new ArrayList<>();
File[] profiles = profileDir.listFiles();
if (profiles != null) {
for (File file : profiles) {
files.add(new FileInfo(file));
}
Collections.sort(files, new Comparator<FileInfo>() {
public int compare(FileInfo a, FileInfo b) {
return -a.modified.compareTo(b.modified);
}
});
}
return files;
}

public static boolean authorized() {
Expand All @@ -90,7 +125,7 @@ public View post(HttpServletRequest request, HttpServletResponse response, Model
String filename = request.getParameter("profile");
String op = request.getParameter("op");
if (filename != null) {
File file = new File(PROFILE_DIR, filename);
File file = new File(profileDir, filename);
if (file.isFile()) {
model.addAttribute("filename", filename);
if ("Apply".equals(op)) {
Expand All @@ -108,7 +143,7 @@ public View post(HttpServletRequest request, HttpServletResponse response, Model
}

/** Chooses a filename based on the given name, with a "-vN" suffix appended for uniqueness. */
String getNextVersionedFilename(String name) {
private String getNextVersionedFilename(String name) {
Copy link
Contributor

Choose a reason for hiding this comment

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

private methods should be after public ones.

// Separate the name into a sanitized base name and an extension.
name = sanitizeName(name);
String ext = "";
Expand All @@ -122,7 +157,7 @@ String getNextVersionedFilename(String name) {
// If "name.ext" exists, it counts as version 1.
String prefix = name + "-v";
int highestVersion = 0;
for (File file : PROFILE_DIR.listFiles()) {
for (File file : profileDir.listFiles()) {
int version = 0;
String n = file.getName();
if (n.equals(name + ext)) {
Expand All @@ -145,47 +180,56 @@ String getNextVersionedFilename(String name) {
}

/** Handles an uploaded profile. */
// TODO: Give some error message if the profile is not uploaded correctly
// (When tomcat7 didn't have permission to PROFILES_DIR it was failing
// without showing any error message to the user)
void addProfile(MultipartHttpServletRequest request, ModelMap model) {
private void addProfile(MultipartHttpServletRequest request, ModelMap model) {
List<String> lines = new ArrayList<>();
MultipartFile mpf = request.getFile("file");
if (mpf != null) {
String message = "";
String filename = mpf.getOriginalFilename();
try {
File tempFile = File.createTempFile("profile", null);
mpf.transferTo(tempFile);
if (execute(VALIDATE_CMD, tempFile, lines)) {
String filename = getNextVersionedFilename(mpf.getOriginalFilename());
File newFile = new File(PROFILE_DIR, filename);
model.addAttribute("filename", filename);
boolean exResult = execute(VALIDATE_CMD, tempFile, lines);
Copy link
Contributor

Choose a reason for hiding this comment

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

exResult is not used anywhere else, so it is better to write

    if (execute(...)) {
        ...
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I did this on purpose. I think it makes code more clear and readable.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @llvasconcellos. It makes the code more clear.
In my opinion, we should keep if clauses side-effect free.
The only function of an if statement is to test whether a condition is true.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay. I guess I'll provide my reasoning for why I think it's not a good way to spend a line of code.

  1. It makes the code harder to read because you have to say in your mind "We are going to execute(...) and store the success flag in exResult, and then we are going to test exResult, and if it is true then..." instead of "If execute(...) succeeds then..." That's a lot more thinking.
  2. It makes the code harder to change because if you edit the execute(...) call, you don't know whether, by changing the meaning of exResult, you have affected the behaviour of the program many lines down below at a later point where it happens to use exResult again. (Or, in other words, the variable exResult has excessively large scope.) If you write the expression without exResult, it is obvious that editing the execute(...) call affects this one if statement and cannot affect anything else.
  3. Being able to tell whether a line of code has a side effect is important, I agree. But that is not the responsibility of the if statement; it is the responsibility of the method name, execute, which is pretty clear that it has a side effect.

I don't think it's a really big deal in this case, but I did want to at least share my thinking with you so you understand where I'm coming from and have a chance to contemplate the benefits of the other way as well.

if (exResult) {
filename = getNextVersionedFilename(filename);
File newFile = new File(profileDir, filename);
FileUtils.moveFile(tempFile, newFile);
model.addAttribute("success", "add");
model.addAttribute("success", true);
message = "Success adding profile: ";
} else {
model.addAttribute("failure", "add");
model.addAttribute("filename", mpf.getOriginalFilename());
model.addAttribute("output", StringUtils.join(lines, "\n"));
model.addAttribute("success", false);
message = "Error adding profile: ";
}
} catch (Exception e) {
model.addAttribute("success", false);
message = "Problem saving uploaded profile: ";
lines.add(e.getMessage());
log.error("Problem saving uploaded profile", e);
} finally {
model.addAttribute("operation", "add");
model.addAttribute("message", message + filename);
model.addAttribute("filename", filename);
model.addAttribute("output", StringUtils.join(lines, "\n"));
}
}
}

/** Applies a profile to the OpenMRS database. */
void applyProfile(File file, ModelMap model) {
private void applyProfile(File file, ModelMap model) {
List<String> lines = new ArrayList<>();
if (execute(APPLY_CMD, file, lines)) {
setCurrentProfile(file.getName());
model.addAttribute("success", "apply");
model.addAttribute("success", true);
model.addAttribute("message", "Success applying profile: " + file.getName());
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to i12n the message or to put a TODO to do it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The entire OpenMRS side of things aren't localized so I don't think it applies here.

} else {
model.addAttribute("failure", "apply");
model.addAttribute("success", false);
model.addAttribute("message", "Error applying profile: " + file.getName());
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to i12n the message or to put a TODO to do it later.

model.addAttribute("output", StringUtils.join(lines, "\n"));
}
}

/** Downloads a profile. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Though you didn't change this line, i would delete it. It's useless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm leaving to keep comments consistency.

public void downloadProfile(File file, HttpServletResponse response) {
private void downloadProfile(File file, HttpServletResponse response) {
response.setContentType("application/octet-stream");
response.setHeader(
"Content-Disposition", "attachment; filename=\"" + file.getName() + "\"");
Expand All @@ -198,23 +242,25 @@ public void downloadProfile(File file, HttpServletResponse response) {
}

/** Deletes a profile. */
Copy link
Contributor

Choose a reason for hiding this comment

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

useless comment, though it was already here.

void deleteProfile(File file, ModelMap model) {
private void deleteProfile(File file, ModelMap model) {
if (file.getName().equals(getCurrentProfile())) {
model.addAttribute("failure", "delete");
model.addAttribute("output", "Cannot delete the currently active profile.");
model.addAttribute("success", false);
model.addAttribute("message", "Cannot delete the currently active profile.");
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to i12n the message or to put a TODO to do it later.

} else if (file.delete()) {
model.addAttribute("success", "delete");
model.addAttribute("success", true);
model.addAttribute("message", "Success deleting profile: " + file.getName());
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to i12n the message or to put a TODO to do it later.

} else {
model.addAttribute("success", false);
model.addAttribute("message", "Error deleting profile: " + file.getName());
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to i12n the message or to put a TODO to do it later.

log.error("Error deleting profile: " + file.getName());
model.addAttribute("failure", "delete");
}
}

/**
* Executes a command with one argument, returning true if the command succeeds.
* Gathers the output from stdout and stderr into the provided list of lines.
*/
boolean execute(String command, File arg, List<String> lines) {
private boolean execute(String command, File arg, List<String> lines) {
ProcessBuilder pb = new ProcessBuilder(command, arg.getAbsolutePath());
pb.redirectErrorStream(true); // redirect stderr to stdout
try {
Expand All @@ -229,24 +275,25 @@ boolean execute(String command, File arg, List<String> lines) {
return proc.exitValue() == 0;
} catch (Exception e) {
log.error("Exception while executing: " + command + " " + arg, e);
lines.add(e.getMessage());
return false;
}
}

/** Sanitizes a string to produce a safe filename. */
String sanitizeName(String filename) {
private String sanitizeName(String filename) {
String[] parts = filename.split("/");
return parts[parts.length - 1].replaceAll("[^A-Za-z0-9._-]", " ").replaceAll(" +", " ");
}

/** Gets the global property for the name of the current profile. */
String getCurrentProfile() {
private String getCurrentProfile() {
return Context.getAdministrationService().getGlobalProperty(
GlobalProperties.CURRENT_PROFILE);
}

/** Sets the global property for the name of the current profile. */
void setCurrentProfile(String name) {
private void setCurrentProfile(String name) {
Context.getAdministrationService().setGlobalProperty(
GlobalProperties.CURRENT_PROFILE, name);
}
Expand All @@ -268,10 +315,14 @@ public String getName() {
public String getFormattedName() {
return (name + " ").substring(0, 30);
}
public Long getSize() { return size; }
public Long getSize() {
return size;
}
public String getFormattedSize() {
return String.format("%7d", size);
}
public Date getModified() { return modified; }
public Date getModified() {
return modified;
}
}
}
Loading