Skip to content
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

Allow for duplicate search parameters #3790

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// -------------------------------------------------------------------------------------------------
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License (MIT). See LICENSE in the repo root for license information.
// -------------------------------------------------------------------------------------------------

namespace Microsoft.Health.Fhir.Core.Features.Search.Parameters
{
public static class Constants
{
public const string DuplicateSearchParameterUrl = "DuplicateSearchParameterUrl";
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,12 @@
// -------------------------------------------------------------------------------------------------

using System;
using System.Collections.Generic;
using System.Threading;
using System.Threading.Tasks;
using EnsureThat;
using FluentValidation.Results;
using Hl7.Fhir.Rest;
using MediatR;
using Microsoft.Health.Fhir.Core.Features.Persistence;
using Microsoft.Health.Fhir.Core.Messages.Create;
Expand All @@ -21,7 +24,9 @@ public class CreateOrUpdateSearchParameterBehavior<TCreateResourceRequest, TUpse
private ISearchParameterOperations _searchParameterOperations;
private IFhirDataStore _fhirDataStore;

public CreateOrUpdateSearchParameterBehavior(ISearchParameterOperations searchParameterOperations, IFhirDataStore fhirDataStore)
public CreateOrUpdateSearchParameterBehavior(
ISearchParameterOperations searchParameterOperations,
IFhirDataStore fhirDataStore)
{
EnsureArg.IsNotNull(searchParameterOperations, nameof(searchParameterOperations));
EnsureArg.IsNotNull(fhirDataStore, nameof(fhirDataStore));
Expand Down Expand Up @@ -52,6 +57,7 @@ public async Task<UpsertResourceResponse> Handle(UpsertResourceRequest request,
{
var resourceKey = new ResourceKey(request.Resource.InstanceType, request.Resource.Id, request.Resource.VersionId);
ResourceWrapper prevSearchParamResource = await _fhirDataStore.GetAsync(resourceKey, cancellationToken);

if (prevSearchParamResource != null)
{
// Update the SearchParameterDefinitionManager with the new SearchParameter in order to validate any changes
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,11 @@
using EnsureThat;
using Hl7.Fhir.ElementModel;
using Microsoft.Extensions.Logging;
using Microsoft.Health.Core.Features.Context;
using Microsoft.Health.Extensions.DependencyInjection;
using Microsoft.Health.Fhir.Core.Exceptions;
using Microsoft.Health.Fhir.Core.Extensions;
using Microsoft.Health.Fhir.Core.Features.Context;
using Microsoft.Health.Fhir.Core.Features.Definition;
using Microsoft.Health.Fhir.Core.Features.Definition.BundleWrappers;
using Microsoft.Health.Fhir.Core.Features.Persistence;
Expand All @@ -30,6 +32,7 @@ public class SearchParameterOperations : ISearchParameterOperations
private readonly ISearchParameterSupportResolver _searchParameterSupportResolver;
private readonly IDataStoreSearchParameterValidator _dataStoreSearchParameterValidator;
private readonly Func<IScoped<ISearchService>> _searchServiceFactory;
private readonly RequestContextAccessor<IFhirRequestContext> _contextAccessor;
private readonly ILogger _logger;

public SearchParameterOperations(
Expand All @@ -39,6 +42,7 @@ public SearchParameterOperations(
ISearchParameterSupportResolver searchParameterSupportResolver,
IDataStoreSearchParameterValidator dataStoreSearchParameterValidator,
Func<IScoped<ISearchService>> searchServiceFactory,
RequestContextAccessor<IFhirRequestContext> contextAccessor,
ILogger<SearchParameterOperations> logger)
{
EnsureArg.IsNotNull(searchParameterStatusManager, nameof(searchParameterStatusManager));
Expand All @@ -47,6 +51,7 @@ public SearchParameterOperations(
EnsureArg.IsNotNull(searchParameterSupportResolver, nameof(searchParameterSupportResolver));
EnsureArg.IsNotNull(dataStoreSearchParameterValidator, nameof(dataStoreSearchParameterValidator));
EnsureArg.IsNotNull(searchServiceFactory, nameof(searchServiceFactory));
EnsureArg.IsNotNull(contextAccessor, nameof(contextAccessor));
EnsureArg.IsNotNull(logger, nameof(logger));

_searchParameterStatusManager = searchParameterStatusManager;
Expand All @@ -55,42 +60,51 @@ public SearchParameterOperations(
_searchParameterSupportResolver = searchParameterSupportResolver;
_dataStoreSearchParameterValidator = dataStoreSearchParameterValidator;
_searchServiceFactory = searchServiceFactory;
_contextAccessor = contextAccessor;
_logger = logger;
}

public async Task AddSearchParameterAsync(ITypedElement searchParam, CancellationToken cancellationToken)
{
try
{
// verify the parameter is supported before continuing
var searchParameterWrapper = new SearchParameterWrapper(searchParam);
var searchParameterInfo = new SearchParameterInfo(searchParameterWrapper);
bool duplicateSearchParameter = IsSearchParameterDuplicate();

if (searchParameterInfo.Component?.Any() == true)
if (!duplicateSearchParameter)
{
foreach (SearchParameterComponentInfo c in searchParameterInfo.Component)
// verify the parameter is supported before continuing
var searchParameterInfo = new SearchParameterInfo(searchParameterWrapper);

if (searchParameterInfo.Component?.Any() == true)
{
c.ResolvedSearchParameter = _searchParameterDefinitionManager.GetSearchParameter(c.DefinitionUrl.OriginalString);
foreach (SearchParameterComponentInfo c in searchParameterInfo.Component)
{
c.ResolvedSearchParameter = _searchParameterDefinitionManager.GetSearchParameter(c.DefinitionUrl.OriginalString);
}
}
}

(bool Supported, bool IsPartiallySupported) supportedResult = _searchParameterSupportResolver.IsSearchParameterSupported(searchParameterInfo);
(bool Supported, bool IsPartiallySupported) supportedResult = _searchParameterSupportResolver.IsSearchParameterSupported(searchParameterInfo);

if (!supportedResult.Supported)
{
throw new SearchParameterNotSupportedException(searchParameterInfo.Url);
}
if (!supportedResult.Supported)
{
throw new SearchParameterNotSupportedException(searchParameterInfo.Url);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Customer sees this message "The search parameter with definition URL '{0}' is not supported.". I know this change is not related to this PR but in this scenario, error is thrown if definition has any issues. Do you think we can use better error message to indicate customer that something is wrong with the definition they have sent?

}

// check data store specific support for SearchParameter
if (!_dataStoreSearchParameterValidator.ValidateSearchParameter(searchParameterInfo, out var errorMessage))
{
throw new SearchParameterNotSupportedException(errorMessage);
// check data store specific support for SearchParameter
if (!_dataStoreSearchParameterValidator.ValidateSearchParameter(searchParameterInfo, out var errorMessage))
{
throw new SearchParameterNotSupportedException(errorMessage);
}
}

_logger.LogTrace("Adding the search parameter '{Url}'", searchParameterWrapper.Url);
_searchParameterDefinitionManager.AddNewSearchParameters(new List<ITypedElement> { searchParam });

await _searchParameterStatusManager.AddSearchParameterStatusAsync(new List<string> { searchParameterWrapper.Url }, cancellationToken);
await _searchParameterStatusManager.AddSearchParameterStatusAsync(
new List<string> { searchParameterWrapper.Url },
cancellationToken,
duplicateSearchParameter ? SearchParameterStatus.Duplicate : SearchParameterStatus.Supported);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are introducing a new status Duplicate here. How is it going to impact reindex and $status operation?

}
catch (FhirException fex)
{
Expand Down Expand Up @@ -167,18 +181,23 @@ public async Task UpdateSearchParameterAsync(ITypedElement searchParam, RawResou
await GetAndApplySearchParameterUpdates(cancellationToken);

var searchParameterWrapper = new SearchParameterWrapper(searchParam);
var searchParameterInfo = new SearchParameterInfo(searchParameterWrapper);
(bool Supported, bool IsPartiallySupported) supportedResult = _searchParameterSupportResolver.IsSearchParameterSupported(searchParameterInfo);
bool duplicateSearchParameter = IsSearchParameterDuplicate();

if (!supportedResult.Supported)
if (!duplicateSearchParameter)
{
throw new SearchParameterNotSupportedException(searchParameterInfo.Url);
}
var searchParameterInfo = new SearchParameterInfo(searchParameterWrapper);
(bool Supported, bool IsPartiallySupported) supportedResult = _searchParameterSupportResolver.IsSearchParameterSupported(searchParameterInfo);

// check data store specific support for SearchParameter
if (!_dataStoreSearchParameterValidator.ValidateSearchParameter(searchParameterInfo, out var errorMessage))
{
throw new SearchParameterNotSupportedException(errorMessage);
if (!supportedResult.Supported)
{
throw new SearchParameterNotSupportedException(searchParameterInfo.Url);
}

// check data store specific support for SearchParameter
if (!_dataStoreSearchParameterValidator.ValidateSearchParameter(searchParameterInfo, out var errorMessage))
{
throw new SearchParameterNotSupportedException(errorMessage);
}
}

var prevSearchParam = _modelInfoProvider.ToTypedElement(previousSearchParam);
Expand All @@ -193,7 +212,10 @@ public async Task UpdateSearchParameterAsync(ITypedElement searchParam, RawResou

_logger.LogTrace("Adding the search parameter '{Url}' (update step 2/2)", searchParameterWrapper.Url);
_searchParameterDefinitionManager.AddNewSearchParameters(new List<ITypedElement>() { searchParam });
await _searchParameterStatusManager.AddSearchParameterStatusAsync(new List<string>() { searchParameterWrapper.Url }, cancellationToken);
await _searchParameterStatusManager.AddSearchParameterStatusAsync(
new List<string>() { searchParameterWrapper.Url },
cancellationToken,
duplicateSearchParameter ? SearchParameterStatus.Duplicate : SearchParameterStatus.Supported);
}
catch (FhirException fex)
{
Expand Down Expand Up @@ -302,5 +324,10 @@ private async Task<ITypedElement> GetSearchParameterByUrl(string url, Cancellati

return null;
}

private bool IsSearchParameterDuplicate()
{
return _contextAccessor.RequestContext.Properties.TryGetValue(Constants.DuplicateSearchParameterUrl, out _);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,6 @@ public enum SearchParameterStatus
PendingDelete = 5,
PendingDisable = 6,
Unsupported = 7,
Duplicate = 8,
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -136,14 +136,23 @@ public async Task UpdateSearchParameterStatusAsync(IReadOnlyCollection<string> s

foreach (string uri in searchParameterUris)
{
// first check if this parameter is marked as duplicate. If so, only status updates to deleted are allowed.
var existinParameterFound = parameters.TryGetValue(uri, out var existingStatus);
if (existinParameterFound && existingStatus.Status == SearchParameterStatus.Duplicate &&
(status != SearchParameterStatus.Duplicate && status != SearchParameterStatus.Deleted))
{
_logger.LogWarning("SearchParameterStatusManager: Attempting to update status of duplicate search parameter {Uri}", uri);
throw new InvalidOperationException(string.Format(Core.Resources.SearchParameterStatusManagerDuplicate, uri));
}

_logger.LogTrace("Setting the search parameter status of '{Uri}' to '{NewStatus}'", uri, status.ToString());

SearchParameterInfo paramInfo = _searchParameterDefinitionManager.GetSearchParameter(uri);
updated.Add(paramInfo);
paramInfo.IsSearchable = status == SearchParameterStatus.Enabled;
paramInfo.IsSupported = status == SearchParameterStatus.Supported || status == SearchParameterStatus.Enabled;

if (parameters.TryGetValue(uri, out var existingStatus))
if (existinParameterFound)
{
existingStatus.LastUpdated = Clock.UtcNow;
existingStatus.Status = status;
Expand Down Expand Up @@ -172,11 +181,16 @@ public async Task UpdateSearchParameterStatusAsync(IReadOnlyCollection<string> s
await _mediator.Publish(new SearchParametersUpdatedNotification(updated), cancellationToken);
}

internal async Task AddSearchParameterStatusAsync(IReadOnlyCollection<string> searchParamUris, CancellationToken cancellationToken)
internal async Task AddSearchParameterStatusAsync(
IReadOnlyCollection<string> searchParamUris,
CancellationToken cancellationToken,
SearchParameterStatus status = SearchParameterStatus.Supported)
{
// new search parameters are added as supported, until reindexing occurs, when
// new search parameters are normally added as supported, until reindexing occurs, when
// they will be fully enabled
await UpdateSearchParameterStatusAsync(searchParamUris, SearchParameterStatus.Supported, cancellationToken);
// However, if the SearchParameter is a duplicate of an existing parameter, it will be added
// duplicate status to ensure it does not trigger a Reindexing operation
await UpdateSearchParameterStatusAsync(searchParamUris, status, cancellationToken);
}

internal async Task DeleteSearchParameterStatusAsync(string url, CancellationToken cancellationToken)
Expand Down
9 changes: 9 additions & 0 deletions src/Microsoft.Health.Fhir.Core/Resources.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions src/Microsoft.Health.Fhir.Core/Resources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -736,6 +736,10 @@
<value>A resource should only appear once in each Bundle.</value>
<comment>Error message for a duplicate resource key in the same bundle</comment>
</data>
<data name="SearchParameterStatusManagerDuplicate" xml:space="preserve">
<value>SearchParameter with Uri: {0} is a duplicate of another search parameter and cannot have it's status changed.</value>
<comment>Uri of duplicate search parameter.</comment>
</data>
<data name="PartialDeleteSuccess" xml:space="preserve">
<value>Deleted {0} versions of the target resource.</value>
<comment>{0} is replaced with the number of deleted versions of the resource.</comment>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,7 @@ public async Task<IActionResult> Create([FromBody] Resource resource)
[ConditionalConstraint]
[Route(KnownRoutes.ResourceType)]
[AuditEventType(AuditEventSubType.ConditionalCreate)]
[ServiceFilter(typeof(SearchParameterFilterAttribute))]
public async Task<IActionResult> ConditionalCreate([FromBody] Resource resource)
{
StringValues conditionalCreateHeader = HttpContext.Request.Headers[KnownHeaders.IfNoneExist];
Expand Down Expand Up @@ -218,6 +219,7 @@ public async Task<IActionResult> ConditionalCreate([FromBody] Resource resource)
[ValidateResourceIdFilter]
[Route(KnownRoutes.ResourceTypeById)]
[AuditEventType(AuditEventSubType.Update)]
[ServiceFilter(typeof(SearchParameterFilterAttribute))]
public async Task<IActionResult> Update([FromBody] Resource resource, [ModelBinder(typeof(WeakETagBinder))] WeakETag ifMatchHeader)
{
SaveOutcome response = await _mediator.UpsertResourceAsync(
Expand All @@ -234,6 +236,7 @@ public async Task<IActionResult> Update([FromBody] Resource resource, [ModelBind
[HttpPut]
[Route(KnownRoutes.ResourceType)]
[AuditEventType(AuditEventSubType.ConditionalUpdate)]
[ServiceFilter(typeof(SearchParameterFilterAttribute))]
public async Task<IActionResult> ConditionalUpdate([FromBody] Resource resource)
{
SetupConditionalRequestWithQueryOptimizeConcurrency();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,11 @@ public void Load(IServiceCollection services)
services.AddTransient(typeof(IPipelineBehavior<CreateResourceRequest, UpsertResourceResponse>), typeof(CreateOrUpdateSearchParameterBehavior<CreateResourceRequest, UpsertResourceResponse>));
services.AddTransient(typeof(IPipelineBehavior<UpsertResourceRequest, UpsertResourceResponse>), typeof(CreateOrUpdateSearchParameterBehavior<UpsertResourceRequest, UpsertResourceResponse>));
services.AddTransient(typeof(IPipelineBehavior<DeleteResourceRequest, DeleteResourceResponse>), typeof(DeleteSearchParameterBehavior<DeleteResourceRequest, DeleteResourceResponse>));

services.Add<SearchParameterConflictingCodeValidator>()
.Singleton()
.AsSelf()
.AsService<ISearchParameterConflictingCodeValidator>();
}
}
}
Loading
Loading