Skip to content

Make ApiUsageException return a more appropriate response code #4396

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

Draft
wants to merge 7 commits into
base: develop
Choose a base branch
from
Draft

Make ApiUsageException return a more appropriate response code #4396

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9d04973
Make `ApiUsageException` return a more appropriate response code
labkey-tchad May 5, 2023
e94eb3e
Make `ApiUsageException` extend BadRequestException
labkey-tchad May 5, 2023
f0f836a
Merge remote-tracking branch 'origin/develop' into fb_apiUsage422
labkey-tchad May 5, 2023
1d2331f
Merge remote-tracking branch 'origin/develop' into fb_apiUsage422
labkey-tchad May 24, 2023
3994004
Use HttpStatus constants instead of integers
labkey-tchad May 25, 2023
160f37a
GWT class can't reference HttpStatus
labkey-tchad May 25, 2023
ec08035
Merge remote-tracking branch 'origin/develop' into fb_apiUsage422
labkey-tchad May 30, 2023
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
6 changes: 3 additions & 3 deletions api/gwtsrc/org/labkey/api/gwt/client/util/ErrorDialogAsyncCallback.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,13 +67,13 @@ public final void onFailure(Throwable caught)
// Indicates the request was cancelled because the user navigated to another page
// Don't bother showing any dialog at all
return;
case 401:
case 403: // HttpStatus.SC_UNAUTHORIZED
message = "You do not have permission to perform this operation. Your session may have expired.";
break;
case 404:
case 404: // HttpStatus.SC_NOT_FOUND
message = "Not found.";
break;
case 500:
case 500: // HttpStatus.SC_INTERNAL_SERVER_ERROR
message = "The server encountered an error";
if(statusCodeException.getMessage() != null)
message += ": " + statusCodeException.getMessage();
Expand Down
18 changes: 9 additions & 9 deletions api/src/org/labkey/api/action/AbstractFileUploadAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/
package org.labkey.api.action;

import org.apache.hc.core5.http.HttpStatus;
import org.labkey.api.util.ExceptionUtil;
import org.labkey.api.util.FileUtil;
import org.labkey.api.util.PageFlowUtil;
Expand Down Expand Up @@ -158,19 +159,19 @@ private void export(FORM form, HttpServletResponse response) throws Exception
{
if (form.getFileName() == null)
{
error(writer, "No fileName parameter values included", HttpServletResponse.SC_BAD_REQUEST);
error(writer, "No fileName parameter values included", HttpStatus.SC_UNPROCESSABLE_ENTITY);
return;
}

if (form.getFileContent() == null)
{
error(writer, "No fileContent parameter values included", HttpServletResponse.SC_BAD_REQUEST);
error(writer, "No fileContent parameter values included", HttpStatus.SC_UNPROCESSABLE_ENTITY);
return;
}

if (form.getFileName().length != form.getFileContent().length)
{
error(writer, "Must include the same number of fileName and fileContent parameter values", HttpServletResponse.SC_BAD_REQUEST);
error(writer, "Must include the same number of fileName and fileContent parameter values", HttpStatus.SC_UNPROCESSABLE_ENTITY);
return;
}

Expand All @@ -179,9 +180,8 @@ private void export(FORM form, HttpServletResponse response) throws Exception
// Parameter name (String) -> File on disk/original file name Pair
Map<String, Pair<File, String>> savedFiles = new HashMap<>();

if (basicRequest instanceof MultipartHttpServletRequest)
if (basicRequest instanceof MultipartHttpServletRequest request)
{
MultipartHttpServletRequest request = (MultipartHttpServletRequest) basicRequest;

Iterator<String> nameIterator = request.getFileNames();
while (nameIterator.hasNext())
Expand Down Expand Up @@ -226,7 +226,7 @@ private void export(FORM form, HttpServletResponse response) throws Exception
}
catch (UploadException e)
{
error(writer, "Must include the same number of fileName and fileContent parameter values", HttpServletResponse.SC_BAD_REQUEST);
error(writer, "Must include the same number of fileName and fileContent parameter values", e.getStatusCode());
}
}
}
Expand All @@ -235,7 +235,7 @@ protected File handleFile(String filename, InputStream input, Writer writer) thr
{
if (filename == null || input == null)
{
error(writer, "No file uploaded, or no filename specified", HttpServletResponse.SC_BAD_REQUEST);
error(writer, "No file uploaded, or no filename specified", HttpStatus.SC_UNPROCESSABLE_ENTITY);
return null;
}

Expand Down Expand Up @@ -266,14 +266,14 @@ protected File handleFile(String filename, InputStream input, Writer writer) thr
}
catch (UploadException e)
{
error(writer, e.getMessage(), HttpServletResponse.SC_BAD_REQUEST);
error(writer, e.getMessage(), e.getStatusCode());
return null;
}
}

public static class UploadException extends IOException
{
private int _statusCode;
private final int _statusCode;

public UploadException(String message, int statusCode)
{
Expand Down
3 changes: 2 additions & 1 deletion api/src/org/labkey/api/action/ApiResponseWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package org.labkey.api.action;

import com.fasterxml.jackson.databind.ObjectMapper;
import org.apache.hc.core5.http.HttpStatus;
import org.apache.logging.log4j.LogManager;
import org.json.JSONArray;
import org.json.JSONObject;
Expand Down Expand Up @@ -60,7 +61,7 @@ public abstract class ApiResponseWriter implements AutoCloseable
*
* Allow new code to specify that SC_OK should be used for errors
*/
static final int defaultErrorStatus = HttpServletResponse.SC_BAD_REQUEST;
static final int defaultErrorStatus = HttpStatus.SC_BAD_REQUEST;
Integer errorResponseStatus = null;

private boolean serializeViaJacksonAnnotations = false;
Expand Down
19 changes: 7 additions & 12 deletions api/src/org/labkey/api/action/ApiUsageException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,33 +15,28 @@
*/
package org.labkey.api.action;

import org.apache.hc.core5.http.HttpStatus;
import org.labkey.api.util.SkipMothershipLogging;
import org.labkey.api.view.BadRequestException;

/**
* Signals the client API caller that they somehow made an invalid request. These errors are not reported to the
* mothership.
* User: jeckels
* Date: Oct 5, 2010
*/
public class ApiUsageException extends RuntimeException implements SkipMothershipLogging
public class ApiUsageException extends BadRequestException implements SkipMothershipLogging
Copy link
Contributor

Choose a reason for hiding this comment

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

I think defaulting to a 422 seems best, and cases that should return a 404 can be switched to use NotFoundException.

Copy link
Contributor

Choose a reason for hiding this comment

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

What a mess. A lot of usages of ApiUsageException do indeed seem equally or better served by NotFoundException or ValidationException.

I like the move to 4xx responses. I do not like using 400 BAD_REQUEST for any request that is basically well formatted but we can't handle for an explainable reason (e.g. Validation, NotFound, Unauthorized).

Separately, but not a topic for today. If I could design this all over again, I would use 200 {success:false} for most of our error cases and reserve non 200 responses for actual protocol errors (e.g. this isn't an legal end point so NOT_FOUND, rather than I can't find that rowid).

Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree. I think we should try to respond with the appropriate HTTP codes as intended. For example, 404:

image

404 doesn't indicate that the end point is illegal but rather that the resource was not found. Not finding the thing with "rowId" is precisely the case it is intended to cover.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for taking a look at this. When I have some spare cycles, I'll take a crack at switching ApiUsageException to return a 422 and switching around some inappropriate usages.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit ambivalent about the 404 case. I still think that generally any interaction where "I got your request", "I understood your request", "Here's my response to your request" doesn't require out of band error handling.

Copy link
Contributor

Choose a reason for hiding this comment

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

Certainly agree that the "out of band error handling" is not the ideal way to process this kind of thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

The latest wording for a 400 error is a little less specific to malformed requests: something that is perceived to be a client error -- compared to the original wording: The request could not be understood by the server due to malformed syntax.
422 was added as a WebDAV specific response code but has since been pulled into the core HTTP standard.

That being said, I'm inclined to move forward with 422 for ApiUsageException. That's what the GitHub API returns for requests that it understands but have some semantic error (e.g. missing a required field).

{
public ApiUsageException()
public ApiUsageException(String message, Throwable cause)
{
super();
super(message, cause, HttpStatus.SC_UNPROCESSABLE_ENTITY, HowBad.MaybeBad);
}

public ApiUsageException(String message)
{
super(message);
}

public ApiUsageException(String message, Throwable cause)
{
super(message, cause);
this(message, null);
}

public ApiUsageException(Throwable cause)
{
super(cause.getMessage() == null ? cause.toString() : cause.getMessage(), cause);
this(cause.getMessage(), cause);
}
}
3 changes: 2 additions & 1 deletion api/src/org/labkey/api/security/AuthFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import org.apache.commons.collections4.IteratorUtils;
import org.apache.commons.lang3.StringUtils;
import org.apache.hc.core5.http.HttpStatus;
import org.labkey.api.module.ModuleLoader;
import org.labkey.api.module.SafeFlushResponseWrapper;
import org.labkey.api.query.QueryService;
Expand Down Expand Up @@ -189,7 +190,7 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
}
catch (UnsupportedEncodingException uee)
{
resp.sendError(HttpServletResponse.SC_BAD_REQUEST, uee.getMessage());
resp.sendError(HttpStatus.SC_BAD_REQUEST, uee.getMessage());
return;
}

Expand Down
2 changes: 1 addition & 1 deletion api/src/org/labkey/api/util/ExceptionUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -772,7 +772,7 @@ static ActionURL handleException(@NotNull HttpServletRequest request, @NotNull H
}
else if (ex instanceof ApiUsageException)
{
responseStatus = HttpServletResponse.SC_BAD_REQUEST;
responseStatus = ((ApiUsageException) ex).getStatus();
errorType = ErrorRenderer.ErrorType.notFound;
if (ex.getMessage() != null)
{
Expand Down
18 changes: 9 additions & 9 deletions api/src/org/labkey/api/view/BadRequestException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@
package org.labkey.api.view;

import org.apache.commons.lang3.StringUtils;
import org.apache.hc.core5.http.HttpStatus;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;

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

/**
* Indicates that the client made a bad HTTP request, typically resulting in a 400 HTTP response code and avoiding much
Expand Down Expand Up @@ -57,7 +57,7 @@ public boolean isSuspiciousRequest(HttpServletRequest req, boolean isSuspicious)
@Override
public boolean isSuspiciousRequest(HttpServletRequest req, boolean isSuspicious)
{
return true;
return false;
}
};

Expand All @@ -73,25 +73,25 @@ public BadRequestException()

public BadRequestException(String message)
{
this(message, null, HttpServletResponse.SC_BAD_REQUEST, HowBad.MaybeBad);
this(message, null, HttpStatus.SC_BAD_REQUEST, HowBad.MaybeBad);
}

public BadRequestException(String message, @Nullable Exception x)
public BadRequestException(String message, @Nullable Throwable x)
{
this(message, x, HttpServletResponse.SC_BAD_REQUEST, HowBad.MaybeBad);
this(message, x, HttpStatus.SC_BAD_REQUEST, HowBad.MaybeBad);
}

public BadRequestException(String message, @Nullable Exception x, int httpStatusCode)
public BadRequestException(String message, @Nullable Throwable x, int httpStatusCode)
{
this(message, x, httpStatusCode, HttpServletResponse.SC_METHOD_NOT_ALLOWED == httpStatusCode ? HowBad.Malicious : HowBad.MaybeBad);
this(message, x, httpStatusCode, HttpStatus.SC_METHOD_NOT_ALLOWED == httpStatusCode ? HowBad.Malicious : HowBad.MaybeBad);
}

public BadRequestException(String message, @NotNull HowBad severity)
{
this(message, null, HttpServletResponse.SC_BAD_REQUEST, severity);
this(message, null, HttpStatus.SC_BAD_REQUEST, severity);
}

BadRequestException(String message, @Nullable Exception x, int httpStatusCode, HowBad severity)
protected BadRequestException(String message, @Nullable Throwable x, int httpStatusCode, HowBad severity)
{
super(StringUtils.defaultIfEmpty(message, "BAD REQUEST"), x, httpStatusCode);
this.severity = severity;
Expand Down
4 changes: 2 additions & 2 deletions core/src/org/labkey/core/security/BlockListFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,9 @@ static void registerBadRequest(HttpServletRequest req)
static void handleBadRequest(HttpServletRequest req)
{
Object ex = req.getAttribute(ExceptionUtil.REQUEST_EXCEPTION_ATTRIBUTE);
if (ex instanceof BadRequestException)
if (ex instanceof BadRequestException badRequestException)
{
if (!((BadRequestException)ex).isSuspiciousRequest(req, isSuspicious(req.getRequestURI(),req.getQueryString(),req.getHeader("User-Agent"))))
if (!badRequestException.isSuspiciousRequest(req, isSuspicious(req.getRequestURI(),req.getQueryString(),req.getHeader("User-Agent"))))
return;
}
registerBadRequest(req);
Expand Down
3 changes: 2 additions & 1 deletion core/src/org/labkey/core/security/SecurityPointcutServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/
package org.labkey.core.security;

import org.apache.hc.core5.http.HttpStatus;
import org.labkey.api.module.Module;
import org.labkey.api.security.SecurityPointcutService;
import org.labkey.api.settings.AppProps;
Expand Down Expand Up @@ -69,7 +70,7 @@ else if (res.getStatus() == SC_UNAUTHORIZED || res.getStatus() == SC_FORBIDDEN)
if (ex instanceof CSRFException)
BlockListFilter.handleBadRequest(req);
}
else if (res.getStatus() == SC_BAD_REQUEST)
else if (res.getStatus() == SC_BAD_REQUEST || res.getStatus() == HttpStatus.SC_UNPROCESSABLE_ENTITY)
{
BlockListFilter.handleBadRequest(req);
}
Expand Down
5 changes: 3 additions & 2 deletions experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import au.com.bytecode.opencsv.CSVWriter;
import org.apache.commons.lang3.StringUtils;
import org.apache.hc.core5.http.HttpStatus;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.poi.openxml4j.exceptions.InvalidFormatException;
Expand Down Expand Up @@ -2563,7 +2564,7 @@ public void export(ConvertArraysToExcelForm form, HttpServletResponse response,
catch (JSONException | ClassCastException e)
{
// We can get a ClassCastException if we expect an array and get a simple String, for example
ExceptionUtil.renderErrorView(getViewContext(), getPageConfig(), ErrorRenderer.ErrorType.notFound, HttpServletResponse.SC_BAD_REQUEST, "Failed to convert to Excel - invalid input", e, false, false);
ExceptionUtil.renderErrorView(getViewContext(), getPageConfig(), ErrorRenderer.ErrorType.notFound, HttpStatus.SC_UNPROCESSABLE_ENTITY, "Failed to convert to Excel - invalid input", e, false, false);
}
}
}
Expand Down Expand Up @@ -2642,7 +2643,7 @@ public void export(ConvertArraysToExcelForm form, HttpServletResponse response,
}
catch (JSONException e)
{
ExceptionUtil.renderErrorView(getViewContext(), getPageConfig(), ErrorRenderer.ErrorType.notFound, HttpServletResponse.SC_BAD_REQUEST, "Failed to convert to table - invalid input", e, false, false);
ExceptionUtil.renderErrorView(getViewContext(), getPageConfig(), ErrorRenderer.ErrorType.notFound, HttpStatus.SC_UNPROCESSABLE_ENTITY, "Failed to convert to table - invalid input", e, false, false);
}
}
}
Expand Down
5 changes: 3 additions & 2 deletions experiment/src/org/labkey/experiment/controllers/property/PropertyController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import com.fasterxml.jackson.databind.ser.impl.SimpleBeanPropertyFilter;
import org.apache.commons.beanutils.ConvertUtils;
import org.apache.commons.lang3.StringUtils;
import org.apache.hc.core5.http.HttpStatus;
import org.apache.poi.util.IOUtils;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
Expand Down Expand Up @@ -1251,7 +1252,7 @@ public String getResponse(FileUploadForm form, Map<String, Pair<File, String>> f
{
if (files.isEmpty())
{
throw new UploadException("No file(s) uploaded, or the uploaded file was empty", 400);
throw new UploadException("No file(s) uploaded, or the uploaded file was empty", HttpStatus.SC_BAD_REQUEST);
}
if (files.size() > 1)
{
Expand All @@ -1263,7 +1264,7 @@ public String getResponse(FileUploadForm form, Map<String, Pair<File, String>> f
separator = ", ";
message.append(fileStringPair.getValue());
}
throw new UploadException("Only one file is supported, but " + files.size() + " were uploaded: " + message, 400);
throw new UploadException("Only one file is supported, but " + files.size() + " were uploaded: " + message, HttpStatus.SC_BAD_REQUEST);
}
// Store the file in the session, and delete it when the session expires
HttpSession session = getViewContext().getSession();
Expand Down