From 3f7ffbc8bfdc8769a0398d25b9a429ad109f239b Mon Sep 17 00:00:00 2001 From: djmil Date: Tue, 25 Jul 2023 13:58:50 +0200 Subject: [PATCH] Delete endpoint --- Delete.md | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++++ Home.md | 60 ++++++++++++++++++++++ Security.md | 2 + 3 files changed, 205 insertions(+) create mode 100644 Delete.md diff --git a/Delete.md b/Delete.md new file mode 100644 index 0000000..60f49d5 --- /dev/null +++ b/Delete.md @@ -0,0 +1,143 @@ +To completely cover upcoming `Delete` endpoint with all the possible test cases, we need to create another user: + +```java +UserDetails kumar = users + .username("kumar2") + .password(passwordEncoder.encode("xyz789")) + .roles("CARD-OWNER") + .build(); +``` + +# Tests + +## The Happy Path + +```java +@Test +@DirtiesContext +void shouldDeleteAnExistingCashCard() { + ResponseEntity response = restTemplate + .withBasicAuth("sarah1", "abc123") + .exchange("/cashcards/99", HttpMethod.DELETE, null, Void.class); + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.NO_CONTENT); + + // Ensure that the resource was actually deleted: + ResponseEntity getResponse = restTemplate + .withBasicAuth("sarah1", "abc123") + .getForEntity("/cashcards/99", String.class); + assertThat(getResponse.getStatusCode()).isEqualTo(HttpStatus.NOT_FOUND); +} +``` + +Notice that we've added the `@DirtiesContext` annotation. We'll add this annotation to all tests which change the data. If we don't, then these tests could affect the result of other tests in the file. + +### Why not use `RestTemplate.delete()`? + +Notice that we're using `RestTemplate.exchange()` even though `RestTemplate` supplies a method that looks like we could use: `RestTemplate.delete()`. However, let's look at the signature: + +```java +public class RestTemplate ... { + public void delete(String url, Object... uriVariables) +``` + +The other methods we've been using (such as `getForEntity()` and `exchange()`) return a `ResponseEntity`, but `delete()` does not. Instead, it's a `void` method. Why is this? + +The Spring Web framework supplies the `delete()` method as a convenience, but it comes with some assumptions: + +1. A response to a `DELETE` request will have no body. +2. The client shouldn't care what the response code is unless it's an error, in which case, it'll throw an exception. + +Given those assumptions, no return value is needed from `delete()`. + +But the second assumption makes `delete()` unsuitable for us: We need the `ResponseEntity` in order to assert on the status code! So we won't use the convenience method, but rather let's use the more general method: `exchange()`. + +## Test case: The Cash Card doesn't exist + +Our contract states that we should return `404 NOT FOUND` if we try to delete a card that doesn't exist. + +```java +@Test +void shouldNotDeleteACashCardThatDoesNotExist() { + ResponseEntity deleteResponse = restTemplate + .withBasicAuth("sarah1", "abc123") + .exchange("/cashcards/99999", HttpMethod.DELETE, null, Void.class); + assertThat(deleteResponse.getStatusCode()).isEqualTo(HttpStatus.NOT_FOUND); +} +``` + +# Enforce ownership + +We need to check whether the record exists. If not, we should _not_ delete the Cash Card, and return `404 NOT FOUND`. Add the `existsByIdAndOwner()` method to the Repository. + +```java +public interface CashCardRepository extends CrudRepository, PagingAndSortingRepository { + ... + boolean existsByIdAndOwner(Long id, String owner); + ... +} +``` + +This is another case where Spring Data will generate the implementation of this method as long as we add it to the Repository. + +So why not just use the `findByIdAndOwner()` method and check whether it returns `null`? We could absolutely do that! But, such a call would return extra information (the content of the Cash Card retrieved), so we'd like to avoid it as to not introduce extra complexity. + +If you'd rather not use the `existsByIdAndOwner()` method, that's ok! You may choose to use `findByIdAndOwner()`. The final result will be the same! + +# Endpoint API + +```java +@DeleteMapping("/{id}") +private ResponseEntity deleteCashCard(@PathVariable Long id, Principal principal) { + if (cashCardRepository.existsByIdAndOwner(id, principal.getName())) { + cashCardRepository.deleteById(id); + return ResponseEntity.noContent().build(); + } + + return ResponseEntity.notFound().build(); +} +``` + +We use the `@DeleteMapping` with the `"{id}"` parameter, which Spring Web matches to the `id` method parameter. `CashCardRepository` already has the method we need: `deleteById()` (it's inherited from `CrudRepository`). + +# Hide Unauthorized Records + +At this point, you may ask yourself, "Are we done?" You are the best person to answer that question! There is one more case that we have yet to test: What if the user attempts to delete a Cash Card owned by someone else? We decided in the associated lesson that the response should be `404 NOT FOUND` in this case. Thats enough information for us to write a test for that case: + +```java +@Test +void shouldNotAllowDeletionOfCashCardsTheyDoNotOwn() { + ResponseEntity deleteResponse = restTemplate + .withBasicAuth("sarah1", "abc123") + .exchange("/cashcards/102", HttpMethod.DELETE, null, Void.class); + assertThat(deleteResponse.getStatusCode()).isEqualTo(HttpStatus.NOT_FOUND); +} +``` + +Do you think the test will pass? Take a moment to predict the outcome, then run the test. + +It passed! That's great news. + +- We have written a test for a specific case in our API. +- The test passed without any code changes! + +Now let's consider a question which may have occurred to you: Why do I need that test, since it passes without having to make any code changes? Isn't the purpose of TDD to use tests to guide the implementation of the application? If that's the case, why did I bother to write that test? + +Answers: + +- Yes, that is _one_ of many benefits that TDD provides: A process to guide the creation of code in order to arrive at a desired outcome. +- Tests themselves, though, have another purpose, separate from TDD: Tests are a powerful safety net to enforce correctness. Since the test you just wrote tests a different case than those already written, it provides value. If someone were to make a code change which caused this new test to fail, then you will have caught the error before it became an issue! Yay for Tests. + +But wait, you say. Shouldn't I test that the record that I tried to delete still exists in the database - that it didn't get deleted? Yes! That is a valid test. Thanks for mentioning it! Add the following code to the test method, to verify that the record you tried unsuccessfully to delete is still there: + +```java +void shouldNotAllowDeletionOfCashCardsTheyDoNotOwn() { +... + // Ensure that the record still exsists: + ResponseEntity getResponse = restTemplate + .withBasicAuth("kumar2", "xyz789") + .getForEntity("/cashcards/102", String.class); + assertThat(getResponse.getStatusCode()).isEqualTo(HttpStatus.OK); +} +``` + +Do you think the test will pass? Of course it will! (right?) Run the test. Just to make sure... please run all the tests and ensure that they pass. \ No newline at end of file diff --git a/Home.md b/Home.md index 43ba5df..7b24420 100644 --- a/Home.md +++ b/Home.md @@ -256,6 +256,66 @@ In the Cash Card API, we don’t need to allow `PUT` to create resources. We als The **bold** rows in the above table are implemented by Cash Card API. The non-bold ones are not. + +## Delete + +By now, you should be familiar with what question we should ask first in order to implement [[Delete]]: What is the API’s data specification for the Delete endpoint? The [specification](https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods/DELETE) includes the details of the Request and Response. + +Request: + +- Verb: `DELETE` +- URI: `/cashcards/{id}` +- Body: _(empty)_ + +Response: + +- Status code: `204 NO CONTENT` +- Body: _(empty)_ + +We’ll return the `204 NO CONTENT` status code for a successful delete, but there are additional cases: + +|Response Code|Use Case| +|---|---| +|`204 NO CONTENT`|- The record exists, and
- The Principal is authorized, and
- The record was successfully deleted.| +|`404 NOT FOUND`|- The record does not exist (a non-existent ID was sent).| +|`404 NOT FOUND`|- The record does exist but the Principal is not the owner.| + +Why do we return 404 for the "ID does not exist" and "not authorized to access this ID" cases? In order to not "leak" information: If the API returned different results for the two cases, then an unauthorized user would be able to discover specific IDs that they are not authorized to access. + +### Hard and Soft Delete + +So what does it mean to delete a Cash Card from a database’s point of view? Similar to how we decided that our Update operation means ”replace the entire existing record” (as opposed to supporting partial update), we need to decide what happens to resources when they are deleted. + +A simple option, called **hard delete**, is to delete the record from the database. With a hard delete, it’s gone forever. So what can we do if we need data that existed prior to its deletion? + +An alternative is **soft delete** which works by marking records as "deleted" in the database (so that they are retained, but marked as deleted). For example, we can introduce an `IS_DELETED` boolean or a `DELETED_DATE` timestamp column and then set that value-instead of fully removing the record by deleting the database row(s). With a soft delete, we also need to change how Repositories interact with the database. For example, a repository needs to respect the “deleted” column and exclude records marked deleted from Read requests. + +### Audit Trail and Archiving + +When working with databases, you’ll find that there’s often a requirement to keep a record of modifications to data records. For example: + +- A customer service representative might need to know _when_ a customer deleted their Cash Card. +- There may be data retention compliance regulations which require deleted data to be retained for a certain period of time. + +If the Cash Card is hard-deleted then we would need to store additional data to be able to answer this question. Let’s discuss some ways to record historical information: + +- **Archive** (move) the deleted data into a different location. +- Add **audit fields** to the record itself. For example, the `DELETED_DATE` column that we mentioned already. Additional audit fields can be added, for example `DELETED_BY_USER`. Again, this is not limited to Delete operations, but Create and Update also. + +APIs which implement soft delete and audit fields can return the state of the object in the response, and the `200 OK` status code. So why did we choose to use 204 instead of 200? Because the `204 NO CONTENT` status implies that there is no body in the response. + +- Maintain an **audit trail**. The audit trail is a record of all important operations done to a record. It can contain not only Delete operations, but Create and Update as well. + +The advantage of an audit trail over audit fields is that a trail records all events, whereas audit fields on the record capture only the most recent operation. An audit trail can be stored in a different database location, or even in log files. + +It’s worth mentioning that a combination of several of the above strategies is possible. Here are some examples: + +- We could implement soft delete, and then have a separate process which hard-deletes or archives soft-deleted records after a certain time period, like once per year. +- We could implement hard delete, and archive the deleted records. +- In any of the above cases, we could keep an audit log of which operations happened when. + +Finally, observe that even the simple specification that we’ve chosen doesn’t determine whether we implement hard or soft delete. It also doesn’t determine whether we add audit fields or keep an audit trail. However, the fact that we chose to return `204 NO CONTENT` implies that soft-delete is not happening, since if it was, we’d probably choose to return `200 OK` with the record in the body. + # Database The [**Separation of Concerns**](https://en.wikipedia.org/wiki/Separation_of_concerns) principle states that well-designed software should be modular, with each module having distinct and separate concerns from any other module. diff --git a/Security.md b/Security.md index f605042..f432a5f 100644 --- a/Security.md +++ b/Security.md @@ -336,6 +336,8 @@ We need only to define the methods, since Spring Data is perfectly capable of ge > **_Note:_** You might wonder whether Spring Data allows you to write your own SQL. After all, Spring Data can't anticipate every need, right? The answer is Yes! It's easy for you to write your own SQL code. The [Spring Data Query Methods](https://docs.spring.io/spring-data/jdbc/docs/current/reference/html/#jdbc.query-methods) documentation describes how to do so by using the `@Query` annotation. +Btw, here is slightly different approach to the [[Delete#Enforce ownership|ownership enforcement]]. As stated in the linked paragraph, which approach to choose - essentially boils down to the question of efficiency. + ## The Principal The `CashCardRepository` now supports filtering `CashCard` data by `owner`. But we're not using this new functionality. Let's enhance our app security by introducing a concept the [Principal](https://stackoverflow.com/questions/37499307/whats-the-principal-in-spring-security) concept to the `CashCardController`. As with other helpful objects, the `Principal` is available for us to use in our Controller. The `Principal` holds our user's authenticated, authorized information.