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

ResourceBinding fails to choose correct method due to SearchMethodBinding matching issue #1421

Closed
tuomoa opened this issue Aug 9, 2019 · 9 comments

Comments

@tuomoa
Copy link
Contributor

@tuomoa tuomoa commented Aug 9, 2019

Hello @jamesagnew,

I already commented on the actual commit (19954fa#r34196172), but decided to make an issue after all.

I think the ALLOWED_PARAMS change b442982 (with change 19954fa) is causing regression in 3.7.0 and later. I think the incomingServerRequestMatchesMethod is not correctly handling underscore prefixed parameters anymore.

We have (for example) two methods in our resource provider, one does not support _include parameter but the other does:

public Bundle searchA(@RequiredParam(name = CarePlan.SP_PATIENT) IdType patientId) { ... }

public Bundle searchB(@RequiredParam(name = CarePlan.SP_PATIENT) IdType patientId, @IncludeParam(..) List<Include> includes) { ... }    

When upgrading to the 3.8.0 the provider is calling the method without _include parameter even though we are using it in the request (we also have other underscore prefixed parameters in search methods, such as _tag). After debugging for a while I think this is accidentally ignoring too many parameters prefixed with underscore. (And therefore when checking if there was unknown params this incomingServerRequestMatchesMethod method ends up returning true instead of false and therefore the ResourceBinding getMethod(RequestDetails theRequest) incorrectly returns the method without _include param).

WDYT?

I think this is also related to the #1373

@tuomoa tuomoa changed the title SearchMethodBinding fails to choose correct method ResourceBinding fails to choose correct method due to SearchMethodBinding matching issue Aug 9, 2019
@ruoat

This comment has been minimized.

Copy link
Contributor

@ruoat ruoat commented Aug 20, 2019

@jamesagnew are there any plans to check this? This blocks our migration from Hapi 3.6.0

@tuomoa

This comment has been minimized.

Copy link
Contributor Author

@tuomoa tuomoa commented Sep 11, 2019

@jamesagnew @dmuylwyk any chance someone could address this issue?

@jamesagnew

This comment has been minimized.

Copy link
Owner

@jamesagnew jamesagnew commented Sep 11, 2019

A pull request with a test demonstrating the issue (or a fix) would be welcomed- I'm not sure I understand what exactly is wrong here.

If you have two methods where the only difference is the absence or presence of an @Include parameter, why not just collapse these into one method and just do an if-null check on the include parameter?

@tuomoa

This comment has been minimized.

Copy link
Contributor Author

@tuomoa tuomoa commented Sep 12, 2019

I'll try to write a unit test for the SearchMethodBinding to explain what's going on here. The @Include parameter is just an example. It looks to me that there is a problem with underscored parameters which are not handled by the methods (and in those cases method shouldn't match because allowUnknownParams is false). And those params are ignored because the next.startsWith("_") in line SearchMethodBinding.java:235. I guess that the if-clause there is trying to ignore parameters such as _format which are handled outside of the Providers?

Well of course it would be possible to circumvent this issue by adding all params to one method but I think it still is a bug in the actual method binding implementation.

@tuomoa

This comment has been minimized.

Copy link
Contributor Author

@tuomoa tuomoa commented Sep 18, 2019

Hello @jamesagnew, I just created a pull request #1492 with a failing test case demonstrating the issue with underscored parameters and binding matching. Let me know what do you think.

@jamesagnew

This comment has been minimized.

Copy link
Owner

@jamesagnew jamesagnew commented Sep 25, 2019

@tuomoa Thanks!

I'm going to commit a fix that corrects this for the specific case you're describing here (_include and _revinclude) as oppossed to changing behaviour more generally.

The underscore params have all kinds of uses, and are often read by filters and interceptors without the SP methods having any knowledge of them, so I'm reluctant to change the general behaviour.. I suspect it would break many people's existing code.

This fix will address your specific use case though.

@jamesagnew

This comment has been minimized.

Copy link
Owner

@jamesagnew jamesagnew commented Sep 25, 2019

Fixed in 7700bb8

@jamesagnew jamesagnew closed this Sep 25, 2019
@tuomoa

This comment has been minimized.

Copy link
Contributor Author

@tuomoa tuomoa commented Nov 27, 2019

Hello @jamesagnew,

Thanks for that fix but I think it's incomplete. If you take a look at https://www.hl7.org/fhir/STU3/search.html#2.21.1.1 there are multiple other underscored parameters such as _tag, _has that are now handler incorrectly. There are also some normal underscored field parameters such as _language (which is also in the special handled set).

Would it be possible to make the handling better to work with those standard parameters as well?

Let me know what do you think.

@jamesagnew

This comment has been minimized.

Copy link
Owner

@jamesagnew jamesagnew commented Nov 27, 2019

Honestly, my recommendation here is just to create a single method that takes all of these parameters and handle your selection logic inside that method.

I realize that it would be nice to have a fancy system that selects between various permutations of all of the meta-parameters and always picks the right one, but this is not trivial. The change you reference in the top post was made in order to fix other issues, and it's really hard to solve this without causing other regressions. Not impossible by any means, but much more work than I am willing to spend on this right now.

I'd be willing to accept a pull request that addresses this of course (provided that it was a clean solution and did not cause other regressions), and as always it's possible that someone with a support contract might file a ticket on this, in which case a dedicated support engineer would take a crack at it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.