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

Prevent no impact updates from creating version - sql #2519

Conversation

apurvabhaleMS
Copy link
Contributor

@apurvabhaleMS apurvabhaleMS commented Mar 22, 2022

Description

If a user updates a record that already exists and nothing has changed in the content, then the user should get a 200 but nothing should update the record. Updated UpsertAsync method to add validations in the code instead of in UpsertResource SP and check the existing resource rawData with new resource rawData by ignoring meta.versionId and meta.lastUpdated. If its an absolute match then we simply return Ok with existing resource information without updating VersionId and lastUpdated. If the strings do not match then we proceed with further steps of creating a new version.

Current login -
Inside a transaction in SP

  • Read a row with a lock
  • Validate
  • Update/Create/Delete

Updated logic -
In code -

  • Loop to continue below transaction when the resource is updated by the time we reach the SP

  • Read the latest resource info

  • Validate

  • Parse, Replace, and Compare resource content with old and new data

  • If match return OK with existing resource

  • If do not match then calls SP with compared version

  • In SP transaction

    • Read a row with a lock
    • Compare old with existing DB version
    • If a match (in code comparison was with valid versions) then go ahead with version creation based on if resource Changed
    • VersionMatched = true -> Create new verson
    • If no match (somebody else updated or added new resource) then throw an error for client to retry

Notes -

  • IsMetaSet

    • Added logic to check IsMetaSet flag while forming verisonId string (before comparing resource content), if it's false then the version id in rawResource is invalid
    • NewResource has VersionId=1 and IsMetaSet=true
    • SQL
      • Update requests - Saves IsMetaSet = false and does not update the raw Resource data - retaining versionId = 1
    • Cosmos
      • JSON Update requests saves IsMetaSet = true and updates the rawResource only for JSON request data type versionId is set correctly
      • Other than JSON Update requests saves IsMetaSet = false and does not update the raw Resource for other request data types like XML - retaining versionId = 1
  • Can this be possible ?

    • "meta": {
      "lastUpdated": "2022-03-23T00:03:05.378+00:00",
      "versionId": "1"
      } => "meta" : {,}

    • "meta":{"lastUpdated":"2022-03-23T00:03:05.378+00:00"} => "meta" : {}

    • Above comparison will fail, but this should not happen. Generally we set lastUpdated and versionId both in a meta depending on the flag keepMeta which is set by canUpdateCreate (UpdateCreate from capability statement)

      • Delete - Set as false
      • Create - Hardcoded to true
      • Update - Depends on policy (canUpdateOrCreate)
        - True = all good
        - False = Shouldn't be set to false and not toggled**
  • We are forming versionId and lastUpdated strings to use String.Replace on raw resource data. Forming lastUpdated with a valid date format is little tricky. We set the value of lastUpdated by using ToString("o") which formats the datetimeoffset to Formats to 2022-03-09T01:40:52.0690000+02:00. String serializer converts this to 2022-03-09T01:40:52.069+02:00 and saves it.

    In C#, millisecond value can range from [0 to 999]. Below are some useful examples of how date is stored in rawResource data in DB.
    0000000+ -> +, 0010000+ -> 001+, 0100000+ -> 01+, 0180000+ -> 018+, 1000000 -> 1+, 1100000+ -> 11+, 1010000+ -> 101+

Related issues

Addresses 89485

Testing

Manual testing
Updated existing tests
Additional tests to cover duplicate data do not creates a new version
If metadata has other properties which were changed during the update then new version should be created

FHIR Team Checklist

  • Update the title of the PR to be succinct and less than 50 characters
  • Add a milestone to the PR for the sprint that it is merged (i.e. add S47)
  • Tag the PR with the type of update: Bug, Dependencies, Enhancement, or New-Feature
  • Tag the PR with Azure API for FHIR if this will release to the Azure API for FHIR managed service (CosmosDB or common code related to service)
  • Tag the PR with Azure Healthcare APIs if this will release to the Azure Healthcare APIs managed service (Sql server or common code related to service)
  • CI is green before merge
  • Review squash-merge requirements

Semver Change (docs)

Patch|Skip|Feature|Breaking (reason)

…prevent-no-impact-updates-from-creating-version-sql

# Conflicts:
#	test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/UpdateTests.cs
#	test/Microsoft.Health.Fhir.Shared.Tests.Integration/Persistence/FhirStorageTests.cs
@apurvabhaleMS apurvabhaleMS added Area-SQL Area related to the SQL Server data provider Azure API for FHIR Label denotes that the issue or PR is relevant to the Azure API for FHIR labels Mar 22, 2022
@apurvabhaleMS apurvabhaleMS marked this pull request as ready for review March 23, 2022 19:02
@apurvabhaleMS apurvabhaleMS requested a review from a team as a code owner March 23, 2022 19:03
SergeyGaluzo
SergeyGaluzo previously approved these changes Mar 26, 2022
Copy link
Contributor

@SergeyGaluzo SergeyGaluzo left a comment

Choose a reason for hiding this comment

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

:shipit:

feordin
feordin previously approved these changes Mar 30, 2022
…prevent-no-impact-updates-from-creating-version-sql
…prevent-no-impact-updates-from-creating-version-sql

# Conflicts:
#	src/Microsoft.Health.Fhir.SqlServer/Features/Schema/Migrations/29.diff.sql
#	src/Microsoft.Health.Fhir.SqlServer/Features/Schema/Migrations/29.sql
#	src/Microsoft.Health.Fhir.SqlServer/Features/Schema/SchemaVersionConstants.cs
…ating-version-sql' of https://github.com/microsoft/fhir-server into personal/apurvabhale/prevent-no-impact-updates-from-creating-version-sql

# Conflicts:
#	src/Microsoft.Health.Fhir.SqlServer/Features/Schema/Migrations/29.diff.sql
#	src/Microsoft.Health.Fhir.SqlServer/Features/Schema/Migrations/29.sql
@apurvabhaleMS apurvabhaleMS dismissed stale reviews from feordin and SergeyGaluzo via 83b8a21 March 31, 2022 17:03
…prevent-no-impact-updates-from-creating-version-sql
…prevent-no-impact-updates-from-creating-version-sql

# Conflicts:
#	src/Microsoft.Health.Fhir.SqlServer/Features/Storage/SqlServerFhirDataStore.cs
@apurvabhaleMS
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@apurvabhaleMS
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@apurvabhaleMS
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@apurvabhaleMS apurvabhaleMS merged commit 3287f87 into main Apr 7, 2022
@apurvabhaleMS apurvabhaleMS deleted the personal/apurvabhale/prevent-no-impact-updates-from-creating-version-sql branch April 7, 2022 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-SQL Area related to the SQL Server data provider Azure API for FHIR Label denotes that the issue or PR is relevant to the Azure API for FHIR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants