Repository: softwaremill/elasticmq (scala)

Issue:
delete-message with invalid receipt handle doesn't throw any error
Assume we have two queues, one is in the AWS and the other resides in the ElasticMQ locally. Both are empty.

When I try to delete the message from the queue in the AWS using the invalid receipt handle, I get the following:
```
aws sqs delete-message --queue-url https://queue.amazonaws.com/real-queue-url --receipt-handle fake

An error occurred (ReceiptHandleIsInvalid) when calling the DeleteMessage operation: The input receipt handle "fake" is not a valid receipt handle.
```

And when I try to do the same operation on the local queue in the elasticMQ it performs silently:
```
aws --endpoint-url http://localhost:9324 sqs delete-message --queue-url http://localhost:9324/queue/local_queue --receipt-handle fake
```

I start elasticMQ from Docker container by running `docker run -p 9324:9324 softwaremill/elasticmq` 
The version of the image is `0.15.3`

Pull request description:

Patch:
diff --git a/core/src/main/scala/org/elasticmq/ElasticMQError.scala b/core/src/main/scala/org/elasticmq/ElasticMQError.scala
index 46d2c8b95..e854ddf9b 100644
--- a/core/src/main/scala/org/elasticmq/ElasticMQError.scala
+++ b/core/src/main/scala/org/elasticmq/ElasticMQError.scala
@@ -20,3 +20,8 @@ class MessageDoesNotExist(val queueName: String, messageId: MessageId) extends E
   val code = "MessageDoesNotExist"
   val message = s"Message does not exist: $messageId in queue: $queueName"
 }
+
+class InvalidReceiptHandle(val queueName: String, receiptHandle: String) extends ElasticMQError {
+  val code = "ReceiptHandleIsInvalid"
+  val message = s"""The receipt handle "$receiptHandle" is not valid."""
+}
diff --git a/core/src/main/scala/org/elasticmq/actor/queue/operations/DeleteMessageOps.scala b/core/src/main/scala/org/elasticmq/actor/queue/operations/DeleteMessageOps.scala
index 3452e5a44..3468d1a93 100644
--- a/core/src/main/scala/org/elasticmq/actor/queue/operations/DeleteMessageOps.scala
+++ b/core/src/main/scala/org/elasticmq/actor/queue/operations/DeleteMessageOps.scala
@@ -1,20 +1,26 @@
 package org.elasticmq.actor.queue.operations
 
-import org.elasticmq.DeliveryReceipt
+import org.elasticmq.{DeliveryReceipt, InvalidReceiptHandle}
 import org.elasticmq.actor.queue.QueueActorStorage
 import org.elasticmq.util.Logging
 
 trait DeleteMessageOps extends Logging {
   this: QueueActorStorage =>
 
-  def deleteMessage(deliveryReceipt: DeliveryReceipt): Unit = {
+  def deleteMessage(deliveryReceipt: DeliveryReceipt): Either[InvalidReceiptHandle, Unit] = {
     val msgId = deliveryReceipt.extractId.toString
 
-    messageQueue.byId.get(msgId).foreach { msgData =>
-      if (msgData.deliveryReceipts.lastOption.contains(deliveryReceipt.receipt)) {
-        // Just removing the msg from the map. The msg will be removed from the queue when trying to receive it.
-        messageQueue.remove(msgId)
-      }
+    messageQueue.byId.get(msgId) match {
+      case Some(msgData) =>
+        if (msgData.deliveryReceipts.lastOption.contains(deliveryReceipt.receipt)) {
+          // Just removing the msg from the map. The msg will be removed from the queue when trying to receive it.
+          messageQueue.remove(msgId)
+          Right(())
+        } else {
+          Left(new InvalidReceiptHandle(queueData.name, deliveryReceipt.receipt))
+        }
+      case None =>
+        Left(new InvalidReceiptHandle(queueData.name, deliveryReceipt.receipt))
     }
   }
 }
diff --git a/core/src/main/scala/org/elasticmq/msg/QueueMsg.scala b/core/src/main/scala/org/elasticmq/msg/QueueMsg.scala
index 25737bb9c..44e24043f 100644
--- a/core/src/main/scala/org/elasticmq/msg/QueueMsg.scala
+++ b/core/src/main/scala/org/elasticmq/msg/QueueMsg.scala
@@ -35,6 +35,6 @@ case class ReceiveMessages(
     waitForMessages: Option[Duration],
     receiveRequestAttemptId: Option[String]
 ) extends QueueMessageMsg[List[MessageData]]
-case class DeleteMessage(deliveryReceipt: DeliveryReceipt) extends QueueMessageMsg[Unit]
+case class DeleteMessage(deliveryReceipt: DeliveryReceipt) extends QueueMessageMsg[Either[InvalidReceiptHandle, Unit]]
 case class LookupMessage(messageId: MessageId) extends QueueMessageMsg[Option[MessageData]]
 case object DeduplicationIdsCleanup extends QueueMessageMsg[Unit]
\ No newline at end of file
diff --git a/rest/rest-sqs/src/main/scala/org/elasticmq/rest/sqs/DeleteMessageBatchDirectives.scala b/rest/rest-sqs/src/main/scala/org/elasticmq/rest/sqs/DeleteMessageBatchDirectives.scala
index 2235eea63..01b1ba7ca 100644
--- a/rest/rest-sqs/src/main/scala/org/elasticmq/rest/sqs/DeleteMessageBatchDirectives.scala
+++ b/rest/rest-sqs/src/main/scala/org/elasticmq/rest/sqs/DeleteMessageBatchDirectives.scala
@@ -7,6 +7,8 @@ import org.elasticmq.actor.reply._
 import org.elasticmq.rest.sqs.Action.DeleteMessageBatch
 import org.elasticmq.rest.sqs.directives.ElasticMQDirectives
 
+import scala.xml.{Node, Text}
+
 trait DeleteMessageBatchDirectives {
   this: ElasticMQDirectives with BatchRequestsModule =>
   def deleteMessageBatch(p: AnyParams) = {
@@ -16,10 +18,17 @@ trait DeleteMessageBatchDirectives {
           val receiptHandle = messageData(ReceiptHandleParameter)
           val result = queueActor ? DeleteMessage(DeliveryReceipt(receiptHandle))
 
-          result.map { _ =>
-            <DeleteMessageBatchResultEntry>
-              <Id>{id}</Id>
-            </DeleteMessageBatchResultEntry>
+          result.map {
+            case Left(error) =>
+              <BatchResultErrorEntry>
+                <Id>{id}</Id>
+                <Code>{error.code}</Code>
+                <Message>{error.message}</Message>
+              </BatchResultErrorEntry>
+            case Right(_) =>
+              <DeleteMessageBatchResultEntry>
+                <Id>{id}</Id>
+              </DeleteMessageBatchResultEntry>
           }
         }
 
diff --git a/rest/rest-sqs/src/main/scala/org/elasticmq/rest/sqs/DeleteMessageDirectives.scala b/rest/rest-sqs/src/main/scala/org/elasticmq/rest/sqs/DeleteMessageDirectives.scala
index 0374a0531..5b3a8b1e5 100644
--- a/rest/rest-sqs/src/main/scala/org/elasticmq/rest/sqs/DeleteMessageDirectives.scala
+++ b/rest/rest-sqs/src/main/scala/org/elasticmq/rest/sqs/DeleteMessageDirectives.scala
@@ -14,14 +14,16 @@ trait DeleteMessageDirectives { this: ElasticMQDirectives =>
         p.requiredParam(ReceiptHandleParameter) { receipt =>
           val result = queueActor ? DeleteMessage(DeliveryReceipt(receipt))
 
-          result.map { _ =>
-            respondWith {
-              <DeleteMessageResponse>
-                <ResponseMetadata>
-                  <RequestId>{EmptyRequestId}</RequestId>
-                </ResponseMetadata>
-              </DeleteMessageResponse>
-            }
+          result.map {
+            case Left(error) => throw new SQSException(error.code, errorMessage = Some(error.message))
+            case Right(_) =>
+              respondWith {
+                <DeleteMessageResponse>
+                  <ResponseMetadata>
+                    <RequestId>{EmptyRequestId}</RequestId>
+                  </ResponseMetadata>
+                </DeleteMessageResponse>
+              }
           }
         }
       }


Test patch:
diff --git a/core/src/test/scala/org/elasticmq/actor/QueueActorMsgOpsTest.scala b/core/src/test/scala/org/elasticmq/actor/QueueActorMsgOpsTest.scala
index 952945de7..df5f49504 100644
--- a/core/src/test/scala/org/elasticmq/actor/QueueActorMsgOpsTest.scala
+++ b/core/src/test/scala/org/elasticmq/actor/QueueActorMsgOpsTest.scala
@@ -249,6 +249,27 @@ class QueueActorMsgOpsTest extends ActorTest with QueueManagerForEachTest with D
     }
   }
 
+  test("delete msg with invalid handle should fail") {
+    // Given
+    val q1 = createQueueData("q1", MillisVisibilityTimeout(1L))
+    val m1 = createNewMessageData("xyz", "123", Map(), MillisNextDelivery(50L))
+
+    for {
+      Right(queueActor) <- queueManagerActor ? CreateQueue(q1)
+      _ <- queueActor ? SendMessage(m1)
+      List(m1data) <- queueActor ? ReceiveMessages(DefaultVisibilityTimeout, 1, None, None)
+
+      // When
+      result <- queueActor ? DeleteMessage(DeliveryReceipt("0000#0000"))
+      lookupResult <- queueActor ? LookupMessage(MessageId("xyz"))
+    } yield {
+      // Then
+      result.isLeft should be(true)
+      result.swap.map(_.code).getOrElse("") should be("ReceiptHandleIsInvalid")
+      lookupResult shouldNot be(None)
+    }
+  }
+
   test("msg statistics should be updated") {
     // Given
     val q1 = createQueueData("q1", MillisVisibilityTimeout(1L))
diff --git a/rest/rest-sqs-testing-amazon-java-sdk/src/test/scala/org/elasticmq/rest/sqs/AmazonJavaSdkTestSuite.scala b/rest/rest-sqs-testing-amazon-java-sdk/src/test/scala/org/elasticmq/rest/sqs/AmazonJavaSdkTestSuite.scala
index d341a1811..794103673 100644
--- a/rest/rest-sqs-testing-amazon-java-sdk/src/test/scala/org/elasticmq/rest/sqs/AmazonJavaSdkTestSuite.scala
+++ b/rest/rest-sqs-testing-amazon-java-sdk/src/test/scala/org/elasticmq/rest/sqs/AmazonJavaSdkTestSuite.scala
@@ -936,6 +936,24 @@ class AmazonJavaSdkTestSuite extends SqsClientServerCommunication with Matchers
     m2 should be(None)
   }
 
+  test("should fail when deleting a message with invalid handle") {
+    // Given
+    val queueUrl = client
+      .createQueue(
+        new CreateQueueRequest("testQueue1")
+          .withAttributes(Map(defaultVisibilityTimeoutAttribute -> "1").asJava)
+      )
+      .getQueueUrl
+
+    // When
+    val result = catching(classOf[AmazonSQSException]) either {
+      client.deleteMessage(new DeleteMessageRequest(queueUrl, "0000#0000"))
+    }
+
+    result.isLeft should be(true)
+    result.swap.map(_.getMessage).getOrElse("") should include("ReceiptHandleIsInvalid")
+  }
+
   test("should delete messages in a batch") {
     // Given
     val queueUrl = client
@@ -962,7 +980,8 @@ class AmazonJavaSdkTestSuite extends SqsClientServerCommunication with Matchers
     val result = client.deleteMessageBatch(
       new DeleteMessageBatchRequest(queueUrl).withEntries(
         new DeleteMessageBatchRequestEntry("1", msgReceipts.head),
-        new DeleteMessageBatchRequestEntry("2", msgReceipts(1))
+        new DeleteMessageBatchRequestEntry("2", msgReceipts(1)),
+        new DeleteMessageBatchRequestEntry("3", "invalid")
       )
     )
     Thread.sleep(1100)
@@ -971,6 +990,7 @@ class AmazonJavaSdkTestSuite extends SqsClientServerCommunication with Matchers
 
     // Then
     result.getSuccessful.asScala.map(_.getId).toSet should be(Set("1", "2"))
+    result.getFailed.asScala.map(_.getId).toSet should be(Set("3"))
     m should be(None) // messages deleted
   }
 
@@ -1486,12 +1506,15 @@ class AmazonJavaSdkTestSuite extends SqsClientServerCommunication with Matchers
     Thread.sleep(1100)
     client.receiveMessage(new ReceiveMessageRequest(queueUrl)).getMessages.get(0) // 2nd receive
 
-    client.deleteMessage(new DeleteMessageRequest(queueUrl, m1.getReceiptHandle)) // Shouldn't delete - old receipt
+    val deleteResult = catching(classOf[AmazonServiceException]) either {
+      client.deleteMessage(new DeleteMessageRequest(queueUrl, m1.getReceiptHandle)) // Shouldn't delete - old receipt
+    }
 
     // Then
     Thread.sleep(1100)
     val m3 = receiveSingleMessage(queueUrl)
     m3 should be(Some("Message 1"))
+    deleteResult.isLeft should be(true)
   }
 
   test("should return an error if creating an existing queue with a different visibility timeout") {