Start support for PATCH verb. #341

jjrdk · 2021-03-01T10:54:57Z

I started this work to add support for patch operations as described in the FHIR specs.

This is just an initial draft to get the work started, so any feedback regarding how it should be properly fit into the architecture would be appreciated. Specifically there are some open points about how to integrate with the IAsyncFhirService.

Currently there is support for FHIR patches using Parameters. No json/xml patch is currently supported.

For the different patch operations, there are test cases for different examples, but additional test cases would help make this feature more robust.

jjrdk · 2021-03-01T10:57:25Z

Back link to related issue

jjrdk · 2021-03-04T07:40:54Z

Can someone tell me what is the secret for the integration tests? I have left them unchanged and still they fail :-(

kennethmyhra · 2021-03-05T11:04:02Z

@jjrdk will have to look into it

src/Spark.Engine/Service/AsyncFhirService.cs

…/patch-verb

src/Spark.Engine/Extensions/PatchExtensions.cs

src/Spark.Engine/Service/FhirServiceExtensions/IPatchApplicationService.cs

src/Spark.Engine/Service/FhirServiceExtensions/PatchApplicationService.cs

src/Spark.Engine/Service/AsyncFhirService.cs

src/Spark.Engine/Service/FhirServiceExtensions/PatchApplicationService.cs

src/Spark.Engine.Test/Service/PatchApplicationServiceTests.cs

src/Spark.Engine/Service/FhirServiceExtensions/PatchApplicationService.cs

kennethmyhra · 2021-03-17T19:30:49Z

Appreciate the effort @jjrdk!

kennethmyhra · 2021-03-22T18:36:47Z

@jjrdk been looking some more into this and doing some tests.

Trying to run a PATCH via Postman using the Parameters resource below throws the exception: Expression of type 'Hl7.Fhir.Model.Code' cannot be used for assignment to type 'Hl7.Fhir.Model.Code``1[Hl7.Fhir.Model.MedicationRequest+medicationrequestStatus]' I think the resource below is valid

Did you consider using FhirPath in place of Linq Expressions? For some of the operations I think this could simplify things

You are maybe already aware that the ToPatch extension method generates delete operations if the property is null. I kind of get why, but this could also be a bit dangerous behavior. So if we in the future going to use this in a client-side manner we would need to be aware of this. In such a scenario we might not always have the full resource

And lastly not sure why the integration tests currently fails, but I have run them locally and they run fine.

{
    "resourceType": "Parameters",
    "parameter": [{
            "name": "operation",
            "part": [{
                    "name": "path",
                    "valueString": "MedicationRequest.status"
                }, {
                    "name": "type",
                    "valueCode": "replace"
                }, {
                    "name": "value",
                    "valueCode": "completed"
                }
            ]
        }
    ]
}

jjrdk · 2021-03-22T22:33:40Z

@kennethmyhra Thanks for getting back on this. I had a look at your test case, and the problem is that the value is parsed by the FHIR parser as a Code (Hl7.Fhir.Model.Code) when the expected value is Code (Hl7.Fhir.Model.Code). These values are unrelated in the type hierarchy, so casting is not an option.

I'll have a closer look at the case before proposing a solution. I'd like to avoid hacking in too many custom conversions.

Regarding FhirPath or LINQ expression, then I actually use the official FhirPath compiler to get the path and then transform this using a visitor. I can have a look at whether it works better to have a visitor that directly updates the resource based on the FhirPath.

On the last point, then I agree that there are going to be a lot of scenarios that have to be considered in terms of access rights and generating patches. I see three ways to approach this:

Take away the extension method and not provide the operation
Clearly document the behavior and let the user customize the patch afterwards based on the current context.
Parameterize the operation to exclude handling specific properties

Maybe a mix of 2 and 3 is preferred.

…nal types.

…tures/patch-verb

jjrdk · 2021-03-23T22:02:45Z

I had a look at the handling of Code and it's the only generic subtype of DataType, so I implemented a special case for that. I added tests for other common types.

kennethmyhra · 2021-03-29T08:52:44Z

@jjrdk Will pull this, just need to have another look at why the integration tests fail

jjrdk · 2021-03-29T09:24:29Z

@kennethmyhra Before you pull, let me try to see if the issue is the test data files. I'll try to pull the test data into code and see if I can somehow get this issue fixed.

Would not like to provide broken integration tests.

kennethmyhra · 2021-04-01T06:54:23Z

Seems like that worked @jjrdk. That's strange, shouldn't really have had an impact and was not all what the integrations tests complained about.

jjrdk · 2021-04-01T06:55:34Z

I'll try to bring them back individually and see if I can pinpoint the offender.

kennethmyhra · 2021-04-01T06:57:13Z

Yep, I'll leave the PR until you have done some more investigation

kennethmyhra · 2021-04-02T09:23:33Z

@jjrdk The guy who set up the integration tests will have a look at why those are failing for this PR.

Any other suggestion I have is to revert the last 4 commits that made this fail again or pull out the diff of the changes you want to apply and create a new PR and see if that still fails.

andy-a-o · 2021-04-02T10:33:03Z

If this may help..

from the html summaries archive:
History001_http---spark-fhir_04-02-21_06-39-55.html

FAIL - HI08 - hi08_all_history_whole_system_with_since_test
id HI08
description all history whole system with since
status fail
message Result contains entries in the wrong order.
requires
resourcePatient
methods["create", "update", "delete"]
validates
resource
methods["history-system"]
links
http://hl7.org/fhir/http.html#history
requests
request{:method=>:get, :url=>"http://spark/fhir/_history?_since=2021-04-02T06:38:55Z", :headers=>{"User-Agent"=>"Ruby FHIR Client", "Accept-Charset"=>"utf-8", "Accept"=>"application/fhir+json"}, :path=>"/_history?_since=2021-04-02T06:38:55Z"}
response{:code=>200, :headers=>{"date"=>"Fri, 02 Apr 2021 06:39:55 GMT", "content-type"=>"application/fhir+json; charset=utf-8", "server"=>"Kestrel", "transfer-encoding"=>"chunked"}, :body=>"{\"resourceType\":\"Bundle\",\"id\":\"53e5e4ac-0f73-4414-bf1e-8e2b1089b77d\",\"type\":\"history\",\"total\":121,\"link\":[{\"relation\":\"self\",\"url\":\"http://spark/fhir/_snapshot?id=2f782c44-5b1f-4568-a2cf-8486f0cd9f05&start=0\"},{\"relation\":\"first\",\"url\":\"http://spark/fhir/_snapshot?id=2f782c44-5b1f-4568-a2cf-8486f0cd9f05&start=0\"},{\"relation\":\"last\",\"url\":\"http://spark/fhir/_snapshot?id=2f782c44-5b1f-4568-a2cf-8486f0cd9f05&start=120\"},{\"relation\":\"next\",\"url\":\"http://spark/fhir/_snapshot?id=2f782c44-5b1f-4568-a2cf-8486f0cd9f05&start=20\"}],\"entry\":[{\"request\":{\"method\":\"DELETE\",\"url\":\"http://spark/fhir/Patient/1007/_history/3\"}},{\"fullUrl\":\"http://spark/fhir/Patient/1007/_history/2\",\"resource\":{\"resourceType\":\"Patient\",\"id\":\"1007\",\"meta\":{\"versionId\":\"2\",\"lastUpdated\":\"2021-04-02T06:39:55.087145+00:00\",\"tag\":[{\"system\":\"http://projectcrucible.org\",\"code\":\"testdata\"}]},\"name\":[{\"family\":\"Emerald\",\"given\":[\"Caro\"]}],\"telecom\":[{\"system\":\"email\",\"value\":\"[email protected]\"}]},\"request\":{\"method\":\"PUT\",\"url\":\"http://spark/fhir/Patient/1007/_history/2\"}},{\"fullUrl\":\"http://spark/fhir/Patient/1007/_history/1\",\"resource\":{\"resourceType\":\"Patient\",\"id\":\"1007\",\"meta\":{\"versionId\":\"1\",\"lastUpdated\":\"2021-04-02T06:39:55.042596+00:00\",\"tag\":[{\"system\":\"http://projectcrucible.org\",\"code\":\"testdata\"}]},\"name\":[{\"family\":\"Emerald\",\"given\":[\"Caro\"]}]},\"request\":{\"method\":\"POST\",\"url\":\"http://spark/fhir/Patient/1007/_history/1\"}},{\"request\":{\"method\":\"DELETE\",\"url\":\"http://spark/fhir/Patient/1006/_history/2\"}},{\"request\":{\"method\":\"DELETE\",\"url\":\"http://spark/fhir/Observation/23/_history/2\"}},{\"request\":{\"method\":\"DELETE\",\"url\":\"http://spark/fhir/Observation/20/_history/2\"}},{\"request\":{\"method\":\"DELETE\",\"url\":\"http://spark/fhir/Condition/6/_history/2\"}},{\"request\":{\"method\":\"DELETE\",\"url\":\"http://spark/fhir/Condition/5/_history/2\"}},{\"request\":{\"method\":\"DELETE\",\"url\":\"http://spark/fhir/Patient/1005/_history/2\"}},{\"fullUrl\":\"http://spark/fhir/Patient/1006/_history/1\",\"resource\":{\"resourceType\":\"Patient\",\"id\":\"1006\",\"meta\":{\"versionId\":\"1\",\"lastUpdated\":\"2021-04-02T06:39:18.214087+00:00\",\"tag\":[{\"system\":\"http://projectcrucible.org\",\"code\":\"testdata\"}]},\"identifier\":[{\"use\":\"official\",\"system\":\"http://projectcrucible.org\",\"value\":\"1617345557\"}],\"name\":[{\"use\":\"official\",\"text\":\"Transaction Crucible\",\"family\":\"Crucible\",\"given\":[\"Transaction\"]}]},\"request\":{\"method\":\"POST\",\"url\":\"http://spark/fhir/Patient/1006/_history/1\"}},{\"request\":{\"method\":\"DELETE\",\"url\":\"http://spark/fhir/Observation/22/_history/2\"}},{\"request\":{\"method\":\"DELETE\",\"url\":\"http://spark/fhir/Observation/21/_history/2\"}},{\"fullUrl\":\"http://spark/fhir/Condition/6/_history/1\",\"resource\":{\"resourceType\":\"Condition\",\"id\":\"6\",\"meta\":{\"versionId\":\"1\",\"lastUpdated\":\"2021-04-02T06:39:18.0805792+00:00\",\"tag\":[{\"system\":\"http://projectcrucible.org\",\"code\":\"testdata\"}]},\"clinicalStatus\":{\"coding\":[{\"system\":\"http://terminology.hl7.org/CodeSystem/condition-clinical\",\"code\":\"resolved\"}]},\"verificationStatus\":{\"coding\":[{\"system\":\"http://terminology.hl7.org/CodeSystem/condition-ver-status\",\"code\":\"refuted\"}]},\"code\":{\"coding\":[{\"system\":\"http://snomed.info/sct\",\"code\":\"414915002\"}]},\"subject\":{\"reference\":\"Patient/1005\"},\"abatementString\":\"Abated at unknown date\"},\"request\":{\"method\":\"POST\",\"url\":\"http://spark/fhir/Condition/6/_history/1\"}},{\"fullUrl\":\"http://spark/fhir/Observation/23/_history/1\",\"resource\":{\"resourceType\":\"Observation\",\"id\":\"23\",\"meta\":{\"versionId\":\"1\",\"lastUpdated\":\"2021-04-02T06:39:18.0652924+00:00\",\"tag\":[{\"system\":\"http://projectcrucible.org\",\"code\":\"testdata\"}]},\"status\":\"final\",\"code\":{\"coding\":[{\"system\":\"http://loinc.org\",\"code\":\"3141-9\"}]},\"subject\":{\"reference\":\"Patient/1005\"},\"valueQuantity\":{\"value\":100.0,\"unit\":\"kg\",\"system\":\"http://unitsofmeasure.org\"}},\"request\":{\"method\":\"POST\",\"url\":\"http://spark/fhir/Observation/23/_history/1\"}},{\"fullUrl\":\"http://spark/fhir/Observation/22/_history/1\",\"resource\":{\"resourceType\":\"Observation\",\"id\":\"22\",\"meta\":{\"versionId\":\"1\",\"lastUpdated\":\"2021-04-02T06:39:17.9974255+00:00\",\"tag\":[{\"system\":\"http://projectcrucible.org\",\"code\":\"testdata\"}]},\"status\":\"final\",\"code\":{\"coding\":[{\"system\":\"http://loinc.org\",\"code\":\"3141-9\"}]},\"subject\":{\"reference\":\"Patient/1005\"},\"valueQuantity\":{\"value\":250.0,\"unit\":\"kg\",\"system\":\"http://unitsofmeasure.org\"}},\"request\":{\"method\":\"POST\",\"url\":\"http://spark/fhir/Observation/22/_history/1\"}},{\"fullUrl\":\"http://spark/fhir/Observation/20/_history/1\",\"resource\":{\"resourceType\":\"Observation\",\"id\":\"20\",\"meta\":{\"versionId\":\"1\",\"lastUpdated\":\"2021-04-02T06:39:17.7610763+00:00\",\"tag\":[{\"system\":\"http://projectcrucible.org\",\"code\":\"testdata\"}]},\"status\":\"final\",\"code\":{\"coding\":[{\"system\":\"http://loinc.org\",\"code\":\"8302-2\"}]},\"subject\":{\"reference\":\"Patient/1005\"},\"valueQuantity\":{\"value\":170.0,\"unit\":\"cm\",\"system\":\"http://unitsofmeasure.org\"}},\"request\":{\"method\":\"POST\",\"url\":\"http://spark/fhir/Observation/20/_history/1\"}},{\"fullUrl\":\"http://spark/fhir/Observation/21/_history/1\",\"resource\":{\"resourceType\":\"Observation\",\"id\":\"21\",\"meta\":{\"versionId\":\"1\",\"lastUpdated\":\"2021-04-02T06:39:17.7611023+00:00\",\"tag\":[{\"system\":\"http://projectcrucible.org\",\"code\":\"testdata\"}]},\"status\":\"final\",\"code\":{\"coding\":[{\"system\":\"http://loinc.org\",\"code\":\"3141-9\"}]},\"subject\":{\"reference\":\"Patient/1005\"},\"valueQuantity\":{\"value\":200.0,\"unit\":\"kg\",\"system\":\"http://unitsofmeasure.org\"}},\"request\":{\"method\":\"POST\",\"url\":\"http://spark/fhir/Observation/21/_history/1\"}},{\"fullUrl\":\"http://spark/fhir/Condition/5/_history/1\",\"resource\":{\"resourceType\":\"Condition\",\"id\":\"5\",\"meta\":{\"versionId\":\"1\",\"lastUpdated\":\"2021-04-02T06:39:17.7611201+00:00\",\"tag\":[{\"system\":\"http://projectcrucible.org\",\"code\":\"testdata\"}]},\"verificationStatus\":{\"coding\":[{\"system\":\"http://terminology.hl7.org/CodeSystem/condition-ver-status\",\"code\":\"confirmed\"}]},\"code\":{\"coding\":[{\"system\":\"http://snomed.info/sct\",\"code\":\"414915002\"}]},\"subject\":{\"reference\":\"Patient/1005\"}},\"request\":{\"method\":\"POST\",\"url\":\"http://spark/fhir/Condition/5/_history/1\"}},{\"fullUrl\":\"http://spark/fhir/Patient/1005/_history/1\",\"resource\":{\"resourceType\":\"Patient\",\"id\":\"1005\",\"meta\":{\"versionId\":\"1\",\"lastUpdated\":\"2021-04-02T06:39:17.7605158+00:00\",\"tag\":[{\"system\":\"http://projectcrucible.org\",\"code\":\"testdata\"}]},\"identifier\":[{\"use\":\"official\",\"system\":\"http://projectcrucible.org\",\"value\":\"1617345557\"}],\"name\":[{\"use\":\"official\",\"text\":\"Transaction Crucible\",\"family\":\"Crucible\",\"given\":[\"Transaction\"]}]},\"request\":{\"method\":\"POST\",\"url\":\"http://spark/fhir/Patient/1005/_history/1\"}},{\"request\":{\"method\":\"DELETE\",\"url\":\"http://spark/fhir/Observation/19/_history/2\"}}]}"}

andy-a-o · 2021-04-02T10:37:58Z

And this is the test code https://github.com/incendilabs/plan_executor/blob/14a964ae64bfc53d93b794cbfe11c06518b1bde4/lib/tests/suites/history_test.rb#L203

andy-a-o · 2021-04-02T10:50:57Z

Problematic records:

               "lastUpdated":"2021-04-02T06:39:55.087145+00:00",
               "lastUpdated":"2021-04-02T06:39:55.042596+00:00",
               "lastUpdated":"2021-04-02T06:39:18.214087+00:00",
               "lastUpdated":"2021-04-02T06:39:18.0805792+00:00",
               "lastUpdated":"2021-04-02T06:39:18.0652924+00:00",
               "lastUpdated":"2021-04-02T06:39:17.9974255+00:00",
               "lastUpdated":"2021-04-02T06:39:17.7610763+00:00", <- THIS ONE
               "lastUpdated":"2021-04-02T06:39:17.7611023+00:00",
               "lastUpdated":"2021-04-02T06:39:17.7611201+00:00",
               "lastUpdated":"2021-04-02T06:39:17.7605158+00:00"

andy-a-o · 2021-04-02T11:04:15Z

Sorry can't tell you more for now, will be able to check it further on the next week, traveling currently.

For now, you could try to run integration tests locally, using the fresh Mongo db and web instance, running via Docker, just as in the Integration Tests workflow setup here, in this repo.

Try running it on the web instance with- and without changes, apply some of them, run again etc. If you can of course.

jjrdk · 2021-04-03T19:13:30Z

@andy-a-o Thanks for the investigation. I had found the error message with mis-ordered items in the test errors, but had not dug further. Will take a closer look.

…/patch-verb

jjrdk · 2021-04-05T06:50:19Z

I tried reverting the latest commits, but that doesn't seem to help :-(

kennethmyhra · 2021-04-05T06:59:47Z

I think currently we can rule out that there is anything wrong with this PR. Andy and me will have to investigate the integration tests further

kennethmyhra · 2021-04-08T13:37:24Z

Again thanks @jjrdk, much appreciated!

Start support for PATCH verb.

3b72c72

jjrdk mentioned this pull request Mar 1, 2021

PATCH support #302

Closed

Add extension methods to create patches from existing resources.

a1c7dd3

kennethmyhra reviewed Mar 6, 2021

View reviewed changes

src/Spark.Engine/Service/AsyncFhirService.cs Outdated Show resolved Hide resolved

jjrdk added 2 commits March 7, 2021 17:50

Revert change to FHIR response when object not found.

f000c4a

Merge remote-tracking branch 'remotes/origin/r4/master' into features…

dcb58e9

…/patch-verb

jjrdk marked this pull request as ready for review March 8, 2021 13:54

kennethmyhra requested changes Mar 17, 2021

View reviewed changes

jjrdk and others added 3 commits March 18, 2021 11:43

Apply code review changes.

3cda988

Move test data to separate files in TestData/R4.

4546499

Remove Task.Run block in async method

4f1f3fd

kennethmyhra linked an issue Mar 19, 2021 that may be closed by this pull request

PATCH support #302

Closed

jjrdk added 2 commits March 23, 2021 22:55

Handle Code<T> as generic sub type of DataType. Add tests for additio…

0a791c0

…nal types.

Merge branch 'features/patch-verb' of github.com:jjrdk/spark into fea…

1bfc8e8

…tures/patch-verb

kennethmyhra approved these changes Apr 1, 2021

View reviewed changes

Merge remote-tracking branch 'remotes/origin/r4/master' into features…

3c5639c

…/patch-verb

jjrdk force-pushed the features/patch-verb branch from 24df46f to 3c5639c Compare April 5, 2021 06:41

kennethmyhra merged commit 248fe9b into FirelyTeam:r4/master Apr 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Start support for PATCH verb. #341

Start support for PATCH verb. #341

jjrdk commented Mar 1, 2021

jjrdk commented Mar 1, 2021

jjrdk commented Mar 4, 2021

kennethmyhra commented Mar 5, 2021

kennethmyhra commented Mar 17, 2021

kennethmyhra commented Mar 22, 2021

jjrdk commented Mar 22, 2021

jjrdk commented Mar 23, 2021

kennethmyhra commented Mar 29, 2021

jjrdk commented Mar 29, 2021

kennethmyhra commented Apr 1, 2021

jjrdk commented Apr 1, 2021

kennethmyhra commented Apr 1, 2021

kennethmyhra commented Apr 2, 2021

andy-a-o commented Apr 2, 2021

andy-a-o commented Apr 2, 2021

andy-a-o commented Apr 2, 2021

andy-a-o commented Apr 2, 2021

jjrdk commented Apr 3, 2021

jjrdk commented Apr 5, 2021

kennethmyhra commented Apr 5, 2021

kennethmyhra commented Apr 8, 2021

Start support for PATCH verb. #341

Start support for PATCH verb. #341

Conversation

jjrdk commented Mar 1, 2021

jjrdk commented Mar 1, 2021

jjrdk commented Mar 4, 2021

kennethmyhra commented Mar 5, 2021

kennethmyhra commented Mar 17, 2021

kennethmyhra commented Mar 22, 2021

jjrdk commented Mar 22, 2021

jjrdk commented Mar 23, 2021

kennethmyhra commented Mar 29, 2021

jjrdk commented Mar 29, 2021

kennethmyhra commented Apr 1, 2021

jjrdk commented Apr 1, 2021

kennethmyhra commented Apr 1, 2021

kennethmyhra commented Apr 2, 2021

andy-a-o commented Apr 2, 2021

andy-a-o commented Apr 2, 2021

andy-a-o commented Apr 2, 2021

andy-a-o commented Apr 2, 2021

jjrdk commented Apr 3, 2021

jjrdk commented Apr 5, 2021

kennethmyhra commented Apr 5, 2021

kennethmyhra commented Apr 8, 2021