Skip to content

Conversation

@seantleonard
Copy link
Contributor

@seantleonard seantleonard commented Feb 6, 2022

Summary

Closes #79 by added capability to UPSERT objects via primary key.

Requirements to Run Locally

Please note: refresh your local database schemas
with the updated MsSqlBooks.sql and PostgreSqlBooks.sql if you are running tests locally.

Microsoft REST Guidelines

Implementation adheres to Microsoft REST Guidelines

"Services MAY optionally support PUT to update existing resources, but if they do they MUST use replacement semantics (that is, after the PUT, the resource's properties MUST match what was provided in the request, including deleting any server properties that were not provided)."

Implementation Notes

Modifies the SqlMutationEngine method public async Task<JsonDocument> ExecuteAsync(RestRequestContext context) to handle UPSERTs. Operation can maintain idempotency by returning the fields that were successfully changed using the OUTPUT clause after the UPDATE/INSERT statement. Upserts are performed with a single payload used on ONE database roundtrip.

  • MS-SQL, the operation is wrapped in a transaction w/ UPDLOCK and is illustrated in public string Build(SqlUpsertQueryStructure structure) within MsSqlQueryBuilder.cs. The result can be TWO result sets:
    • Result Set # 1: Result of the Update operation, will be blank if no update performed.
    • Result Set # 2: Result of the Insert operation, this result set will not exist if the update was successful.
  • MySQL: Insert operation tests are ignored for now until they adhere to single transaction/db roundtrip model. The mutation engine has been modified to NOT perform an additional query after the mutation completes, as this introduces a concurrency issue: separate requests can modify the record between db roundtrip # 1 and # 2. This concurrency issue break the Idempotent promise this operation requires per Microsoft REST API Guidelines. Issue tracked in MySQL: HTTP POST/PUT Request Implementation Changes #220 . MySQL requires extra attention as it does not support the OUTPUT clause, which we use for MSSQL to return the entire new object.
  • PG: Same issue as MySQL. And is also on hold pending usage of PGREST decision.

Per the PM repo RFC Issue 50, PUT requires a request to define the Primary Key in the URL, and include remaining non-PK key/value pairs in the request body. The request will fail when trying to perform PUT that results in an Insert IF the primary key column of the entity is AutoGenerated (IDENTITY() column). The request will also fail if the request does not include key/value pairs for non nullable/non default valued fields.

Open Questions

Should we be redundant with config such that

  • Autogenerated designated columns also have IsNullable property? I would imagine yes, IF we ever want to support changing primary key.
  • Should FK column in "Columns" contain "IsNullable" ? and then be defaulted to false?
  • Integration tests test against the REST controller. More refactoring in this PR (probably a new PR) is necessary to handle tests that calculate "Expected" value by running db query against database and processing the appropriate SqlException. This is needed when the Insert operation fails due to PK/FK/NonNullable field constraints.

Tests

UpsertById

  • PUT operation on existing item updates the item with declared parameters, nulling out non-included parameters (that are nullable per schema).
  • PUT operation on non-existent item, inserts that item with declared parameters.
    • (Neg) if non nullable/non default valued parameter defined in request body -> 400 Bad Request
    • (Neg) PK NOT provided as part of PUT request -> 400 Bad Request.
    • (Neg) invalid FK provided for FK column -> 400 Bad Request.
    • (Neg) Upsert should not allow changing a row's primary key Column -> PK not allowed in body -> bad request.

Sample Requests

  1. PUT https://localhost:5001/books/id/1
    Request Body: { "title": "A Hobbit's Journey into the Forest 2", "publisher_id": 1234 }
  • If object exists:
    • JSON Response: HTTP 204 No Content
  • If object does not exist:
    • JSON Response: HTTP 400 Bad Request This is because the Book table's primary key is autogenerated.
  1. PUT https://localhost:5001/magazines/id/1
    Request Body: { "title": "Batman", "issueNumber": 1234 }
  • If object exists:
    • JSON Response: HTTP 204 No Content
  • If object does not exist:
    • JSON Response: HTTP 201 Created
    • JSON Response Body: Operation-Location:<resourceURI> this is successful because the magazine table does not have an autogenerated PK.
  1. PUT https://localhost:5001/books/id/2
  • Request Body: `{extendedTitle:"the hobbit",publisherId:"1234"}

  • If parameter listed in body are inconsistent with target resource schema, provide appropriate error:

JSON Response: HTTP 400 Bad Request which may need to be revised to be HTTP 409 Conflict.

…G and MySQL REST API tests for PUT , pending their implementation in separate PRs. MySQL revert Insert as we are migrating to one DB roundtrip approach, and need proper query building guidance.
…mns. Fixed logic for populating columns and params for PKs.
… Update variable names for UpsertRequest validation.
…esting. Updated REST API tests to only enable PUT for MSSQL, pending PG and MySQL implementation and adherence to One DB Roundtrip mutation query design.
… and make more methods virtual for manual skipping.
…ability to use one DB roundtrip mutation model. Also utilize insert constant for PUT.
@seantleonard
Copy link
Contributor Author

Updated PR description with latest PUT strategy details.

…sending HTTP 400 for trying to insert in a table with autogen'd primary key. Adjusted test framework to accommodate.
Copy link
Collaborator

@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.

Thank you for making sure we capture the right PUT behavior and with Transactions!

@seantleonard seantleonard merged commit aff6e0c into main Feb 16, 2022
@seantleonard seantleonard deleted the dev/seleonar/putREST branch February 16, 2022 01:42
@seantleonard seantleonard restored the dev/seleonar/putREST branch February 16, 2022 01:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MsSql/PgSql : UpsertById REST PUT

4 participants