From 86cae53789140f688e2ae7cb5c869c09677083de Mon Sep 17 00:00:00 2001 From: jendib Date: Wed, 26 Aug 2015 22:11:39 +0200 Subject: [PATCH] Closes #20: Clean error message if document or file does not exist --- .../docs/rest/resource/AuditLogResource.java | 3 ++- .../docs/rest/resource/DocumentResource.java | 7 ++++--- .../docs/rest/resource/FileResource.java | 20 ++++++++----------- .../src/app/docs/controller/DocumentView.js | 2 ++ .../webapp/src/app/docs/directive/ImgError.js | 16 +++++++++++++++ docs-web/src/main/webapp/src/index.html | 1 + .../src/partial/docs/document.view.html | 9 ++++++++- .../webapp/src/partial/docs/file.view.html | 9 ++++++++- .../docs/rest/TestDocumentResource.java | 3 +-- 9 files changed, 50 insertions(+), 20 deletions(-) create mode 100644 docs-web/src/main/webapp/src/app/docs/directive/ImgError.js diff --git a/docs-web/src/main/java/com/sismics/docs/rest/resource/AuditLogResource.java b/docs-web/src/main/java/com/sismics/docs/rest/resource/AuditLogResource.java index 3b431a95..b87c3b7e 100644 --- a/docs-web/src/main/java/com/sismics/docs/rest/resource/AuditLogResource.java +++ b/docs-web/src/main/java/com/sismics/docs/rest/resource/AuditLogResource.java @@ -9,6 +9,7 @@ import javax.ws.rs.Produces; import javax.ws.rs.QueryParam; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; +import javax.ws.rs.core.Response.Status; import org.codehaus.jettison.json.JSONException; import org.codehaus.jettison.json.JSONObject; @@ -55,7 +56,7 @@ public class AuditLogResource extends BaseResource { // Check ACL on the document AclDao aclDao = new AclDao(); if (!aclDao.checkPermission(documentId, PermType.READ, principal.getId())) { - throw new ForbiddenClientException(); + return Response.status(Status.NOT_FOUND).build(); } criteria.setDocumentId(documentId); } diff --git a/docs-web/src/main/java/com/sismics/docs/rest/resource/DocumentResource.java b/docs-web/src/main/java/com/sismics/docs/rest/resource/DocumentResource.java index c276e44a..a0c8ba44 100644 --- a/docs-web/src/main/java/com/sismics/docs/rest/resource/DocumentResource.java +++ b/docs-web/src/main/java/com/sismics/docs/rest/resource/DocumentResource.java @@ -20,6 +20,7 @@ import javax.ws.rs.Produces; import javax.ws.rs.QueryParam; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; +import javax.ws.rs.core.Response.Status; import org.apache.commons.lang.StringUtils; import org.codehaus.jettison.json.JSONException; @@ -92,7 +93,7 @@ public class DocumentResource extends BaseResource { throw new ForbiddenClientException(); } } catch (NoResultException e) { - throw new ClientException("DocumentNotFound", MessageFormat.format("Document not found: {0}", documentId)); + return Response.status(Status.NOT_FOUND).build(); } JSONObject document = new JSONObject(); @@ -430,7 +431,7 @@ public class DocumentResource extends BaseResource { try { document = documentDao.getDocument(id, PermType.WRITE, principal.getId()); } catch (NoResultException e) { - throw new ClientException("DocumentNotFound", MessageFormat.format("Document not found: {0}", id)); + return Response.status(Status.NOT_FOUND).build(); } // Update the document @@ -514,7 +515,7 @@ public class DocumentResource extends BaseResource { document = documentDao.getDocument(id, PermType.WRITE, principal.getId()); fileList = fileDao.getByDocumentId(principal.getId(), id); } catch (NoResultException e) { - throw new ClientException("DocumentNotFound", MessageFormat.format("Document not found: {0}", id)); + return Response.status(Status.NOT_FOUND).build(); } // Delete the document diff --git a/docs-web/src/main/java/com/sismics/docs/rest/resource/FileResource.java b/docs-web/src/main/java/com/sismics/docs/rest/resource/FileResource.java index f86383f4..5e1a65f8 100644 --- a/docs-web/src/main/java/com/sismics/docs/rest/resource/FileResource.java +++ b/docs-web/src/main/java/com/sismics/docs/rest/resource/FileResource.java @@ -102,7 +102,7 @@ public class FileResource extends BaseResource { try { document = documentDao.getDocument(documentId, PermType.WRITE, principal.getId()); } catch (NoResultException e) { - throw new ClientException("DocumentNotFound", MessageFormat.format("Document not found: {0}", documentId)); + return Response.status(Status.NOT_FOUND).build(); } } @@ -199,7 +199,7 @@ public class FileResource extends BaseResource { file = fileDao.getFile(id, principal.getId()); document = documentDao.getDocument(documentId, PermType.WRITE, principal.getId()); } catch (NoResultException e) { - throw new ClientException("DocumentNotFound", MessageFormat.format("Document not found: {0}", documentId)); + return Response.status(Status.NOT_FOUND).build(); } // Check that the file is orphan @@ -259,7 +259,7 @@ public class FileResource extends BaseResource { try { documentDao.getDocument(documentId, PermType.WRITE, principal.getId()); } catch (NoResultException e) { - throw new ClientException("DocumentNotFound", MessageFormat.format("Document not found: {0}", documentId)); + return Response.status(Status.NOT_FOUND).build(); } // Reorder files @@ -295,13 +295,9 @@ public class FileResource extends BaseResource { // Check document visibility if (documentId != null) { - try { - AclDao aclDao = new AclDao(); - if (!aclDao.checkPermission(documentId, PermType.READ, shareId == null ? principal.getId() : shareId)) { - throw new ForbiddenClientException(); - } - } catch (NoResultException e) { - throw new ClientException("DocumentNotFound", MessageFormat.format("Document not found: {0}", documentId)); + AclDao aclDao = new AclDao(); + if (!aclDao.checkPermission(documentId, PermType.READ, shareId == null ? principal.getId() : shareId)) { + return Response.status(Status.NOT_FOUND).build(); } } else if (!authenticated) { throw new ForbiddenClientException(); @@ -358,7 +354,7 @@ public class FileResource extends BaseResource { documentDao.getDocument(file.getDocumentId(), PermType.WRITE, principal.getId()); } } catch (NoResultException e) { - throw new ClientException("FileNotFound", MessageFormat.format("File not found: {0}", id)); + return Response.status(Status.NOT_FOUND).build(); } // Delete the file @@ -498,7 +494,7 @@ public class FileResource extends BaseResource { throw new ForbiddenClientException(); } } catch (NoResultException e) { - throw new ClientException("DocumentNotFound", MessageFormat.format("Document not found: {0}", documentId)); + return Response.status(Status.NOT_FOUND).build(); } // Get files and user associated with this document diff --git a/docs-web/src/main/webapp/src/app/docs/controller/DocumentView.js b/docs-web/src/main/webapp/src/app/docs/controller/DocumentView.js index fe80252a..8962c884 100644 --- a/docs-web/src/main/webapp/src/app/docs/controller/DocumentView.js +++ b/docs-web/src/main/webapp/src/app/docs/controller/DocumentView.js @@ -7,6 +7,8 @@ angular.module('docs').controller('DocumentView', function ($scope, $state, $sta // Load document data from server Restangular.one('document', $stateParams.id).get().then(function(data) { $scope.document = data; + }, function(response) { + $scope.error = response; }); // Load audit log data from server diff --git a/docs-web/src/main/webapp/src/app/docs/directive/ImgError.js b/docs-web/src/main/webapp/src/app/docs/directive/ImgError.js new file mode 100644 index 00000000..78772edd --- /dev/null +++ b/docs-web/src/main/webapp/src/app/docs/directive/ImgError.js @@ -0,0 +1,16 @@ +'use strict'; + +/** + * Image error event directive. + */ +angular.module('docs').directive('imgError', function() { + return { + restrict: 'A', + link: function(scope, element, attrs) { + element.bind('error', function() { + //call the function that was passed + scope.$apply(attrs.imgError); + }); + } + }; +}) \ No newline at end of file diff --git a/docs-web/src/main/webapp/src/index.html b/docs-web/src/main/webapp/src/index.html index ce5a6e97..6247f46b 100644 --- a/docs-web/src/main/webapp/src/index.html +++ b/docs-web/src/main/webapp/src/index.html @@ -64,6 +64,7 @@ + diff --git a/docs-web/src/main/webapp/src/partial/docs/document.view.html b/docs-web/src/main/webapp/src/partial/docs/document.view.html index 43988090..2643b426 100644 --- a/docs-web/src/main/webapp/src/partial/docs/document.view.html +++ b/docs-web/src/main/webapp/src/partial/docs/document.view.html @@ -1,4 +1,11 @@ - + + +
+

+ + Document not found +

+
diff --git a/docs-web/src/main/webapp/src/partial/docs/file.view.html b/docs-web/src/main/webapp/src/partial/docs/file.view.html index b0ecd077..8bf1cdb1 100644 --- a/docs-web/src/main/webapp/src/partial/docs/file.view.html +++ b/docs-web/src/main/webapp/src/partial/docs/file.view.html @@ -22,5 +22,12 @@
- + +

+ + File not found +

diff --git a/docs-web/src/test/java/com/sismics/docs/rest/TestDocumentResource.java b/docs-web/src/test/java/com/sismics/docs/rest/TestDocumentResource.java index 88ebe3b1..3e818114 100644 --- a/docs-web/src/test/java/com/sismics/docs/rest/TestDocumentResource.java +++ b/docs-web/src/test/java/com/sismics/docs/rest/TestDocumentResource.java @@ -282,8 +282,7 @@ public class TestDocumentResource extends BaseJerseyTest { documentResource = resource().path("/document/" + document1Id); documentResource.addFilter(new CookieAuthenticationFilter(document1Token)); response = documentResource.get(ClientResponse.class); - json = response.getEntity(JSONObject.class); - Assert.assertEquals(Status.BAD_REQUEST, Status.fromStatusCode(response.getStatus())); + Assert.assertEquals(Status.NOT_FOUND, Status.fromStatusCode(response.getStatus())); } /**