Skip to content

Commit

Permalink
Misc Extended Query Tag Fixes (#1213)
Browse files Browse the repository at this point in the history
- Ensure user has read access for fetching operation status
- Ignore DICOM instances that have been deleted between fetching the batch and performing the re-index
- Add non-root user to docker image for functions
- Update enum serialization to strictly use names
- Update swagger doc to include new APIs and use enum names instead of numbers
  • Loading branch information
wsugarman authored Dec 1, 2021
1 parent b3da57f commit 2a3c0a3
Show file tree
Hide file tree
Showing 26 changed files with 3,470 additions and 348 deletions.
3 changes: 3 additions & 0 deletions docker/functions/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@
FROM mcr.microsoft.com/azure-functions/dotnet:4.0.1.16816@sha256:2c18c4ca6ad982257e9023161329c38be6d9d494bc48a6aae15ef43cc8f7ea5d AS az-func-runtime
ENV AzureFunctionsJobHost__Logging__Console__IsEnabled=true \
AzureWebJobsScriptRoot=/home/site/wwwroot
RUN groupadd nonroot && useradd -r -M -s /sbin/nologin -g nonroot -c nonroot nonroot
RUN chown -R nonroot:nonroot /azure-functions-host
USER nonroot

# Copy the DICOM Server repository and build the Azure Functions project
FROM mcr.microsoft.com/dotnet/sdk:6.0.100-alpine3.14@sha256:1d9d66775f0d67cb68af9c7a083d22b576ea96f3f7a6893660d48f536b73e59f AS build
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,13 +127,13 @@ public async Task GivenOperationId_WhenAddingTags_ReturnIdWithHeader()
{
new AddExtendedQueryTagEntry
{
Level = QueryTagLevel.Instance.ToString(),
Level = QueryTagLevel.Instance,
Path = "00101001",
VR = DicomVRCode.PN,
},
new AddExtendedQueryTagEntry
{
Level = QueryTagLevel.Instance.ToString(),
Level = QueryTagLevel.Instance,
Path = "11330001",
PrivateCreator = "Microsoft",
VR = DicomVRCode.SS,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ public void GivenModelError_WhenOnActionExecution_ThenShouldThrowInvalidRequestB
_context.ModelState.AddModelError(key2, error2);
_context.ModelState.AddModelError(key3, error3);
var exp = Assert.Throws<InvalidRequestBodyException>(() => _filter.OnActionExecuting(_context));
Assert.Equal($"The field '{key2}' in request body is invalid: {error1}", exp.Message);
Assert.Equal($"The field '{key3}' in request body is invalid: {error3}", exp.Message);
}

private static ActionExecutingContext CreateContext()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ public override void OnActionExecuting(ActionExecutingContext context)
EnsureArg.IsNotNull(context, nameof(context));
if (!context.ModelState.IsValid)
{
var error = context.ModelState.First(x => x.Value.ValidationState == AspNetCore.Mvc.ModelBinding.ModelValidationState.Invalid);
var error = context.ModelState.Last(x => x.Value.ValidationState == AspNetCore.Mvc.ModelBinding.ModelValidationState.Invalid);
throw new InvalidRequestBodyException(error.Key, error.Value.Errors.First().ErrorMessage);
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
// -------------------------------------------------------------------------------------------------
// Copyright (c) Microsoft Corporation. All rights reserved.
// 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.Reflection;
using Microsoft.OpenApi.Models;
using Swashbuckle.AspNetCore.SwaggerGen;

namespace Microsoft.Health.Dicom.Api.Features.Swagger
{
// The ReflectionTypeFilter is used to remove types added mistakenly by the Swashbuckle library.
// Swashbuckle mistakenly assumes the DICOM server will serialize Type properties present on the fo-dicom types,
// so this filter removes erroneously added properties in addition to their recursively added data models.
internal class ReflectionTypeFilter : IDocumentFilter
{
// Get all built-in generic collections (and IEnumerable<T>)
private static readonly HashSet<Type> CollectionTypes = typeof(List<>).Assembly
.ExportedTypes
.Where(x => x.Namespace == "System.Collections.Generic")
.Where(x => x == typeof(IEnumerable<>) || x.GetInterfaces().Any(i => i.IsGenericType && i.GetGenericTypeDefinition() == typeof(IEnumerable<>)))
.ToHashSet();

public void Apply(OpenApiDocument swaggerDoc, DocumentFilterContext context)
{
HashSet<string> reflectionTypes = GetExposedTypes(typeof(Type));

// Resolve the enumerable up-front so that the dictionary can be mutated below
List<KeyValuePair<string, OpenApiSchema>> schemas = context.SchemaRepository.Schemas.ToList();

// Remove all properties, and schemas themselves, whose types are from System.Reflection
foreach (KeyValuePair<string, OpenApiSchema> entry in schemas)
{
if (reflectionTypes.Contains(entry.Key))
{
context.SchemaRepository.Schemas.Remove(entry.Key);
}
else if (entry.Value.Type == "object" && entry.Value.Properties?.Count > 0)
{
entry.Value.Properties = entry.Value.Properties
.Where(x => x.Value.Reference == null || !reflectionTypes.Contains(x.Value.Reference.Id))
.ToDictionary(x => x.Key, x => x.Value);
}
}
}

private static HashSet<string> GetExposedTypes(Type t)
{
var exposedTypes = new HashSet<Type>();
UpdateExposedTypes(t, exposedTypes);

// The OpenApiSchema type only has the type names
return exposedTypes.Select(x => x.Name).ToHashSet(StringComparer.Ordinal);
}

private static void UpdateExposedTypes(Type t, HashSet<Type> exposed)
{
// Get all public instance properties present in the Type that may be discovered via reflection by Swashbuckle
foreach (Type propertyType in t.GetProperties(BindingFlags.Public | BindingFlags.Instance).Select(x => GetExposedType(x.PropertyType)))
{
if (!IsBuiltInType(propertyType) && exposed.Add(propertyType))
{
UpdateExposedTypes(propertyType, exposed);
}
}
}

private static bool IsBuiltInType(Type t)
=> (t.IsPrimitive && t != typeof(IntPtr) && t != typeof(UIntPtr))
|| t == typeof(string)
|| t == typeof(TimeSpan)
|| t == typeof(DateTime)
|| t == typeof(DateTimeOffset)
|| t == typeof(Guid);

private static Type GetExposedType(Type t)
{
// If we're serializing a collection to JSON, it will be represented as a JSON array.
// So the type we're really concerned with is the element type contained in the collection.
if (t.IsArray)
{
t = t.GetElementType();
}
else if (t.IsGenericType && CollectionTypes.Contains(t.GetGenericTypeDefinition()))
{
Type enumerableType = t.GetGenericTypeDefinition() != typeof(IEnumerable<>)
? t.GetInterfaces().Single(i => i.IsGenericType && i.GetGenericTypeDefinition() == typeof(IEnumerable<>))
: t;

t = enumerableType.GetGenericArguments()[0];
}

return t;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

using System;
using System.Reflection;
using System.Text.Json.Serialization;
using EnsureThat;
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Mvc;
Expand All @@ -32,6 +31,7 @@
using Microsoft.Health.Dicom.Core.Features.Context;
using Microsoft.Health.Dicom.Core.Features.Routing;
using Microsoft.Health.Dicom.Core.Registration;
using Microsoft.Health.Dicom.Core.Serialization;
using Microsoft.Health.Extensions.DependencyInjection;
using Microsoft.IO;
using Swashbuckle.AspNetCore.SwaggerGen;
Expand Down Expand Up @@ -109,7 +109,7 @@ public static IDicomServerBuilder AddDicomServer(
options.RespectBrowserAcceptHeader = true;
options.OutputFormatters.Insert(0, new DicomJsonOutputFormatter());
})
.AddJsonSerializerOptions(o => o.Converters.Add(new JsonStringEnumConverter()));
.AddJsonSerializerOptions(o => o.Converters.Add(new StrictStringEnumConverterFactory()));

services.AddApiVersioning(c =>
{
Expand All @@ -128,10 +128,13 @@ public static IDicomServerBuilder AddDicomServer(
});

services.AddTransient<IConfigureOptions<SwaggerGenOptions>, ConfigureSwaggerOptions>();
services.AddSwaggerGen(options => options.OperationFilter<SwaggerDefaultValues>());
services.AddSwaggerGen(options => options.OperationFilter<ErrorCodeOperationFilter>());
services.AddSwaggerGen(options => options.OperationFilter<RetrieveOperationFilter>());
services.AddSwaggerGenNewtonsoftSupport();
services.AddSwaggerGen(options =>
{
options.OperationFilter<SwaggerDefaultValues>();
options.OperationFilter<ErrorCodeOperationFilter>();
options.OperationFilter<RetrieveOperationFilter>();
options.DocumentFilter<ReflectionTypeFilter>();
});

services.AddSingleton<IUrlResolver, UrlResolver>();

Expand Down Expand Up @@ -197,13 +200,13 @@ public Action<IApplicationBuilder> Configure(Action<IApplicationBuilder> next)
});

//Disabling swagger ui until accesability team gets back to us
/*app.UseSwaggerUI(options =>
{
foreach (ApiVersionDescription description in provider.ApiVersionDescriptions)
{
options.SwaggerEndpoint($"/swagger/{description.GroupName}/swagger.yaml", description.GroupName.ToUpperInvariant());
}
});*/
//app.UseSwaggerUI(options =>
//{
// foreach (ApiVersionDescription description in provider.ApiVersionDescriptions)
// {
// options.SwaggerEndpoint($"/swagger/{description.GroupName}/swagger.yaml", description.GroupName.ToUpperInvariant());
// }
//});

next(app);
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public void GivenValidExtendedQueryTagEntry_WhenNormalizing_ThenShouldReturnSame
public void GivenPrivateTagWithNonEmptyPrivateCreator_WhenNormalizing_ThenPrivateCreatorShouldBeNull(string privateCreator)
{
DicomTag tag1 = new DicomTag(0x0405, 0x1001);
AddExtendedQueryTagEntry normalized = new AddExtendedQueryTagEntry() { Level = QueryTagLevel.Instance.ToString(), Path = tag1.GetPath(), PrivateCreator = privateCreator, VR = DicomVRCode.CS }.Normalize();
AddExtendedQueryTagEntry normalized = new AddExtendedQueryTagEntry() { Level = QueryTagLevel.Instance, Path = tag1.GetPath(), PrivateCreator = privateCreator, VR = DicomVRCode.CS }.Normalize();
Assert.Null(normalized.PrivateCreator);
}

Expand Down Expand Up @@ -117,7 +117,7 @@ private static GetExtendedQueryTagEntry CreateExtendedQueryTagEntry(string path,

private static AddExtendedQueryTagEntry CreateExtendedQueryTagEntry(string path, string vr, string privateCreator, QueryTagLevel level = QueryTagLevel.Instance)
{
return new AddExtendedQueryTagEntry { Path = path, VR = vr, PrivateCreator = privateCreator, Level = level.ToString() };
return new AddExtendedQueryTagEntry { Path = path, VR = vr, PrivateCreator = privateCreator, Level = level };
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ public class AddExtendedQueryTagEntryValidationTests
[Fact]
public void GivenValidAddExtendedQueryTagEntry_WhenValidating_ShouldSucced()
{
AddExtendedQueryTagEntry addExtendedQueryTagEntry = new AddExtendedQueryTagEntry() { Path = "00101001", Level = "Study" };
AddExtendedQueryTagEntry addExtendedQueryTagEntry = new AddExtendedQueryTagEntry() { Path = "00101001", Level = QueryTagLevel.Study };
var validationContext = new ValidationContext(addExtendedQueryTagEntry);
IEnumerable<ValidationResult> results = addExtendedQueryTagEntry.Validate(validationContext);
Assert.Empty(results);
Expand All @@ -28,51 +28,44 @@ public void GivenValidAddExtendedQueryTagEntry_WhenValidating_ShouldSucced()
[InlineData(" ")]
public void GivenInvalidPath_WhenValidating_ResultShouldHaveExceptions(string pathValue)
{
AddExtendedQueryTagEntry addExtendedQueryTagEntry = new AddExtendedQueryTagEntry() { Path = pathValue, Level = "Study" };
AddExtendedQueryTagEntry addExtendedQueryTagEntry = new AddExtendedQueryTagEntry() { Path = pathValue, Level = QueryTagLevel.Study };
var validationContext = new ValidationContext(addExtendedQueryTagEntry);
IEnumerable<ValidationResult> results = addExtendedQueryTagEntry.Validate(validationContext);
Assert.Single(results);
Assert.Equal("The Dicom Tag Property Path must be specified and must not be null, empty or whitespace.", results.First().ErrorMessage);
}

[Theory]
[InlineData(null)]
[InlineData("")]
[InlineData(" ")]
public void GivenEmptyNullOrWhitespaceLevel_WhenValidating_ResultShouldHaveExceptions(string levelValue)
[Fact]
public void GivenEmptyNullOrWhitespaceLevel_WhenValidating_ResultShouldHaveExceptions()
{
AddExtendedQueryTagEntry addExtendedQueryTagEntry = new AddExtendedQueryTagEntry() { Path = "00101001", Level = levelValue };
AddExtendedQueryTagEntry addExtendedQueryTagEntry = new AddExtendedQueryTagEntry() { Path = "00101001", Level = null };
var validationContext = new ValidationContext(addExtendedQueryTagEntry);
IEnumerable<ValidationResult> results = addExtendedQueryTagEntry.Validate(validationContext);
Assert.Collection(
results,
item => Assert.Equal("The Dicom Tag Property Level must be specified and must not be null, empty or whitespace.", item.ErrorMessage),
item => Assert.Equal(string.Format("Input Dicom Tag Level '{0}' is invalid. It must have value 'Study', 'Series' or 'Instance'.", levelValue), item.ErrorMessage)
);
item => Assert.Equal("The Dicom Tag Property Level must be specified and must not be null, empty or whitespace.", item.ErrorMessage));
}

[Fact]
public void GivenInvalidLevel_WhenValidating_ResultShouldHaveExceptions()
{
AddExtendedQueryTagEntry addExtendedQueryTagEntry = new AddExtendedQueryTagEntry() { Path = "00101001", Level = "Studys" };
AddExtendedQueryTagEntry addExtendedQueryTagEntry = new AddExtendedQueryTagEntry() { Path = "00101001", Level = (QueryTagLevel)47 };
var validationContext = new ValidationContext(addExtendedQueryTagEntry);
IEnumerable<ValidationResult> results = addExtendedQueryTagEntry.Validate(validationContext);
Assert.Single(results);
Assert.Equal("Input Dicom Tag Level 'Studys' is invalid. It must have value 'Study', 'Series' or 'Instance'.", results.First().ErrorMessage);
Assert.Equal("Input Dicom Tag Level '47' is invalid. It must have value 'Study', 'Series' or 'Instance'.", results.First().ErrorMessage);
}

[Fact]
public void GivenMultipleValidationErrors_WhenValidating_ResultShouldHaveExceptions()
{
AddExtendedQueryTagEntry addExtendedQueryTagEntry = new AddExtendedQueryTagEntry() { Path = "", Level = " " };
AddExtendedQueryTagEntry addExtendedQueryTagEntry = new AddExtendedQueryTagEntry() { Path = "", Level = null };
var validationContext = new ValidationContext(addExtendedQueryTagEntry);
IEnumerable<ValidationResult> results = addExtendedQueryTagEntry.Validate(validationContext);
Assert.Collection(
results,
item => Assert.Equal("The Dicom Tag Property Path must be specified and must not be null, empty or whitespace.", item.ErrorMessage),
item => Assert.Equal("The Dicom Tag Property Level must be specified and must not be null, empty or whitespace.", item.ErrorMessage),
item => Assert.Equal("Input Dicom Tag Level ' ' is invalid. It must have value 'Study', 'Series' or 'Instance'.", item.ErrorMessage)
);
item => Assert.Equal("The Dicom Tag Property Level must be specified and must not be null, empty or whitespace.", item.ErrorMessage));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ public void GivenPrivateIdentificationCodeWithWrongVR_WhenValidating_ThenShouldS

private static AddExtendedQueryTagEntry CreateExtendedQueryTagEntry(string path, string vr, string privateCreator = null, QueryTagLevel level = QueryTagLevel.Instance)
{
return new AddExtendedQueryTagEntry { Path = path, VR = vr, PrivateCreator = privateCreator, Level = level.ToString() };
return new AddExtendedQueryTagEntry { Path = path, VR = vr, PrivateCreator = privateCreator, Level = level };
}
}
}
Loading

0 comments on commit 2a3c0a3

Please sign in to comment.