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.

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

DeleteById REST Support #172

Merged
merged 19 commits into from
Jan 31, 2022
Merged

DeleteById REST Support #172

merged 19 commits into from
Jan 31, 2022

Conversation

seantleonard
Copy link
Contributor

@seantleonard seantleonard commented Jan 28, 2022

Summary

Closes #80 by adding capability to DELETE objects via primary key. Please note: refresh your local database schemas
with the updated MsSqlBooks.sql and PostgreSqlBooks.sql if you are running tests locally.

Sample Requests

  1. DELETE https://localhost:5001/books/id/1
  • If object exists:

JSON Response: HTTP 204 No Content
JSON Body: Null

  • If object has already been deleted:

JSON Response: HTTP 404 Not Found
JSON Body:

{
    "type": "https://tools.ietf.org/html/rfc7231#section-6.5.4",
    "title": "Not Found",
    "status": 404,
    "traceId": "00-6b7c287f51dbe1bd51e4f9ea9f2a4740-0ed692042b8bc00f-00"
}

Implementation Notes

SqlDeleteQueryStructure.cs

Predicates must be properly created in order to have the runtime type/database type resolved. The right-hand side of the predicate looks like
new PredicateOperand($"@{MakeParamWithValue(GetParamAsColumnSystemType(param.Value.ToString(), param.Key))}")
to avoid Postgres Test issue with DELETE parameters where primary key id, of type integer, is sent to Postgres as a string.
This results in bigint != text errors.

SqlTestHelper.cs

In VerifyResult(), I used a C# 7 feature of switching off of Types to keep the code neat. The actionResult (of type IActionResult) is then matched against the correct implementation (OkObjectResult,NoContentResult, etc.) to then have aligning logic pulling out HTTP status code and message, if available. More info about switching on type can be found in this Microsoft Announcement blog

RestController.cs

Queries that result in the database returning 0 results will return a DataGateway exception with statusCode:NotFound. This includes GET (no item found) and DELETE (no item exists to delete).

Additional Notes

DeleteById requests must have the primary key validated. Prior to this PR, the code path in RestService.cs validates PK when it is included, otherwise not. Logic is now added to accommodate for DELETE requests.

Tests

DeleteById integration tests check the following:

  • DeleteOne correctly returns 1 affected result from the DB reader and validates the response 204 NoContent.
  • DeleteNonExistent validates that subsequent requests after a DELETE return 404 Not Found.
  • DeleteWithInvalidPrimaryKey ensures DELETE requests must define a primary key and otherwise returns 400 Bad Request.

There are dedicated items added to the SQL/PG SQL files used to create the test tables. This ensures DELETE tests are consistent such that they only delete and validate items exclusive to their tests if running concurrently to other tests. i.e. Delete will not touch database records other tests rely on and vice versa.

Todo Future PR

  • DeleteById, if delete fails due to table FK constraints (No cascading deletes), validate error is handled gracefully.

@seantleonard seantleonard added this to the M0 milestone Jan 28, 2022
@seantleonard seantleonard linked an issue Jan 28, 2022 that may be closed by this pull request
@seantleonard seantleonard added the mssql pgsql mysql an issue that applies to all relational databases, same as labeling with `mssql` `mysql` and `pgsql` label Jan 28, 2022
Copy link
Contributor

@Aniruddh25 Aniruddh25 left a comment

Choose a reason for hiding this comment

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

Thanks for the fast turn around! Just some minor fixes needed.

Copy link
Contributor

@Aniruddh25 Aniruddh25 left a comment

Choose a reason for hiding this comment

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

LGTM!
Just update the description to remove the work in progress because its no longer the case :)

Copy link
Contributor

@JelteF JelteF left a comment

Choose a reason for hiding this comment

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

I think this is good apart from the comment I left about validation.

@JelteF
Copy link
Contributor

JelteF commented Jan 31, 2022

Why is the NotFound response in this strange format:

{
    "type": "https://tools.ietf.org/html/rfc7231#section-6.5.4",
    "title": "Not Found",
    "status": 404,
    "traceId": "00-6b7c287f51dbe1bd51e4f9ea9f2a4740-0ed692042b8bc00f-00"
}

Instead of the exception format we use for all other exceptions. Can you change it to match our other exceptions?

@seantleonard
Copy link
Contributor Author

seantleonard commented Jan 31, 2022

@JelteF Updated NotFound() to instead be DataGatewayException with statusCode: NotFound

@seantleonard seantleonard merged commit a62b5ea into main Jan 31, 2022
@seantleonard seantleonard deleted the dev/seleonar/delRestRequest branch January 31, 2022 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mssql pgsql mysql an issue that applies to all relational databases, same as labeling with `mssql` `mysql` and `pgsql`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MsSql/PgSql : DeleteById REST DELETE
5 participants