FHIR Chat · Host Validator in HAPI · hapi

Stream: hapi

Topic: Host Validator in HAPI

Grahame Grieve (Jan 17 2020 at 00:05):

The new Validator I'm just releasing raises a couple of issues for those people hosting it in HAPI

Grahame Grieve (Jan 17 2020 at 00:08):

firstly, the validator will now consistently hunt down and validate resources that are referred to. if you implement IValidatorResourceFetcher, be aware that this will just keep hunting references and crawl through your resource web much more consistently than before, until you return false for resolveURL, or until it runs out of references to follow.

I'm considering putting some depth limit on the engine as a configurable setting, but I don't know if I need to. Is this going to be a problem for anyone (note that the issue isn't new, but due to a number of bugs, the validator was inconsistent in following the references, so it may not have been as much of a problem)

Grahame Grieve (Jan 17 2020 at 00:09):

secondly, the validator now caches the results returned from IValidatorResourceFetcher. Quite aggressively. If this turns into a resource hog, we can look at whether it should be quite so aggresive - but there's several obscure but real bugs that can occur if the responses to IValidatorResourceFetcher aren't consistent

Grahame Grieve (Jan 17 2020 at 00:10):

there's also a new api IValidationProfileUsageTracker that allows you to track the usage of structure definitions down inside the validator if you want to do stuff like that

Jens Villadsen (Jan 17 2020 at 09:16):

@Grahame Grieve
Question 1) Is type check of References using targetProfile's base type still not included? (if not, I will raise a PR that does it)
Question 2) Could depth be an argument to Element fetch(Object appContext, String url) or where do you intend to put it
Question 3 - this is more of a consideration) I my setup we rely on validation split into to phases. First phase is to run the validator using regular handles in HAPI in an interceptor (this is where I want type checking). Only if this step passes, we go on to resolve all external references. I imagine this split has a small performance penalty, but the design is simple and easy to understand. I do hope you provide some feature toggles that allow us to continue the way we do it now, before we should consider to merge the two validation phases into one, using only the new Validator and our custom implementation of IValidatorResourceFetcher.

Grahame Grieve (Jan 17 2020 at 10:37):

1) - I'm not sure - be specific. What gets checked depends on what you return in validationPolicy (nothing, type, exist, everything)

2). maybe. I'll about how I'd track that

3) I'm not sure how you have that set up, but I'm not changing the API around this, so it shouldn't stop you from doing that. I'm just alerting people that the behaviour is going to change across the API

Jens Villadsen (Jan 18 2020 at 21:24):

@Grahame Grieve - Please have a look at https://github.com/jvitrifork/fhirReferenceCheck. It illustrates both problems and solutions ;)

Grahame Grieve (Jan 18 2020 at 21:31):

I don't see why removing checking the type is a good way forward. Please explain your logic

Grahame Grieve (Jan 18 2020 at 21:32):

and if you have tests to submit, why not submit them to fhir-test-cases?

Jens Villadsen (Jan 18 2020 at 21:37):

if you debug it, you'll see that the type of the reference is always null

Grahame Grieve (Jan 18 2020 at 21:39):

you're referring to the test on 1923?

Jens Villadsen (Jan 18 2020 at 21:39):

yep

Grahame Grieve (Jan 18 2020 at 21:40):

so because you're not setting reference.type, you don't think anyone else should?

Jens Villadsen (Jan 18 2020 at 21:42):

I interact with the validator indirectly - through the use of the HAPI classes. If type should be set, I would expect it to happen through those classes then

Jens Villadsen (Jan 18 2020 at 21:43):

Could you tell me how to set the type through the use of the HAPI classes then?

Jens Villadsen (Jan 18 2020 at 21:43):

only using the handles provided in the unit test

Jens Villadsen (Jan 18 2020 at 21:44):

so because you're not setting reference.type, you don't think anyone else should?

If im not mistaken - my use represents an ordinary use of the validator using HAPI, so its actually not just me - I suspect this is way all HAPI users use it

Jens Villadsen (Jan 18 2020 at 21:47):

@Grahame Grieve but of course I could be wrong. Please correct me in any false assumptions here

Grahame Grieve (Jan 18 2020 at 21:48):

reference.type is http://hl7.org/fhir/references-definitions.html#Reference.type - so you would set that by your normal use of the HAPI model classes, or in json or xml etc. I'm not sure what you're saying here

Jens Villadsen (Jan 18 2020 at 21:58):

I can't see how I can set the type using https://hapifhir.io/hapi-fhir/apidocs/hapi-fhir-structures-dstu3/org/hl7/fhir/dstu3/model/Reference.html

Grahame Grieve (Jan 18 2020 at 21:58):

it was only introduced in R4

Jens Villadsen (Jan 18 2020 at 21:59):

so my tests are running on dstu3

Grahame Grieve (Jan 18 2020 at 21:59):

so?

Jens Villadsen (Jan 18 2020 at 22:00):

so don't run the type check the same way in dstu3 as in R4 or newer

Grahame Grieve (Jan 18 2020 at 22:01):

so in DSTU3, type will always be null, so it will never run.

Jens Villadsen (Jan 18 2020 at 22:11):

the lines 1923 to 1942 were removed because I could see they were never used ... that part makes sense now

Jens Villadsen (Jan 18 2020 at 22:12):

now, for test case validateReferenceAggregationMode_ContainedOrReferenced_withReferenced:

Jens Villadsen (Jan 18 2020 at 22:14):

Element we gets assigned the value of null

Jens Villadsen (Jan 18 2020 at 22:14):

line 1892

Grahame Grieve (Jan 18 2020 at 22:15):

ok?

Jens Villadsen (Jan 18 2020 at 22:15):

so the entire block from 1945 by default ends up not being run

Jens Villadsen (Jan 18 2020 at 22:16):

that block actually does the validation part as wanted

Jens Villadsen (Jan 18 2020 at 22:16):

(I have little idea about what weis)

Jens Villadsen (Jan 18 2020 at 22:17):

the same null check (which is redundant on we) comes again in 1964

Jens Villadsen (Jan 18 2020 at 22:20):

The policy (pol) seems to be controllable by the fetcher- I just haven't found a nice way to assign the fetcher using the briding code from HAPI

Jens Villadsen (Jan 18 2020 at 22:21):

so since we is null and I cannot control the policy (by using the fetcher) I need to disable line 1944

Jens Villadsen (Jan 18 2020 at 22:21):

same goes for 1964

Jens Villadsen (Jan 18 2020 at 22:27):

and if weis null, then I hit the case from line 2019 and to 2035 - ending in getting an error that says "Bundled or contained reference not found within the bundle/resource "

Jens Villadsen (Jan 18 2020 at 22:27):

which makes little sense to me

Jens Villadsen (Jan 18 2020 at 22:30):

@Grahame Grieve so ... am I on to something?

Jens Villadsen (Jan 18 2020 at 22:33):

(https://github.com/FHIR/fhir-test-cases explicitly mentions "no java code here") - I would have a harder time presenting this finding not using unit tests code and class shadowing

Grahame Grieve (Jan 18 2020 at 23:07):

I’m on the road so I’ll look later but you have to be able to set the fetcher - it’s critical. So that’s a HAPI issue

Grahame Grieve (Jan 18 2020 at 23:08):

As for tests... sounds like you have a problem with how the validator is hosted in HAPI, not with the actual validation itself. I won’t make fixes the validator unless the problem is related to the actual spec, with test cases

Grahame Grieve (Jan 18 2020 at 23:08):

There are test cases around the hosting in HAPI...

Jens Villadsen (Jan 18 2020 at 23:10):

the webeing null and the block removed in 2019-2035 is not related to HAPI -

Jens Villadsen (Jan 18 2020 at 23:10):

the double null check on weis not related to HAPI

Jens Villadsen (Jan 18 2020 at 23:15):

and if the fetcher is actually accessible and letting me set the policy, then I'm eager to know what is going to happen here: :slight_smile:

if (pol == ReferenceValidationPolicy.CHECK_VALID) {
      // todo....
    }

Grahame Grieve (Jan 19 2020 at 05:44):

so am I

Grahame Grieve (Jan 19 2020 at 05:44):

I'm not sure what your point is on the other things

Jens Villadsen (Jan 19 2020 at 06:26):

I won’t make fixes the validator unless the problem is related to the actual spec, with test cases

those other points AFAIK are related to pure code, not spec

Grahame Grieve (Jan 19 2020 at 07:20):

I'm still not sure what your issue is other than not having access to fetch

Jens Villadsen (Jan 19 2020 at 11:13):

we != Null is checked twice

Grahame Grieve (Jan 19 2020 at 19:04):

this is a logic problem?

Jens Villadsen (Jan 19 2020 at 19:42):

no

Jens Villadsen (Jan 19 2020 at 19:43):

I believe you are correct in your observation regarding the access to assigning the fetcher

Jens Villadsen (Jan 22 2020 at 13:40):

Now I know at least one of the reasons why you ended up adding type to Reference ;) awesome

Jens Villadsen (Jan 22 2020 at 13:46):

(I was just about to make a distinct FHIRpath expression selecting on type on reference in STU3 - which is sort of not possible )

Jens Villadsen (Jan 22 2020 at 13:47):

@Grahame Grieve will you be talking to James about access to the fetcher through the current bridging classes or will it be up one self?

Grahame Grieve (Jan 22 2020 at 19:13):

You, I think

Jens Villadsen (Jan 30 2020 at 08:32):

for future reference and so others can follow: https://github.com/jamesagnew/hapi-fhir/pull/1691

Last updated: Apr 12 2022 at 19:14 UTC