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

[Do Not Review] Implementing $includes operation with paging. #4810

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
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
Expand Up @@ -9,7 +9,7 @@
using System.Web;
using EnsureThat;
using Microsoft.AspNetCore.Http;
using Microsoft.Health.Fhir.Api.Features.Routing;
using Microsoft.Health.Fhir.Core.Features.Routing;

namespace Microsoft.Health.Fhir.Api.Extensions
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using System.Net;
using System.Reflection;
Expand All @@ -17,9 +16,9 @@
using Microsoft.Health.Core.Features.Context;
using Microsoft.Health.Core.Features.Security;
using Microsoft.Health.Fhir.Api.Features.AnonymousOperations;
using Microsoft.Health.Fhir.Api.Features.Routing;
using Microsoft.Health.Fhir.Core.Features.Audit;
using Microsoft.Health.Fhir.Core.Features.Context;
using Microsoft.Health.Fhir.Core.Features.Routing;

namespace Microsoft.Health.Fhir.Api.Features.Audit
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@
using Microsoft.AspNetCore.Routing;
using Microsoft.Health.Api.Features.Audit;
using Microsoft.Health.Core.Features.Context;
using Microsoft.Health.Fhir.Api.Features.Routing;
using Microsoft.Health.Fhir.Core.Features.Context;
using Microsoft.Health.Fhir.Core.Features.Routing;

namespace Microsoft.Health.Fhir.Api.Features.Context
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using EnsureThat;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Routing;
using Microsoft.Health.Fhir.Core.Features.Routing;

namespace Microsoft.Health.Fhir.Api.Features.Routing
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using EnsureThat;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Routing;
using Microsoft.Health.Fhir.Core.Features.Routing;
using Microsoft.Health.Fhir.Core.Models;

namespace Microsoft.Health.Fhir.Api.Features.Routing
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using EnsureThat;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Routing;
using Microsoft.Health.Fhir.Core.Features.Routing;
using Microsoft.Health.Fhir.Core.Models;

namespace Microsoft.Health.Fhir.Api.Features.Routing
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
using EnsureThat;
using Microsoft.AspNetCore.Http;
using Microsoft.Extensions.Primitives;
using Microsoft.Health.Fhir.Core.Features.Routing;

namespace Microsoft.Health.Fhir.Api.Features.Routing
{
Expand Down
53 changes: 42 additions & 11 deletions src/Microsoft.Health.Fhir.Api/Features/Routing/UrlResolver.cs
Original file line number Diff line number Diff line change
Expand Up @@ -145,9 +145,9 @@
Request.Host.Value);
}

public Uri ResolveRouteUrl(IReadOnlyCollection<Tuple<string, string>> unsupportedSearchParams = null, IReadOnlyList<(SearchParameterInfo searchParameterInfo, SortOrder sortOrder)> resultSortOrder = null, string continuationToken = null, bool removeTotalParameter = false)
public Uri ResolveRouteUrl(IReadOnlyCollection<Tuple<string, string>> unsupportedSearchParams = null, IReadOnlyList<(SearchParameterInfo searchParameterInfo, SortOrder sortOrder)> resultSortOrder = null, string continuationToken = null, bool removeTotalParameter = false, string includesContinuationToken = null, string routeNameOverride = null, IDictionary<string, object> ambientRouteValuesOverride = null)
{
string routeName = _fhirRequestContextAccessor.RequestContext.RouteName;
string routeName = routeNameOverride ?? _fhirRequestContextAccessor.RequestContext.RouteName;

Debug.Assert(!string.IsNullOrWhiteSpace(routeName), "The routeName should not be null or empty.");

Expand Down Expand Up @@ -215,12 +215,29 @@
routeValues[KnownQueryParameterNames.ContinuationToken] = continuationToken;
}

if (includesContinuationToken != null)
{
routeValues[KnownQueryParameterNames.IncludesContinuationToken] = includesContinuationToken;
}

RouteValuesAddress routeValuesAddress = null;
if (ambientRouteValuesOverride != null)
{
routeValuesAddress = new RouteValuesAddress()
{
AmbientValues = new RouteValueDictionary(ambientRouteValuesOverride),
ExplicitValues = routeValues ?? new(),
RouteName = routeName,
};
}

return GetRouteUri(
ActionContext.HttpContext,
routeName,
routeValues,
Request.Scheme,
Request.Host.Value);
Request.Host.Value,
routeValuesAddress);
}

public Uri ResolveRouteNameUrl(string routeName, IDictionary<string, object> routeValues)
Expand Down Expand Up @@ -328,7 +345,7 @@
Request?.Host.Value);
}

private Uri GetRouteUri(HttpContext httpContext, string routeName, RouteValueDictionary routeValues, string scheme, string host)
private Uri GetRouteUri(HttpContext httpContext, string routeName, RouteValueDictionary routeValues, string scheme, string host, RouteValuesAddress address = null)
{
var uriString = string.Empty;

Expand All @@ -351,13 +368,27 @@
pathBase += "/";
}

uriString = _linkGenerator.GetUriByRouteValues(
httpContext,
routeName,
routeValues,
scheme,
new HostString(host),
pathBase);
if (address == null)
{
uriString = _linkGenerator.GetUriByRouteValues(
httpContext,
routeName,
routeValues,
scheme,
new HostString(host),
pathBase);
}
else
{
uriString = _linkGenerator.GetUriByAddress<RouteValuesAddress>(
httpContext,
address,
address.ExplicitValues,
address.AmbientValues,
scheme,
new HostString(host),
pathBase);
}
Comment on lines +371 to +391

Check notice

Code scanning / CodeQL

Missed ternary opportunity Note

Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
}

return new Uri(uriString);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,8 @@
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Diagnostics.HealthChecks;
using Microsoft.Extensions.Options;
using Microsoft.Health.Api.Registration;
using Microsoft.Health.Fhir.Api.Configs;
using Microsoft.Health.Fhir.Api.Features.Routing;
using Microsoft.Health.Fhir.Core.Features.Cors;
using Microsoft.Health.Fhir.Core.Features.Routing;
using Newtonsoft.Json;

namespace Microsoft.AspNetCore.Builder
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ public async Task GivenThereAreTwoPagesOfSearchResults_WhenExecuted_ThenCorrectS
// Second search returns a search result without continuation token.
_searchService.SearchAsync(
null,
Arg.Is(CreateQueryParametersExpressionWithContinuationToken(ContinuationTokenConverter.Encode(continuationToken), KnownResourceTypes.Patient)),
Arg.Is(CreateQueryParametersExpressionWithContinuationToken(ContinuationTokenEncoder.Encode(continuationToken), KnownResourceTypes.Patient)),
_cancellationToken,
true,
ResourceVersionType.Latest)
Expand Down Expand Up @@ -199,7 +199,7 @@ public async Task GivenThereAreTwoPagesOfSearchResultsWithSinceParameter_WhenExe
// Second search returns a search result without continuation token.
_searchService.SearchAsync(
null,
Arg.Is(CreateQueryParametersExpressionWithContinuationToken(ContinuationTokenConverter.Encode(continuationToken), _exportJobRecord.Since, KnownResourceTypes.Patient)),
Arg.Is(CreateQueryParametersExpressionWithContinuationToken(ContinuationTokenEncoder.Encode(continuationToken), _exportJobRecord.Since, KnownResourceTypes.Patient)),
_cancellationToken,
true)
.Returns(x =>
Expand Down Expand Up @@ -239,7 +239,7 @@ public async Task GivenThereAreMultiplePagesOfSearchResults_WhenExecuted_ThenCor
// Second search returns a search result with continuation token.
_searchService.SearchAsync(
null,
Arg.Is(CreateQueryParametersExpressionWithContinuationToken(ContinuationTokenConverter.Encode(continuationToken), KnownResourceTypes.Patient)),
Arg.Is(CreateQueryParametersExpressionWithContinuationToken(ContinuationTokenEncoder.Encode(continuationToken), KnownResourceTypes.Patient)),
_cancellationToken,
true)
.Returns(x =>
Expand All @@ -254,7 +254,7 @@ public async Task GivenThereAreMultiplePagesOfSearchResults_WhenExecuted_ThenCor
// Third search returns a search result without continuation token.
_searchService.SearchAsync(
null,
Arg.Is(CreateQueryParametersExpressionWithContinuationToken(ContinuationTokenConverter.Encode(newContinuationToken), KnownResourceTypes.Patient)),
Arg.Is(CreateQueryParametersExpressionWithContinuationToken(ContinuationTokenEncoder.Encode(newContinuationToken), KnownResourceTypes.Patient)),
_cancellationToken,
true)
.Returns(x =>
Expand Down Expand Up @@ -295,7 +295,7 @@ public async Task GivenThereAreMultiplePagesOfSearchResultsWithSinceParameter_Wh
// Second search returns a search result with continuation token.
_searchService.SearchAsync(
null,
Arg.Is(CreateQueryParametersExpressionWithContinuationToken(ContinuationTokenConverter.Encode(continuationToken), _exportJobRecord.Since, KnownResourceTypes.Patient)),
Arg.Is(CreateQueryParametersExpressionWithContinuationToken(ContinuationTokenEncoder.Encode(continuationToken), _exportJobRecord.Since, KnownResourceTypes.Patient)),
_cancellationToken,
true)
.Returns(x =>
Expand All @@ -310,7 +310,7 @@ public async Task GivenThereAreMultiplePagesOfSearchResultsWithSinceParameter_Wh
// Third search returns a search result without continuation token.
_searchService.SearchAsync(
null,
Arg.Is(CreateQueryParametersExpressionWithContinuationToken(ContinuationTokenConverter.Encode(newContinuationToken), _exportJobRecord.Since, KnownResourceTypes.Patient)),
Arg.Is(CreateQueryParametersExpressionWithContinuationToken(ContinuationTokenEncoder.Encode(newContinuationToken), _exportJobRecord.Since, KnownResourceTypes.Patient)),
_cancellationToken,
true)
.Returns(x =>
Expand Down Expand Up @@ -1340,7 +1340,7 @@ public async Task GivenAGroupExportJobToResume_WhenExecuted_ThenAllPatientResour
{
// The ids aren't in the query parameters because of the reset
ids = new string[] { "1", "2", "3" };
continuationTokenIndex = int.Parse(ContinuationTokenConverter.Decode(
continuationTokenIndex = int.Parse(ContinuationTokenEncoder.Decode(
x.ArgAt<IReadOnlyList<Tuple<string, string>>>(1)
.Where(x => x.Item1 == Core.Features.KnownQueryParameterNames.ContinuationToken)
.Select(x => x.Item2).First())[2..]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,15 @@ namespace Microsoft.Health.Fhir.Core.UnitTests.Features.Search
{
[Trait(Traits.OwningTeam, OwningTeam.Fhir)]
[Trait(Traits.Category, Categories.Search)]
public class ContinuationTokenConverterTests
public class ContinuationTokenEncoderTests
{
[Fact]
public void GivenAString_WhenEcodingAndDecoding_ThenOriginalStringIsPreserved()
{
var data = Guid.NewGuid().ToString();

var encoded = ContinuationTokenConverter.Encode(data);
var decoded = ContinuationTokenConverter.Decode(encoded);
var encoded = ContinuationTokenEncoder.Encode(data);
var decoded = ContinuationTokenEncoder.Decode(encoded);

Assert.Equal(data, decoded);
}
Expand All @@ -35,7 +35,7 @@ public void GivenAnOldStringInBase64_WhenDecoding_ThenOriginalStringIsPreserved(

var encodedPrevious = Convert.ToBase64String(Encoding.UTF8.GetBytes(data));

var decoded = ContinuationTokenConverter.Decode(encodedPrevious);
var decoded = ContinuationTokenEncoder.Decode(encodedPrevious);

Assert.Equal(data, decoded);
}
Expand All @@ -47,15 +47,15 @@ public void GivenAnInvalidString_WhenDecoding_ThenAnErrorIsThrown()

var encodedPrevious = Convert.ToBase64String(Encoding.UTF8.GetBytes(data)).Insert(5, "aaaafffff");

Assert.Throws<BadRequestException>(() => ContinuationTokenConverter.Decode(encodedPrevious));
Assert.Throws<BadRequestException>(() => ContinuationTokenEncoder.Decode(encodedPrevious));
}

[Fact]
public void GivenShortBase64WhenDecoding_ThenCorrectValueIsReturned()
{
var data = "YWJj";

var decoded = ContinuationTokenConverter.Decode(data);
var decoded = ContinuationTokenEncoder.Decode(data);

Assert.Equal("abc", decoded);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public class CoreFeatureConfiguration
/// <summary>
/// Gets or sets the default value for included search results.
/// </summary>
public int DefaultIncludeCountPerSearch { get; set; } = 100;
public int DefaultIncludeCountPerSearch { get; set; } = 500;

/// <summary>
/// Gets or sets a value whether we need to run profile validation during resource creation.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ public static class SearchServiceExtensions
var searchParameters = new List<Tuple<string, string>>(filteredParameters);
if (!string.IsNullOrEmpty(lastContinuationToken))
{
searchParameters.Add(Tuple.Create(KnownQueryParameterNames.ContinuationToken, ContinuationTokenConverter.Encode(lastContinuationToken)));
searchParameters.Add(Tuple.Create(KnownQueryParameterNames.ContinuationToken, ContinuationTokenEncoder.Encode(lastContinuationToken)));
}

statistics.Iterate();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -125,5 +125,10 @@ public static class KnownQueryParameterNames
/// Used by export to specify the number of resources to be processed by the search engine.
/// </summary>
public const string MaxCount = "_maxCount";

/// <summary>
/// The include continuation token parameter.
/// </summary>
public const string IncludesContinuationToken = "includesCt";
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -797,7 +797,7 @@ private async Task ProcessProgressChange(
{
// Update the continuation token in local cache and queryParams.
// We will add or udpate the continuation token in the query parameters list.
progress.UpdateContinuationToken(ContinuationTokenConverter.Encode(continuationToken));
progress.UpdateContinuationToken(ContinuationTokenEncoder.Encode(continuationToken));

bool replacedContinuationToken = false;
for (int index = 0; index < queryParametersList.Count; index++)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,7 @@ private async Task<ReindexJobQueryStatus> ProcessQueryAsync(ReindexJobQueryStatu
// For cases like retry or stale query we don't want to start another chain.
if (!string.IsNullOrEmpty(results?.ContinuationToken) && !query.CreatedChild)
{
var encodedContinuationToken = ContinuationTokenConverter.Encode(results.ContinuationToken);
var encodedContinuationToken = ContinuationTokenEncoder.Encode(results.ContinuationToken);
var nextQuery = new ReindexJobQueryStatus(query.ResourceType, encodedContinuationToken)
{
LastModified = Clock.UtcNow,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,11 @@ public interface IUrlResolver
/// <param name="resultSortOrder">The order the results are sorted in</param>
/// <param name="continuationToken">The continuation token.</param>
/// <param name="removeTotalParameter">True if the _total parameter should be removed from the url, false otherwise.</param>
/// <param name="includesContinuationToken">The continuation token for $includes operation.</param>
/// <param name="routeNameOverride">The route name to override one in the http context.</param>
/// <param name="ambientRouteValuesOverride">The ambient route values to override the route-values address in the http context.</param>
/// <returns>The URL.</returns>
Uri ResolveRouteUrl(IReadOnlyCollection<Tuple<string, string>> unsupportedSearchParams = null, IReadOnlyList<(SearchParameterInfo searchParameterInfo, SortOrder sortOrder)> resultSortOrder = null, string continuationToken = null, bool removeTotalParameter = false);
Uri ResolveRouteUrl(IReadOnlyCollection<Tuple<string, string>> unsupportedSearchParams = null, IReadOnlyList<(SearchParameterInfo searchParameterInfo, SortOrder sortOrder)> resultSortOrder = null, string continuationToken = null, bool removeTotalParameter = false, string includesContinuationToken = null, string routeNameOverride = null, IDictionary<string, object> ambientRouteValuesOverride = null);

/// <summary>
/// Resolves the URL for the specified routeName.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
// Licensed under the MIT License (MIT). See LICENSE in the repo root for license information.
// -------------------------------------------------------------------------------------------------

namespace Microsoft.Health.Fhir.Api.Features.Routing
namespace Microsoft.Health.Fhir.Core.Features.Routing
{
internal class KnownActionParameterNames
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

using OperationsConstants = Microsoft.Health.Fhir.Core.Features.Operations.OperationsConstants;

namespace Microsoft.Health.Fhir.Api.Features.Routing
namespace Microsoft.Health.Fhir.Core.Features.Routing
{
internal class KnownRoutes
{
Expand Down Expand Up @@ -93,5 +93,8 @@ internal class KnownRoutes
public const string BulkDeleteOperationDefinition = OperationDefinition + "/" + OperationsConstants.BulkDelete;
public const string ResourceTypeBulkDeleteOperationDefinition = OperationDefinition + "/" + OperationsConstants.ResourceTypeBulkDelete;
public const string BulkDeleteSoftDeletedOperationDefinition = OperationDefinition + "/" + OperationsConstants.BulkDeleteSoftDeleted;

public const string Includes = "$includes";
public const string IncludesResourceType = ResourceType + "/" + Includes;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why move this from API to Core?

Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
// Licensed under the MIT License (MIT). See LICENSE in the repo root for license information.
// -------------------------------------------------------------------------------------------------

namespace Microsoft.Health.Fhir.Api.Features.Routing
namespace Microsoft.Health.Fhir.Core.Features.Routing
{
internal static class RouteNames
{
Expand Down Expand Up @@ -80,5 +80,7 @@ internal static class RouteNames
internal const string BulkDeleteDefinition = nameof(BulkDeleteDefinition);

internal const string BulkDeleteSoftDeletedDefinition = nameof(BulkDeleteSoftDeletedDefinition);

internal const string Includes = nameof(Includes);
}
}
Loading
Loading