-
Notifications
You must be signed in to change notification settings - Fork 30
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
The configuredExpressions variable has wrong values quantity after limitConfiguredExpressionsBySeverity is applied #107
Comments
Thanks, interesting. Hopefully, we can look into this in the next release cycle. If I remember correctly, normally the configured expressions are only loaded once. But, there is a system in place to load them dynamically. Maybe, in order for this to work correctly (multiple sequential validations of the same file using different severity levels and tracking the proper list size), we should be kicking off the dynamic load for every validation regardless of if it has changed or not. Just a different idea off the top of my head without looking into this yet in any way (so may not apply at all). In the meantime, did you want to maybe make a PR of your suggestion above, to the develop branch? Then we can work from that as a baseline (or potentially just merge it as is if all is agreed upon). If not, we can work from this, but that might be more clear. |
I am going to check validation results without dynamic load and compare with site.healthit.gov. As for the PR, yes I will create one or two with different approaches: 1) like in my first message; 2) create map with three lists at the application initialisation process, like we have now for the vocabularyValidationConfigurations bean. |
If it changes to 120 after using error mode, and stays there from then on, do you think that it's only checking errors from then on, regardless of severity selection? To answer my own question, I tested this. Even though vocabularyValidationConfigurationsCount is returned as 120, it still checks for all severity types, if info is selected. I see you've added a PR for a new approach. Which is awesome, thanks! I will review and look into merging. But, what would you say the root of the bug was, in the most direct way? For example, if using the old approach, what change would fix it? Or, is the new approach fully necessary? Maybe we could chat about this sometime. Let me know when you are free. Thanks. |
One day we found that two equal instances give us different results for equal requests after some time. This issue was investigated and I found that the app filters the configuration XML file each time you send a non-info severity level. But if we have 180 configs total we will get only 160 after we send "warning" and 120 after the "error" severity level and this number never will never be updated later. In fact, we have fewer configs in the vocabularyValidationConfigurations list for this file if we repeat the request or for the next files. So the fix I provided filters configs on three lists on the application startup. Tests: Requests: Request:
Response:
Response:
Response:
As you can see: The first request gave us 1 warning and 9 infos for Vocabulary Validation Conformance part.
Equal request number 5 after severity level modifications gave us zero warnings and 5 infos.
|
Thanks again for your work on the PR. I have updated where necessary to merge this code into the code validator and to make it work with the reference ccda validator, fixing minor conflicts along the way. I wonder, did you already do this? Or, do you not use the reference ccda validator? There is one item which is not working as expected though (as it was before). And this is keeping me from merging your work into develop. Maybe you encountered this and have thoughts on how to resolve? Everything is on the branch fixSeverityLevel (branch on reference-ccda-validator and branch on code-validator-api). Of course, if you don't have time, that is of course fine, I will update as needed. I'm not sure if you're looking to use the new project as a baseline, or if you'd be interested in the issue I found. Basically, if we load up RefCCDATest.java in the Ref Val project, and find the vocabAndMdhtSeverityLevelErrorMultipleValidatorsPerExpressionTest test. We will note that that test fails but used to pass. The reason appears to be related to how we are handling multiple validators with one expression. I created some new tests, which work (e.g. vocabAndMdhtSeverityLevelErrorSingleValidatorsPerExpressionTest), to compare, which use a file where the validators are separated, per expression to isolate the discrepancy.
This is what has changed since the old implementation as far as tests passing (after accounting for updating tests to accommodate the new map style): *Fails with new impl, passes with old: Passes still (as it did before): Passes with old and new impl (these are new tests): Again, I want to stress, this is not your job, and I appreciate what you have provided. I am only giving an update. And if interested, check out the fixSeverityLimiting branch on code-validator-api and reference-ccda-validator (the content validator can be develop, master, or a pre-built jar). On another, quite relevant note, interestingly enough, the bug you found where configs are missing over time, does not exist on our old impl on our dev server. This is because the dev server reloads the config every time because it is configured to use a dynamic config. Like so:
Where as if it were set to:
Then it encounters the bug, because it is static, and only loaded once. This is why the bug had escaped us until now, as, we don't test with isDynamicVocab set to false (we don't test with static configs). So, one simple fix to the entire issue without any new code would be to just use the dynamic config (setting isDynamicVocab to true in the referenceccdaservice.xml). And, this fix has been applied to production. So, we no longer experience the bug there. |
See: org.sitenv.vocabularies.validation.services.VocabularyValidationService#validate
The configuredExpressions variable has wrong values quantity in case we want to validate same document more than two times with the different severity level. As a result we can get different validation results.
Validation iterations:
In this case we can add a new list as a local variable and add suitable ConfiguredExpressions to it for each validation process.
Possible fix:
The text was updated successfully, but these errors were encountered: