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

Extend or update the Search DSL 'has' function to include resource type as parameter #1921

Closed
ellykits opened this issue Mar 20, 2023 · 8 comments · Fixed by #1927
Closed

Comments

@ellykits
Copy link
Collaborator

ellykits commented Mar 20, 2023

Is your feature request related to a problem? Please describe.
In FHIRCore we would want to use the nested search DSL has function however we do not know the type of the resource at the time of the execution; the current Search#has function is generic and expects a resource type.

Describe the solution you'd like
Implement an overloaded extension function that accepts resource type as a parameter without affecting the existing implementation.

Describe alternatives you've considered
As an alternative we can update the existing Search#has function to accept resourceType as a parameter instead of using generic types.

Additional context
To offer more flexibility the recommendation is to update or implement a new function with the following signature

fun Search.has(
  resourceType: ResourceType,
  referenceParam: ReferenceClientParam,
  init: Search.() -> Unit
) 

Here is the current

inline fun <reified R : Resource> Search.has(
  referenceParam: ReferenceClientParam,
  init: Search.() -> Unit
) 

Would you like to work on the issue?
Yes

@f-odhiambo
Copy link
Collaborator

@omarismail94 Do you mind taking a lot at this and approving if it is something we can work on resolving? Thanks!

@omarismail94
Copy link
Contributor

Im adding @aditya-07 as he wrote the code for this. Aditya - is there a reason we went with a reified function as opposed to the signature that @ellykits is suggesting? I think what Elly is suggesting makes sense, but wanted to see what you thought as well!

@aditya-07
Copy link
Collaborator

@omarismail94 There wasn't any particular reason to make the function reified. @ellykits @f-odhiambo Please feel free to add generic has api.

@ellykits
Copy link
Collaborator Author

Thanks @aditya-07. This is noted.

@jingtang10
Copy link
Collaborator

@ellykits @f-odhiambo @pld can you please elaborate a bit more on how you're using this new API?

I'm guessing you have some kind of configuration that defines a search by specifying its resource type and search params and that's why the type resolution must be done at runtime?

@jingtang10 jingtang10 reopened this Mar 22, 2023
@ellykits
Copy link
Collaborator Author

ellykits commented Mar 22, 2023

@jingtang10 In our JSON config we declare the resource type as a property see the example below:

Example config for searching Patients having some Condition

{
  "resource": "Patient",
      "nestedSearchResources": [
          {
            "resourceType": "Condition"
            // Condition resource filters
         }
      ]
}

The configuration can be used in other use cases e.g. fetching Patients with particular Observation. My PR makes it possible to provide the resource type as a parameter to the Search#has function so we can use the same implementation for different resource types with similar relationships. The existing implementation uses reified generic function that requires an explicit resource type.

@ellykits
Copy link
Collaborator Author

The implementation can be compared to the overloaded FhirEngine#get function

(with no resource type as parameter)

suspend inline fun <reified R : Resource> FhirEngine.get(id: String): R 

and this

(with resource type as parameter)

suspend fun get(type: ResourceType, id: String): Resource

@jingtang10
Copy link
Collaborator

jingtang10 commented Mar 31, 2023

@omarismail94 There wasn't any particular reason to make the function reified. @ellykits @f-odhiambo Please feel free to add generic has api.

reified is needed if at runtime (EDIT: compile time) you need to access the type information of the generic class.

But that's not really relevant in this issue.

Thanks @ellykits for providing the examples. Makes sense.

EDIT: just to add, I think about reified as "compile time generics"... it's sort of like fake generics that is essentially inlined versions of the same code with different types...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Complete
Development

Successfully merging a pull request may close this issue.

5 participants