Update QIDO to respect QueryStatus and header to include Erroneous at…

…tribute (#1061)

Update QIDO to respect QueryStatus and header to include Erroneous attribute

src/Microsoft.Health.Dicom.Api.UnitTests/Extensions/HttpResponseExtensionsTests.cs

@@ -16,7 +16,7 @@ namespace Microsoft.Health.Dicom.Api.UnitTests.Extensions
     public class HttpResponseExtensionsTests
     {
         [Fact]
-        public void AddLocationHeader_GivenNullArguments_ThrowsArgumentNullException()
+        public void GivenNullParameters_WhenAddLocationHeader_ThenThrowsArgumentNullException()
         {
             var context = new DefaultHttpContext();
             var uri = new Uri("https://example.host.com/unit/tests?method=AddLocationHeader#GivenNullArguments_ThrowException");
@@ -29,7 +29,7 @@ public void AddLocationHeader_GivenNullArguments_ThrowsArgumentNullException()
         [InlineData("https://absolute.url:8080/there%20are%20spaces")]
         [InlineData("/relative/url?with=query&string=and#fragment")]
         [SuppressMessage("Design", "CA1054:URI-like parameters should not be strings", Justification = "XUnit more easily leverages compile-time values.")]
-        public void AddLocationHeader_GivenValidArguments_AddsHeader(string url)
+        public void GivenValidLocationHeader_WhenAddLocationHeader_ThenShouldAddExpectedHeader(string url)
         {
             var response = new DefaultHttpContext().Response;
             var uri = new Uri(url, UriKind.RelativeOrAbsolute);
@@ -41,5 +41,32 @@ public void AddLocationHeader_GivenValidArguments_AddsHeader(string url)
             Assert.Single(headerValue);
             Assert.Equal(url, headerValue[0]); // Should continue to be escaped!
         }
+
+        [Fact]
+        public void GivenNullParameters_WhenTryAddErroneousAttributesHeader_ThenThrowsArgumentNullException()
+        {
+            var context = new DefaultHttpContext();
+            Assert.Throws<ArgumentNullException>(() => HttpResponseExtensions.TryAddErroneousAttributesHeader(null, Array.Empty<string>()));
+            Assert.Throws<ArgumentNullException>(() => HttpResponseExtensions.TryAddErroneousAttributesHeader(context.Response, null));
+        }
+
+        [Fact]
+        public void GivenEmptyErroneousTags_WhenTryAddErroneousAttributesHeader_ThenShouldReturnFalse()
+        {
+            var context = new DefaultHttpContext();
+            Assert.False(HttpResponseExtensions.TryAddErroneousAttributesHeader(context.Response, Array.Empty<string>()));
+        }
+
+        [Fact]
+        public void GivenNonEmptyErroneousTags_WhenTryAddErroneousAttributesHeader_ThenShouldAddExpectedHeader()
+        {
+            var context = new DefaultHttpContext();
+            var tags = new string[] { "PatientAge", "00011231" };
+            Assert.True(HttpResponseExtensions.TryAddErroneousAttributesHeader(context.Response, tags));
+
+            Assert.True(context.Response.Headers.TryGetValue(HttpResponseExtensions.ErroneousAttributesHeader, out StringValues headerValue));
+            Assert.Single(headerValue);
+            Assert.Equal(string.Join(",", tags), headerValue[0]); // Should continue to be escaped!
+        }
     }
 }

src/Microsoft.Health.Dicom.Api/Controllers/QueryController.cs

@@ -171,6 +171,7 @@ public async Task<IActionResult> QueryForInstancesInSeriesAsync([FromRoute] stri
 
         private IActionResult CreateResult(QueryResourceResponse resourceResponse)
         {
+            Response.TryAddErroneousAttributesHeader(resourceResponse.ErroneousTags);
             if (!resourceResponse.ResponseDataset.Any())
             {
                 return NoContent();

src/Microsoft.Health.Dicom.Api/Extensions/HttpResponseExtensions.cs

@@ -4,6 +4,7 @@
 // -------------------------------------------------------------------------------------------------
 
 using System;
+using System.Collections.Generic;
 using EnsureThat;
 using Microsoft.AspNetCore.Http;
 using Microsoft.Net.Http.Headers;
@@ -12,12 +13,27 @@ namespace Microsoft.Health.Dicom.Api.Extensions
 {
     internal static class HttpResponseExtensions
     {
+        public const string ErroneousAttributesHeader = "erroneous-dicom-attributes";
+
         public static void AddLocationHeader(this HttpResponse response, Uri locationUrl)
         {
             EnsureArg.IsNotNull(response, nameof(response));
             EnsureArg.IsNotNull(locationUrl, nameof(locationUrl));
 
             response.Headers.Add(HeaderNames.Location, Uri.EscapeUriString(locationUrl.ToString()));
         }
+
+        public static bool TryAddErroneousAttributesHeader(this HttpResponse response, IReadOnlyCollection<string> erroneousAttributes)
+        {
+            EnsureArg.IsNotNull(response, nameof(response));
+            EnsureArg.IsNotNull(erroneousAttributes, nameof(erroneousAttributes));
+            if (erroneousAttributes.Count == 0)
+            {
+                return false;
+            }
+
+            response.Headers.Add(ErroneousAttributesHeader, string.Join(",", erroneousAttributes));
+            return true;
+        }
     }
 }

src/Microsoft.Health.Dicom.Core.UnitTests/Features/Query/QueryParserTests.cs

@@ -336,6 +336,48 @@ public void GivenPatientNameFilterCondition_WithFuzzyMatchingTrue_FuzzyMatchCond
             Assert.Equal("CoronaPatient", fuzzyCondition.Value);
         }
 
+        [Fact]
+        public void GivenErroneousTag_WhenParse_ThenShouldBeInList()
+        {
+            DicomTag tag1 = DicomTag.PatientAge;
+            DicomTag tag2 = DicomTag.PatientAddress;
+            QueryTag[] tags = new QueryTag[]
+            {
+              new QueryTag(new ExtendedQueryTagStoreEntry(1, tag1.GetPath(), tag1.GetDefaultVR().Code, null, QueryTagLevel.Instance, ExtendedQueryTagStatus.Ready, QueryStatus.Enabled,1)), // has error
+              new QueryTag(new ExtendedQueryTagStoreEntry(2, tag2.GetPath(), tag2.GetDefaultVR().Code, null, QueryTagLevel.Instance, ExtendedQueryTagStatus.Ready, QueryStatus.Enabled,0)), // no error
+            };
+            QueryExpression queryExpression = _queryParser.Parse(
+                CreateParameters(
+                    new Dictionary<string, string>
+                    {
+                        { tag1.GetFriendlyName(), "CoronaPatient" },
+                        { tag2.GetPath(), "20200403" },
+                    },
+                    QueryResource.AllInstances),
+                tags);
+
+            Assert.Single(queryExpression.ErroneousTags);
+            Assert.Equal(queryExpression.ErroneousTags.First(), tag1.GetFriendlyName());
+        }
+
+        [Fact]
+        public void GivenDisabledTag_WhenParse_ThenShouldThrowException()
+        {
+            DicomTag tag1 = DicomTag.PatientAge;
+            QueryTag[] tags = new QueryTag[]
+            {
+              new QueryTag(new ExtendedQueryTagStoreEntry(1, tag1.GetPath(), tag1.GetDefaultVR().Code, null, QueryTagLevel.Instance, ExtendedQueryTagStatus.Ready, QueryStatus.Disabled,1)), // disabled
+            };
+            var parameters = CreateParameters(
+                    new Dictionary<string, string>
+                    {
+                        { tag1.GetFriendlyName(), "CoronaPatient" },
+                    },
+                    QueryResource.AllStudies);
+
+            Assert.Throws<QueryParseException>(() => _queryParser.Parse(parameters, tags));
+        }
+
         private void VerifyIncludeFieldsForValidAttributeIds(params string[] values)
         {
             QueryExpression queryExpression = _queryParser.Parse(

src/Microsoft.Health.Dicom.Core.UnitTests/Features/Query/QueryResponseBuilderTests.cs

@@ -3,6 +3,7 @@
 // Licensed under the MIT License (MIT). See LICENSE in the repo root for license information.
 // -------------------------------------------------------------------------------------------------
 
+using System;
 using System.Collections.Generic;
 using System.Linq;
 using Dicom;
@@ -26,7 +27,7 @@ public void GivenStudyLevel_WithIncludeField_ValidReturned()
             {
                 new StringSingleValueMatchCondition(queryTag, "35"),
             };
-            var query = new QueryExpression(QueryResource.AllStudies, includeField, false, 0, 0, filters);
+            var query = new QueryExpression(QueryResource.AllStudies, includeField, false, 0, 0, filters, Array.Empty<string>());
             var responseBuilder = new QueryResponseBuilder(query);
 
             DicomDataset responseDataset = responseBuilder.GenerateResponseDataset(GenerateTestDataSet());
@@ -49,7 +50,7 @@ public void GivenStudySeriesLevel_WithIncludeField_ValidReturned()
             {
                 new StringSingleValueMatchCondition(queryTag, "35"),
             };
-            var query = new QueryExpression(QueryResource.StudySeries, includeField, false, 0, 0, filters);
+            var query = new QueryExpression(QueryResource.StudySeries, includeField, false, 0, 0, filters, Array.Empty<string>());
             var responseBuilder = new QueryResponseBuilder(query);
 
             DicomDataset responseDataset = responseBuilder.GenerateResponseDataset(GenerateTestDataSet());
@@ -67,7 +68,7 @@ public void GivenAllSeriesLevel_WithIncludeField_ValidReturned()
         {
             var includeField = QueryIncludeField.AllFields;
             var filters = new List<QueryFilterCondition>();
-            var query = new QueryExpression(QueryResource.AllSeries, includeField, false, 0, 0, filters);
+            var query = new QueryExpression(QueryResource.AllSeries, includeField, false, 0, 0, filters, Array.Empty<string>());
             var responseBuilder = new QueryResponseBuilder(query);
 
             DicomDataset responseDataset = responseBuilder.GenerateResponseDataset(GenerateTestDataSet());
@@ -85,7 +86,7 @@ public void GivenAllInstanceLevel_WithIncludeField_ValidReturned()
         {
             var includeField = QueryIncludeField.AllFields;
             var filters = new List<QueryFilterCondition>();
-            var query = new QueryExpression(QueryResource.AllInstances, includeField, false, 0, 0, filters);
+            var query = new QueryExpression(QueryResource.AllInstances, includeField, false, 0, 0, filters, Array.Empty<string>());
             var responseBuilder = new QueryResponseBuilder(query);
 
             DicomDataset responseDataset = responseBuilder.GenerateResponseDataset(GenerateTestDataSet());
@@ -106,7 +107,7 @@ public void GivenStudyInstanceLevel_WithIncludeField_ValidReturned()
             {
                 new StringSingleValueMatchCondition(new QueryTag(DicomTag.StudyInstanceUID), "35"),
             };
-            var query = new QueryExpression(QueryResource.StudyInstances, includeField, false, 0, 0, filters);
+            var query = new QueryExpression(QueryResource.StudyInstances, includeField, false, 0, 0, filters, Array.Empty<string>());
             var responseBuilder = new QueryResponseBuilder(query);
 
             DicomDataset responseDataset = responseBuilder.GenerateResponseDataset(GenerateTestDataSet());
@@ -129,7 +130,7 @@ public void GivenStudySeriesInstanceLevel_WithIncludeField_ValidReturned()
                 new StringSingleValueMatchCondition(new QueryTag(DicomTag.StudyInstanceUID), "35"),
                 new StringSingleValueMatchCondition(new QueryTag(DicomTag.SeriesInstanceUID), "351"),
             };
-            var query = new QueryExpression(QueryResource.StudySeriesInstances, includeField, false, 0, 0, filters);
+            var query = new QueryExpression(QueryResource.StudySeriesInstances, includeField, false, 0, 0, filters, Array.Empty<string>());
             var responseBuilder = new QueryResponseBuilder(query);
 
             DicomDataset responseDataset = responseBuilder.GenerateResponseDataset(GenerateTestDataSet());

src/Microsoft.Health.Dicom.Core.UnitTests/Features/Query/DicomQueryServiceTests.cs → src/Microsoft.Health.Dicom.Core.UnitTests/Features/Query/QueryServiceTests.cs

@@ -3,6 +3,7 @@
 // Licensed under the MIT License (MIT). See LICENSE in the repo root for license information.
 // -------------------------------------------------------------------------------------------------
 
+using System;
 using System.Collections.Generic;
 using System.Linq;
 using System.Threading;
@@ -19,14 +20,14 @@
 
 namespace Microsoft.Health.Dicom.Core.UnitTests.Features.Query
 {
-    public class DicomQueryServiceTests
+    public class QueryServiceTests
     {
         private readonly QueryService _queryService;
         private readonly IQueryParser _queryParser;
         private readonly IQueryStore _queryStore;
         private readonly IQueryTagService _queryTagService;
 
-        public DicomQueryServiceTests()
+        public QueryServiceTests()
         {
             _queryParser = Substitute.For<IQueryParser>();
             _queryStore = Substitute.For<IQueryStore>();
@@ -74,6 +75,7 @@ public void GivenQidoQuery_WithInvalidStudySeriesUid_ThrowsValidationException(Q
         [InlineData(QueryResource.StudySeriesInstances)]
         public async void GivenRequestForInstances_WhenRetrievingQueriableExtendedQueryTags_ReturnsAllTags(QueryResource resourceType)
         {
+            _queryParser.Parse(default, default).ReturnsForAnyArgs(new QueryExpression(default, default, default, default, default, Array.Empty<QueryFilterCondition>(), Array.Empty<string>()));
             var parameters = new QueryParameters
             {
                 Filters = new Dictionary<string, string>(),
@@ -101,6 +103,7 @@ public async void GivenRequestForInstances_WhenRetrievingQueriableExtendedQueryT
         [InlineData(QueryResource.StudySeries)]
         public async void GivenRequestForSeries_WhenRetrievingQueriableExtendedQueryTags_ReturnsSeriesAndStudyTags(QueryResource resourceType)
         {
+            _queryParser.Parse(default, default).ReturnsForAnyArgs(new QueryExpression(default, default, default, default, default, Array.Empty<QueryFilterCondition>(), Array.Empty<string>()));
             var parameters = new QueryParameters
             {
                 Filters = new Dictionary<string, string>(),
@@ -126,6 +129,7 @@ public async void GivenRequestForSeries_WhenRetrievingQueriableExtendedQueryTags
         [InlineData(QueryResource.AllStudies)]
         public async void GivenRequestForStudies_WhenRetrievingQueriableExtendedQueryTags_ReturnsStudyTags(QueryResource resourceType)
         {
+            _queryParser.Parse(default, default).ReturnsForAnyArgs(new QueryExpression(default, default, default, default, default, Array.Empty<QueryFilterCondition>(), Array.Empty<string>()));
             var parameters = new QueryParameters
             {
                 Filters = new Dictionary<string, string>(),

src/Microsoft.Health.Dicom.Core/DicomCoreResource.resx

@@ -414,4 +414,8 @@ The first part date {1} should be lesser than or equal to the second part date {
   <data name="InvalidIncludeAllFields" xml:space="preserve">
     <value>Invalid QIDO-RS query. Cannot specify included fields in addition to 'all'.</value>
   </data>
+  <data name="QueryIsDisabledOnAttribute" xml:space="preserve">
+    <value>Query is disabled on specified attribute '{0}'.</value>
+    <comment>'{0}' attribute in QIDO query. </comment>
+  </data>
 </root>

src/Microsoft.Health.Dicom.Core/Features/Query/Model/QueryExpression.cs

@@ -5,6 +5,7 @@
 
 using System.Collections.Generic;
 using System.Linq;
+using EnsureThat;
 using Microsoft.Health.Dicom.Core.Messages;
 
 namespace Microsoft.Health.Dicom.Core.Features.Query.Model
@@ -20,14 +21,16 @@ public QueryExpression(
             bool fuzzyMatching,
             int limit,
             int offset,
-            IReadOnlyCollection<QueryFilterCondition> filterConditions)
+            IReadOnlyCollection<QueryFilterCondition> filterConditions,
+            IReadOnlyCollection<string> erroneousTags)
         {
             QueryResource = resourceType;
             IncludeFields = includeFields;
             FuzzyMatching = fuzzyMatching;
             Limit = limit;
             Offset = offset;
-            FilterConditions = filterConditions;
+            FilterConditions = EnsureArg.IsNotNull(filterConditions, nameof(filterConditions));
+            ErroneousTags = EnsureArg.IsNotNull(erroneousTags, nameof(erroneousTags));
             SetIELevel();
         }
 
@@ -66,6 +69,11 @@ public QueryExpression(
         /// </summary>
         public IReadOnlyCollection<QueryFilterCondition> FilterConditions { get; }
 
+        /// <summary>
+        /// List of erroneous tags.
+        /// </summary>
+        public IReadOnlyCollection<string> ErroneousTags { get; }
+
         /// <summary>
         /// Request query was empty
         /// </summary>

src/Microsoft.Health.Dicom.Core/Features/Query/QueryParser.cs

@@ -61,6 +61,8 @@ public QueryExpression Parse(QueryParameters parameters, IReadOnlyCollection<Que
             // Update the list of query tags
             queryTags = GetQualifiedQueryTags(queryTags, parameters.QueryResourceType);
 
+            List<string> erroneousTags = new List<string>();
+
             var filterConditions = new Dictionary<DicomTag, QueryFilterCondition>();
             foreach (KeyValuePair<string, string> filter in parameters.Filters)
             {
@@ -70,6 +72,11 @@ public QueryExpression Parse(QueryParameters parameters, IReadOnlyCollection<Que
                     throw new QueryParseException(string.Format(DicomCoreResource.UnknownQueryParameter, filter.Key));
                 }
 
+                if (condition.QueryTag.IsExtendedQueryTag && condition.QueryTag.ExtendedQueryTagStoreEntry.ErrorCount > 0)
+                {
+                    erroneousTags.Add(filter.Key);
+                }
+
                 if (!filterConditions.TryAdd(condition.QueryTag.Tag, condition))
                 {
                     throw new QueryParseException(string.Format(DicomCoreResource.DuplicateAttribute, filter.Key));
@@ -101,7 +108,8 @@ public QueryExpression Parse(QueryParameters parameters, IReadOnlyCollection<Que
                 parameters.FuzzyMatching,
                 parameters.Limit,
                 parameters.Offset,
-                filterConditions.Values);
+                filterConditions.Values,
+                erroneousTags);
         }
 
         private static IReadOnlyCollection<QueryTag> GetQualifiedQueryTags(IReadOnlyCollection<QueryTag> queryTags, QueryResource queryResource)
@@ -134,7 +142,14 @@ private bool ParseFilterCondition(
                 return false;
             }
 
-            QueryTag queryTag = GetSupportedQueryTag(dicomTag, queryParameter.Key, queryTags);
+            // QueryTag could be either core or extended query tag.
+            QueryTag queryTag = GetMatchingQueryTag(dicomTag, queryParameter.Key, queryTags);
+
+            // check if tag is disabled
+            if (queryTag.IsExtendedQueryTag && queryTag.ExtendedQueryTagStoreEntry.QueryStatus == QueryStatus.Disabled)
+            {
+                throw new QueryParseException(string.Format(DicomCoreResource.QueryIsDisabledOnAttribute, queryParameter.Key));
+            }
 
             if (string.IsNullOrWhiteSpace(queryParameter.Value))
             {
@@ -168,7 +183,7 @@ private bool TryParseDicomAttributeId(string attributeId, out DicomTag dicomTag)
             return false;
         }
 
-        private static QueryTag GetSupportedQueryTag(DicomTag dicomTag, string attributeId, IEnumerable<QueryTag> queryTags)
+        private static QueryTag GetMatchingQueryTag(DicomTag dicomTag, string attributeId, IEnumerable<QueryTag> queryTags)
         {
             QueryTag queryTag = queryTags.FirstOrDefault(item =>
             {