Skip to content

Allow for duplicate search parameters #3790

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

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
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(string.Format(Core.Resources.NoConverterForSearchParamType, searchParameterInfo.Type, searchParameterInfo.Expression));
}
if (!supportedResult.Supported)
{
throw new SearchParameterNotSupportedException(string.Format(Core.Resources.NoConverterForSearchParamType, searchParameterInfo.Type, searchParameterInfo.Expression));
}

// 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.LogInformation("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 @@ -174,18 +188,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 @@ -200,7 +219,10 @@ public async Task UpdateSearchParameterAsync(ITypedElement searchParam, RawResou

_logger.LogInformation("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 @@ -316,5 +338,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 @@ -138,14 +138,23 @@ public async Task UpdateSearchParameterStatusAsync(IReadOnlyCollection<string> s

foreach (string uri in searchParameterUris)
{
_logger.LogInformation("Setting the search parameter status of '{Uri}' to '{NewStatus}'", uri, status.ToString());
// 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 @@ -174,11 +183,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))]
[TypeFilter(typeof(CrudEndpointMetricEmitterAttribute))]
public async Task<IActionResult> Update([FromBody] Resource resource, [ModelBinder(typeof(WeakETagBinder))] WeakETag ifMatchHeader)
{
Expand All @@ -235,6 +237,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 @@ -156,6 +156,11 @@ public void Load(IServiceCollection services)
services.AddTransient<IPipelineBehavior<CreateResourceRequest, UpsertResourceResponse>, CreateOrUpdateSearchParameterBehavior<CreateResourceRequest, UpsertResourceResponse>>();
services.AddTransient<IPipelineBehavior<UpsertResourceRequest, UpsertResourceResponse>, CreateOrUpdateSearchParameterBehavior<UpsertResourceRequest, UpsertResourceResponse>>();
services.AddTransient<IPipelineBehavior<DeleteResourceRequest, DeleteResourceResponse>, DeleteSearchParameterBehavior<DeleteResourceRequest, DeleteResourceResponse>>();

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