use min validator on core tags for latest api #3056
Conversation
… to fail a post - continue to use fo-dicom validator to generate warnings, but continue to omit warning when is valid string aside from being padded with null values
…sses minimum validator but not fo-dicom validator
| @@ -188,6 +187,27 @@ public async Task GivenV2Enabled_WhenNonRequiredTagNull_ExpectTagValidatedAndNoE | |||
| Assert.Empty(result.InvalidTagErrors); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
this is the test that fails in main branch on core tag uid validation where the uid validation passes minimum validator but not fo-dicom validator
|
|
||
| ValidationWarnings warning = ValidationWarnings.None; | ||
| if (dicomElement != null) | ||
| { | ||
| if (dicomElement.ValueRepresentation != queryTag.VR) | ||
| if (dicomElement.ValueRepresentation != dicomTag.GetDefaultVR()) |
There was a problem hiding this comment.
nit: should we store the VR, rather than calling twice
| /// <exception cref="ElementValidationException"/> | ||
| void Validate(DicomElement dicomElement); | ||
| void Validate(DicomElement dicomElement, bool withLeniency = false); |
There was a problem hiding this comment.
I think this should perhaps be an enum indicative of some sort of "ruleset", like:
| void Validate(DicomElement dicomElement, bool withLeniency = false); | |
| void Validate(DicomElement dicomElement, ValidationRules rules = ValidationRules.Default); // Or maybe "Strict"? |
| { | ||
| // validate with additional leniency | ||
| var validationWarning = | ||
| dicomDataset.ValidateDicomTag(requiredCoreTag, _minimumValidator, withLeniency: true); |
There was a problem hiding this comment.
What is the reason for this new boolean? I see ValidateDicomTag called from 2 places, one is STOW and other in Re-index. We can have the same in re-indexing too right?
There was a problem hiding this comment.
none of us like this with Leniency flag- Monday I will encapsulate this within some type of rules/enum/something
src/Microsoft.Health.Dicom.Core/Features/Validation/PartitionNameValidator.cs
Show resolved
Hide resolved
| @@ -250,15 +304,15 @@ string EnsureRequiredTagIsPresent(DicomTag dicomTag) | |||
| } | |||
| catch (DicomValidationException ex) | |||
| { | |||
| validationResultBuilder.Add(ex, item.Tag, isCoreTag: RequiredCoreTags.Contains(item.Tag)); | |||
| validationResultBuilder.Add(ex, item.Tag, isCoreTag: false); | |||
There was a problem hiding this comment.
Is the implication here that the query tags cannot include core tags?
There was a problem hiding this comment.
QueryTags can include since we validate the core tags in the previous step and I think it should still choose to check for isCoreTag
Description
Related issues
Addresses [AB#109809].
Testing
Added and updated tests. Added tests around expectations for in/valid core tags like patient id and uids