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

Phase 1: Add generic HTTP request #889

Closed
aleksa-krolls opened this issue Jan 9, 2025 · 5 comments · Fixed by #923
Closed

Phase 1: Add generic HTTP request #889

aleksa-krolls opened this issue Jan 9, 2025 · 5 comments · Fixed by #923
Assignees

Comments

@aleksa-krolls
Copy link
Member

aleksa-krolls commented Jan 9, 2025

See parent issue #887 for details.

@martalovescoffee
Copy link

@hunterachieng could you please add an estimate to this issue?

@josephjclark
Copy link
Collaborator

Adjustments to the design:

  • change the existing common request helper so that it does not use instanceUrl
  • Add to the existing common request helper an option called baseUrl, which gets forwarded directly into the common request helper on request options
  • The existing HTTP operations should be modified to pass instanceUrl as baseUrl - ie, they'll each pass in the option.

So far, we haven't changed any behaviour from the user's point of view. Hopefully some tests will break, and we can fix them

Next, in http. add a new request operation which just calls out to the common request helper. It does NOT pass a value for baseUrl

The utils.request signature is probably going to get very long (too many parameters) so you may want to refactor it to request(state, method, path, options), where options is an object like { query, body, baseUrl }

My only doubt about this is: what about pagination? Should the generic request util really handle pagination? The fhir API will want to do it differently. Maybe that's something we can worry about later.

@aleksa-krolls
Copy link
Member Author

aleksa-krolls commented Jan 17, 2025

@josephjclark if the get() in http adaptor does not handle pagination, then we don't need to worry about it for openmrs http.get(). If it does handle pagination, why not add the same pattern here?

@josephjclark
Copy link
Collaborator

@aleksa-krolls the http adaptor does not handle pagination because pagination on every HTTP service is a bit different.

We have implemented "generic" pagination support for for OpenMRS already. But by "generic" we really mean for the Rest API. Other modules, including fhir, could use different pagination semantics. In practice it's probably just fhir we need to worry about.

What I think we'll need to do when we get to the fhir APIs is:

  • update the common request helper with no pagination
  • add a rest API helper which calls common and adds pagination (and feeds the other rest API functions)
  • add a fhir API helper which calls common and adds pagination (and feeds the other FHIR API functions)

It's just a question of moving all the code to the right place so its nice and tidy.

@aleksa-krolls
Copy link
Member Author

ok cool, so as I said... if http does not handle pagination... then no need to here. Just wanted to be consistent, and I'm board with all of your points.

Great if later down the line we want to add enhanced support for the fhir api, but at minimum next week I want to be able to implement the common request helper (and all good if no pagination).

@github-project-automation github-project-automation bot moved this from New Issues to Done in v2 Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants