Fixes an NPE and other issues in ViewDefinition validator

[bashir2]

When adding new examples to the SQL-on-FHIR-v2 repo, the validator was failing with the NPE below. I have tried to fix this and some other minor issues in this PR. Also some TODOs are added for issues I found but did not fix.

To test, I built publisher.jar in fhir-ig-publisher locally and ran on my SoF changes.

java.lang.NullPointerException: Cannot invoke "java.lang.Boolean.booleanValue()" because "this.complexTypes" is null
        at org.hl7.fhir.r5.utils.sql.Validator.checkColumn(Validator.java:349)
        at org.hl7.fhir.r5.utils.sql.Validator.checkSelect(Validator.java:145)
        at org.hl7.fhir.r5.utils.sql.Validator.checkViewDefinition(Validator.java:111)
        at org.hl7.fhir.validation.instance.InstanceValidator.checkSpecials(InstanceValidator.java:5718)
        at org.hl7.fhir.validation.instance.InstanceValidator.startInner(InstanceValidator.java:5654)
        at org.hl7.fhir.validation.instance.InstanceValidator.start(InstanceValidator.java:5391)
        at org.hl7.fhir.validation.instance.InstanceValidator.validateResource(InstanceValidator.java:7246)
        at org.hl7.fhir.validation.instance.InstanceValidator.validate(InstanceValidator.java:986)
        at org.hl7.fhir.validation.instance.InstanceValidator.validate(InstanceValidator.java:823)
        at org.hl7.fhir.validation.instance.InstanceValidator.validate(InstanceValidator.java:750)
        at org.hl7.fhir.igtools.publisher.Publisher.validate(Publisher.java:7194)
        at org.hl7.fhir.igtools.publisher.Publisher.validate(Publisher.java:7487)
        at org.hl7.fhir.igtools.publisher.Publisher.validate(Publisher.java:7139)
        at org.hl7.fhir.igtools.publisher.Publisher.createIg(Publisher.java:1183)
        at org.hl7.fhir.igtools.publisher.Publisher.execute(Publisher.java:1018)
        at org.hl7.fhir.igtools.publisher.Publisher.main(Publisher.java:13036)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 12.56%. Comparing base (4603b3a) to head (263d92e).
Report is 8 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##             master    #1737   +/-   ##
=========================================
  Coverage     12.55%   12.56%           
- Complexity    32416    32423    +7     
=========================================
  Files          2204     2204           
  Lines        674087   674118   +31     
  Branches     198717   198726    +9     
=========================================
+ Hits          84657    84679   +22     
- Misses       559352   559353    +1     
- Partials      30078    30086    +8     

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

[grahamegrieve]

why the changes to acceptArrays, acceptComplexTypes, needsName ?

[grahamegrieve]

I still have to figure out what to do with the test cases

[bashir2]

why the changes to acceptArrays, acceptComplexTypes, needsName ?

If you mean the name changes, it is just to clarify what a true value for each mean. If you mean the type change, it is to prevent similar NPEs in future and remove requirements for null checks. In general, it did not seem to me that being able to set these to null adds any value and boolean is sufficient (but please let me know if that is not the case).

[grahamegrieve]

the name changes are fine- an improvement. But it does make a difference to the validator whether the value of these attributes is known or not; sometimes the validation happens in the absence of a particular implementation, and so the validation changes

[grahamegrieve]

also I sorted out what to do with the test cases, but that created more conflicts - sorry

[bashir2]

the name changes are fine- an improvement. But it does make a difference to the validator whether the value of these attributes is known or not; sometimes the validation happens in the absence of a particular implementation, and so the validation changes

Okay, I reverted back to Boolean but TBH it feels a tri-state enum is needed here (e.g., IGNORE/WARNING/ERROR) instead of relying on null values for Boolean.

BTW, in the latest release of the publisher tool (after recent merged changes), now I see a lot more examples of mis-recognizing collections, e.g., the error below for this resource (I updated the collection TODO accordingly but did not debug further):

The column 'gender' appears to be a collection based on it's path. Collections are not supported in all execution contexts

[grahamegrieve]

I talked to @dotasek about it, and we prefer a tri-state, though the tri-state is "yes | no | unknown" since the messages change more than just severity. Do you want to do this in this PR, or do you me to do it?

As for the gender column, I'm not sure why that's being called a collection, but we need to split that issue out from this PR into a separate discussion.

[bashir2]

I talked to @dotasek about it, and we prefer a tri-state, though the tri-state is "yes | no | unknown" since the messages change more than just severity. Do you want to do this in this PR, or do you me to do it?

Okay I made some changes. I started by a tri-state enum but then it seemed to me using IssueSeverity enum makes more sense. That way, the user of the Validator class can fully control the severity level that each type of error is reported, or turn the check off by using IssueSeverity.NULL.

I also made the decision logic more consistent. For example before, needsName being null was still producing some hint messages.

And finally I made all three check flags similar in terms of being on/off (e.g., instead of acceptArrays/needsName we have checkArrays/checkNames.

PTAL and let me know if you prefer the tri-state approach and I add a new enum.

As for the gender column, I'm not sure why that's being called a collection, but we need to split that issue out from this PR into a separate discussion.

Right, I just mentioned that as an FYI; I saw those new errors after I synced my repo to HEAD.

[bashir2]

@grahamegrieve did you have a chance to look at my last commit? Is there anything else you like to be done for this change?

[grahamegrieve]

you didn't do it the way we said. So closing this, see #1773