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

"Undelete" action is logged as an update in the changes table but a create in audit #3507

Closed
lmsurpre opened this issue Mar 25, 2022 · 5 comments
Assignees
Labels
P3 Priority 3 - Nice To Have schema-change a schema change

Comments

@lmsurpre
Copy link
Member

lmsurpre commented Mar 25, 2022

Describe the bug
We're somewhat inconsistent in the way we handle the "undelete" case (an update after a delete).

Our server returns a 201 (Created) status and we now log this as a create in the audit log (as of #2471).

This matches what the spec says (from https://www.hl7.org/fhir/http.html#delete)

If the interaction is successful, the server SHALL return either a 200 OK HTTP status code if the resource was updated, or a 201 Created status code if the resource was created (or brought back to life/re-created)

Prior to #3293 we were calling the beforeUpdate and afterUpdate methods for this case.
Therefor, the notification events we emitted also called this an update.
But now that the "prevResource" is null for this "undelete" case, we now call beforeCreate and afterCreate and therefor this interaction will lead to a create action code in the notification event going forward.
Furthermore, we also call FHIRPersistence.create (rather than FHIRPersistence.update).

However, when we insert into the RESOURCE_CHANGE_LOG table, we're still setting the changeType to U (and not C).
This leads to the _history api using the status code 200 for these requests, even though the actual status code was a 201.

Environment
main

To Reproduce
Steps to reproduce the behavior:

  1. create a resource
  2. delete it
  3. update it (bringing it "back to life")
  4. invoke system or type-level history

Note that the entry for step 3 indicates that the result was 200 OK even though the actual interaction resulted in a 201 Created

Expected behavior
The history api returns a status code that accurately reflects the actual status code for the "undelete" interaction.

Additional context
Originally identified at #2988 (comment)

@lmsurpre lmsurpre added the P3 Priority 3 - Nice To Have label Mar 25, 2022
@lmsurpre lmsurpre changed the title "Undelete" action is logged as an update in the "changes" table but a "create" in audit "Undelete" action is logged as an update in the changes table but a create in audit Mar 25, 2022
@lmsurpre lmsurpre self-assigned this Mar 25, 2022
@lmsurpre
Copy link
Member Author

I think it will require an update to the stored procedures (and to DerbyResourceDAO)

@lmsurpre lmsurpre added the schema-change a schema change label Mar 25, 2022
lmsurpre added a commit that referenced this issue Mar 25, 2022
1. add Entry.response.location to instance-history response
2. set 201 for the initial create entry (we need #3507 for the undelete
case)
3. remove the status code name from Entry.response.status for system and
type-level history interactions (e.g. `200 OK` -> `200`)

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Mar 25, 2022
1. add Entry.response.location to instance-history response
2. set 201 for the initial create entry (we need #3507 for the undelete
case)
3. remove the status code name from Entry.response.status for system and
type-level history interactions (e.g. `200 OK` -> `200`)

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Mar 25, 2022
1. add Entry.response.location to instance-history response
2. set 201 for the initial create entry (we need #3507 for the undelete
case)
3. remove the status code name from Entry.response.status for system and
type-level history interactions (e.g. `200 OK` -> `200`)

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
@lmsurpre
Copy link
Member Author

lmsurpre commented Mar 28, 2022

For system and type-level history interactions, we can easily start writing those to the CHANGE_LOG table as creates ('C') rather than updates ('U').

However, for instance-level history its not so simple due to the following limitations of the current approach:

  1. if versionId is 1, we don't know if it was created via a POST or a PUT (we always say POST)
  2. else, we can tell if its a PUT vs a DELETE, but in the case of a PUT we cannot reliably determine if its an "update after delete" unless we update the REST layer to retrieve a ResourceResult for the last entry from the prior page

To address both points (so that the Bundle.entry.response element always reflects the proper HTTP method and response status of that specific interaction) I think we'd need to reimplement this instance-level history on top of the CHANGE_LOG table.

For now, I'm thinking to just fix the system and type-level interactions... maybe we can open a separate issue for the instance-level history?

lmsurpre added a commit that referenced this issue Mar 28, 2022
Update db2 and postgresql add_any_resource.sql stored procs (and
DerbyResourceDAO.storeResource) to write a 'C' instead of a 'U' for this
case. This way, system and type-level history interactions can properly
report the response status as a 201 instead of a 200.

Update FHIRRestHelper.createHistoryBundle to handle this same case for
the instance-level history. Note, however, that there are ongoing
limitations for the accuracy of the instance-level history entries as
outlined at #3507 (comment)

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Mar 28, 2022
Update db2 and postgresql add_any_resource.sql stored procs (and
DerbyResourceDAO.storeResource) to write a 'C' instead of a 'U' for this
case. This way, system and type-level history interactions can properly
report the response status as a 201 instead of a 200.

Update FHIRRestHelper.createHistoryBundle to handle this same case for
the instance-level history. Note, however, that there are ongoing
limitations for the accuracy of the instance-level history entries as
outlined at
#3507 (comment)

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Mar 29, 2022
Update db2 and postgresql add_any_resource.sql stored procs (and
DerbyResourceDAO.storeResource) to write a 'C' instead of a 'U' for this
case. This way, system and type-level history interactions can properly
report the response status as a 201 instead of a 200.

Note, however, that there are ongoing
limitations for the accuracy of instance-level history entries as
outlined at
#3507 (comment)

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Mar 29, 2022
Update db2 and postgresql add_any_resource.sql stored procs (and
DerbyResourceDAO.storeResource) to write a 'C' instead of a 'U' for this
case. This way, system and type-level history interactions can properly
report the response status as a 201 instead of a 200.

Note, however, that there are ongoing
limitations for the accuracy of instance-level history entries as
outlined at
#3507 (comment)

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Mar 29, 2022
Update db2 and postgresql add_any_resource.sql stored procs (and
DerbyResourceDAO.storeResource) to write a 'C' instead of a 'U' for this
case. This way, system and type-level history interactions can properly
report the response status as a 201 instead of a 200.

Note, however, that there are ongoing
limitations for the accuracy of instance-level history entries as
outlined at
#3507 (comment)

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Mar 29, 2022
Update db2 and postgresql add_any_resource.sql stored procs (and
DerbyResourceDAO.storeResource) to write a 'C' instead of a 'U' for this
case. This way, system and type-level history interactions can properly
report the response status as a 201 instead of a 200.

Note, however, that there are ongoing
limitations for the accuracy of instance-level history entries as
outlined at
#3507 (comment)

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Mar 30, 2022
per review feedback

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Mar 30, 2022
per review feedback

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Mar 30, 2022
issue #3507 - treat "undelete" updates like create-on-update in changes
@d0roppe
Copy link
Collaborator

d0roppe commented Apr 3, 2022

From testing this with the latest code, I am still seeing code 200 on a recreate for the _history

@lmsurpre
Copy link
Member Author

lmsurpre commented Apr 4, 2022

From testing this with the latest code, I am still seeing code 200 on a recreate for the _history

That is mentioned as a limitation of the current approach for instance-level history just above.
I've now opened #3554 to track that.

@d0roppe
Copy link
Collaborator

d0roppe commented Apr 4, 2022

With verifying this for whole system history and type history this is working as expected.

@d0roppe d0roppe closed this as completed Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 Priority 3 - Nice To Have schema-change a schema change
Projects
None yet
Development

No branches or pull requests

2 participants