Patient/$match: Add remaining parameters

[felixwiemuth]

Even if they are not used, it simplifies using the parameters when overriding the match function.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.56%. Comparing base (406db33) to head (4c52c94).
Report is 92 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #6447      +/-   ##
============================================
+ Coverage     83.54%   83.56%   +0.02%     
- Complexity    27432    27785     +353     
============================================
  Files          1707     1739      +32     
  Lines        106185   107472    +1287     
  Branches      13397    13489      +92     
============================================
+ Hits          88710    89811    +1101     
- Misses        11750    11877     +127     
- Partials       5725     5784      +59     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tadgh]

Heya Felix, thanks for the contribution! I'm having some trouble understanding the purpose of the MR. Is it that you have an underlying service which wants to make use of these parameters in your own implementation? What do you mean when you say it simplifies the parameters? If you want to make use of other parameters, instead of overriding this class, why not replace the provider entirely?

This adds two parameters that are effectively vestigial in our implementation, which could lead to confusion to users who expect them to do something (since they will be published automatically in our docs).

I'm not opposed to this necessarily, but if we were to add these parameters, I would require a few small changes:

1. Implement all the params from the spec, meaning onlySingleMatch as well.
2. In this provider implementation, throw a 400 if any of the new vestigial parameters are introduced, with explanatory message saying these are not yet supported. I would rather fail explicitly, than have users assume that those parameters are doing something.

I actually don't know the answer to this offhand, but if you just wrote a separate method which bound to the same name, using different parameters, does the method binder correctly route it to the new method? If so, it's probably a better solution to have your own overriding provider implement these new parameters, and have our method delegate to yours in your provider, e.g. something like

@Operation("$match")
match(a,b) {
  return match(a,null,null,b)
}

@Operation("$match")
match (a,b,c,d) {}

That said, I don't know if our method binder would route that correctly, probably worth checking as I think that solution is cleaner from a platform perspective. If our binder doesnt route that correctly, then I think we could take this MR with the above changes (1+2 )made.

[felixwiemuth]

Hi @tadgh, thanks for your detailed comment, and sure I can provide further information why I think this is important.

Use case and reason for suggested change

Our use case is the following: We use MdmProviderDstu3Plus, as it provides useful functionality. However, we do have our own implementation of the Patient/$match operation. So we extend the MdmProviderDstu3Plus and override the match method. This usually works fine, however, one cannot change the method signature when overriding, so if HAPI's implementation is not declaring all parameters, we have to retrieve the parameters manually from the requestDetails, plus we have to manually add validation which would have been done by the @OperationParam annotation (e.g. @OperationParam(name = ProviderConstants.MDM_MATCH_COUNT, min = 0, max = 1, typeName = "integer")).

Regarding adding parameters which are not used

You are right regarding that providing parameters that are not used is confusing. On the other hand, as the standard specifies them, it's also fair that they are present.
I think it is a good idea to return an error when a parameter is provided that is not being considered by the implementation. Alternatively, it could be an OperationOutcome with a warning or error issue.

Probably eventually HAPI's example implementation should also support the parameters, even though the implementation as a whole is quite rudimentary, as it is based only on a parts of what MDM matching does (as opposed to an actual scoring, which is complex).

Regarding

1. Implement all the params from the spec, meaning onlySingleMatch as well.

I can see that onlySingleMatch is added in the current draft of FHIR R6 (but it is not present in R4 or R5) - do you want to add it already anyway?

Regarding annotating another method with the same @Operation

I actually had tried that approach, but it seems that always the parent class's method would be called.

Also, it's probably not a good idea for HAPI to decide dynamically based on the provided parameters which overloaded method to route to.

One could of course consider changing the routing so that if there are multiple @Operation annotations for the same operation in a class hierarchy, the lowest in the hierarchy takes precedence. This would add a robust mechanism to deal with use cases like the one described above, for any operation in general. At the same time, it should probably be forbidden to have multiple @Operation for the same operation in one class.

Apart from that, it is probably still preferable to have all parameters as according to the spec, because it saves work and error-sources due to manually replicating the spec with cardinalities etc.

Additional discussion regarding match being part of MdmProviderDstu3Plus

Generally, it is actually also not optimal that match lives in a provider that is strongly related to MDM, and probably should only be loaded when MDM is enabled, as the Patient/$match operation also makes sense without MDM.

To provide it in both cases, we have to redirect to the actual implementation both from the MdmProviderDstu3Plus-extension (enabled when MDM is enabled) and from an extra provider added specifically for this (loaded when MDM is disabled).

[tadgh]

If you are talking about Patient/$match, that is now implemented in PatientMatchProvider, which is divorced from MdmDstu3PlusProvider, as of this pull request: #6503. Is your fork perhaps out of date? This was ~45 days ago we changed this.

Given that, and the fact that PatientMatchProvider.java is only 50 lines long, it's probably easier for you to just write your own and load that? Alternatively, rework your pr to:

1. Add the new parameters to this PatientMatchProvider.
2. Throw 400 if an unsupported param is used.

And yeah, you can omit the R6-only parameter, that's fine.

[felixwiemuth]

Good to hear that Patient/$match was separated into a different provider and yes, that will allow us to just have our own provider. (That update simply wasn't there yet when I worked on this.)

I'll update the PR as discussed.

I think I'll open a separate issue regarding @Operation routing for the case where there are multiple methods with the same annotation.

[tadgh]

Sounds great, thanks! @me when its ready for review, i may not see it otherwise.