OpenFn / adaptors

The new home for OpenFn adaptors; re-usable connectors for the most common DPGs and DPI building blocks.
GNU General Public License v3.0
7 stars 8 forks source link

`fhir`: implement http for generic `get` and `post` methods #622

Closed aleksa-krolls closed 5 months ago

aleksa-krolls commented 5 months ago

Request

For the Jembi hackathon tomorrow, I really want to showcase a workflow that can send basic GET and POST requests to a demo HAPI FHIR API. (Ideally, this can be ready for testing later today!)

Please update the existing fhir adaptor to include HTTP for generic get and post methods.

(FYI eventually we'll need put and delete as well, but pls just start with get/post.)

To test

I recommend testing with the patient resource to test. See the docs linked below for sample endpoints and payloads.

Screenshot 2024-06-12 at 10 15 10 PM
aleksa-krolls commented 5 months ago

hey @hunterachieng I'm sneaking this into the sprint, because I really want to have this tomorrow for the Jembi hackathon! Can you please prioritize this issue above all else in the morning and see if you can help me release a new adaptor version that supports this today? If not, I can continue to use http... but this is a big opportunity to showcase OpenFn + FHIR workflows and I need this to support our use case. cc @josephjclark

hunterachieng commented 5 months ago

Hi @aleksa-krolls I will prioritise it today

hunterachieng commented 5 months ago

@aleksa-krolls fhir already has a generic get and we can use it for getting patients

josephjclark commented 5 months ago

Haha I just came here to post the same thing: https://docs.openfn.org/adaptors/packages/fhir-docs#get

josephjclark commented 5 months ago

@hunterachieng let's raise a PR for post too

We should not rebase on the new helpers for the Hackathon, it's a bit too much work, but we can prioritise the work for later for a cleaner, crisper experience

@aleksa-krolls you'll see this late I know, but is there anything wrong with the existing get? @mtuchi I don't suppose you've used FHIR's get function?

hunterachieng commented 5 months ago

@josephjclark I see we have a create as well but I can add a post too

mtuchi commented 5 months ago

The current get() for fhir should work as expected and create() works as a post()

josephjclark commented 5 months ago

Perfect, thanks! Can we just add post as a wrapper/alias for create then?

aleksa-krolls commented 5 months ago

@mtuchi @hunterachieng okay if create() works as post(), then we should be fine for now - but if there was a way to quickly add a post wrapper, that might be nice/easier for folks to understand

hunterachieng commented 5 months ago

Do you mean I switch the name from create to post or a wrapper that is built on top of create?

josephjclark commented 5 months ago

@hunterachieng wrap on top of create

Something like:

export function post(...args) {
   return create(...args)
}

You'll need to add basic docs on top, but it's OK to say "see Create docs for more details"

aleksa-krolls commented 5 months ago

@hunterachieng defer to Joe on approach. In my demo job, I would like to be able to have a job that looks like this... so not sure if we can simply rename create to post or what...

post('/Patient/$validate', {
   body: state.data,
   headers: {'content-type': 'application/json'},
 }); 

However, heads up that the HAPI FHIR demo server (https://hapi.fhir.org/baseR4/) is down - if that means you won't be able to work on this, then move this issue to blocked and we can pick it back up later when the server is back up.

hunterachieng commented 5 months ago

@aleksa-krolls I noticed the same on the server downtime. I will revert back to it when its back online

aleksa-krolls commented 5 months ago

hey @hunterachieng @josephjclark if there was any way to release a version of the fhir adaptor tomorrow AM (before 12p SAST) that supported running this job below... that would be absolute 🔥. I have this running in http, but it would be so great if I could show the fhir logo instead. Right now, the fhir's create() function doesn't support all of the options I am using with the http post() function.... lmk if that would be possible.

In this fhir adaptor, either the create() function needs to be updated or I want a new post() to support the following:

  1. POST to any api endpoint, incl. '/Patient'
  2. Option to specify and change the header content-type as needed
  3. Support these http RequestOptions, especially errors and parseAs
  4. Be able to add a callback so that I can manipulate and log the response
post("Patient", {
 body: state.data,  
 headers: {'content-type': 'application/json'}, 
 errors: { 400: false },    
 parseAs: 'json',
 }, state => {
  state.data.response = state.response; 
  return state; 
 }
)

Note: I know HAPI FHIR demo server has been really slow - but testing with this other FHIR Demo server should work

For testing - see here for a payload you can use to test this.

hunterachieng commented 5 months ago

@aleksa-krolls I shall get started on that and hopefully have it ready before 12pm SAST

aleksa-krolls commented 5 months ago

@hunterachieng that would be so cool! Thank you 😁😁😁

josephjclark commented 5 months ago

@hunterachieng if we wanna support those RequestOptions we're gonna have to use the new http utils

@aleksa-krolls I'm a little uncomfortable rolling this out in such a rush. This issue is now "rewrite the fhir adaptor to use use new http get"

Hunter might be able to hack a solution together in the morning - maybe - but even that means we're putting out a pretty flaky build of that adaptor.

Let me take a closer look at the current implementation

josephjclark commented 5 months ago

Ok, having said that, I think we can do this

So with a few tweaks this will work:

post("Patient", {
 body: state.data,  
 headers: {'content-type': 'application/json'}, 
 throwOnError: false
 })

And you'll be able to do state.response.ok, which is true if the response was 2xx or 3xx, or false otherwise

I'm happy to create a post alias to streamline this a bit.

I would like to properly port this over to new HTTP in the near future though, which means thinking a little bit about throwOnError and the response handling.

josephjclark commented 5 months ago

Right, I've spiked out a solution here https://github.com/OpenFn/adaptors/pull/628

I think it'll be fine :tada:

@hunterachieng in the morning please fix any unit tests if you can/need to (I'm sure you will), and please try and test against this build according to AK's notes above.

I'll be online at 8am UTC to support

hunterachieng commented 5 months ago

Thank you @josephjclark I will get right to it first thing in the morning

hunterachieng commented 5 months ago

@aleksa-krolls I have implemented the feature and its ready for review cc @josephjclark

josephjclark commented 5 months ago

@aleksa-krolls If I understand correctly, you're calling out to a FHIR server that uses content type application/json, not application/fhir+json. Is that correct? Is that typical?

josephjclark commented 5 months ago

@aleksa-krolls This should work for you in the new 4.0.0

post("Patient", {
  body:$.data,  
  throwOnError: false
})

You shouldn't need parseAs or custom headers (if you do, just pass them on to the options)

The result will be written to state.data. The response will be written to state.response. Use state.response.ok and state.response.status. The response is a fetch response object - docs