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.

Sign up for GitHub

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

Jump to bottom

Server-side paging should not apply to collections of non-entity types #951

Merged
merged 1 commit into from
Jun 8, 2023
Merged

Server-side paging should not apply to collections of non-entity types #951

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
51 changes: 21 additions & 30 deletions src/Microsoft.AspNetCore.OData/Query/Expressions/SelectExpandBinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -653,15 +653,15 @@ private static DynamicPathSegment ProcessSelectedItem(PathSelectItem pathSelectI
// for $select=NS.SubType/Property, we don't care about the leading type segment, because with or without the type segment
// the "Property" property value expression should be built into the property container.

PropertySegment firstSturucturalPropertySegment = firstNonTypeSegment as PropertySegment;
if (firstSturucturalPropertySegment != null)
PropertySegment firstStructuralPropertySegment = firstNonTypeSegment as PropertySegment;
if (firstStructuralPropertySegment != null)
{
// $select=abc/..../xyz
SelectExpandIncludedProperty newPropertySelectItem;
if (!currentLevelPropertiesInclude.TryGetValue(firstSturucturalPropertySegment.Property, out newPropertySelectItem))
if (!currentLevelPropertiesInclude.TryGetValue(firstStructuralPropertySegment.Property, out newPropertySelectItem))
{
newPropertySelectItem = new SelectExpandIncludedProperty(firstSturucturalPropertySegment, navigationSource);
currentLevelPropertiesInclude[firstSturucturalPropertySegment.Property] = newPropertySelectItem;
newPropertySelectItem = new SelectExpandIncludedProperty(firstStructuralPropertySegment, navigationSource);
currentLevelPropertiesInclude[firstStructuralPropertySegment.Property] = newPropertySelectItem;
}

newPropertySelectItem.AddSubSelectItem(remainingSegments, pathSelectItem);
Expand Down Expand Up @@ -962,7 +962,7 @@ internal void BuildSelectedProperty(QueryBinderContext context, Expression sourc
}

int? modelBoundPageSize = querySettings == null ? null : querySettings.PageSize;
propertyValue = ProjectAsWrapper(context, propertyValue, subSelectExpandClause, structuralProperty.Type.ToStructuredType(), pathSelectItem.NavigationSource,
propertyValue = ProjectAsWrapper(context, propertyValue, subSelectExpandClause, propertyStructuredType, pathSelectItem.NavigationSource,
pathSelectItem.OrderByOption, // $orderby=...
pathSelectItem.ComputeOption,
pathSelectItem.TopOption, // $top=...
Expand All @@ -976,17 +976,6 @@ internal void BuildSelectedProperty(QueryBinderContext context, Expression sourc
{
propertyExpression.NullCheck = nullCheck;
}
else if (settings.PageSize.HasValue)
{
propertyExpression.PageSize = settings.PageSize.Value;
}
else
{
if (querySettings != null && querySettings.PageSize.HasValue)
{
propertyExpression.PageSize = querySettings.PageSize.Value;
}
}

propertyExpression.TotalCount = countExpression;
propertyExpression.CountOption = pathSelectItem.CountOption;
Expand Down Expand Up @@ -1197,8 +1186,7 @@ private Expression ProjectCollection(QueryBinderContext context, Expression sour
bool hasTopValue = topOption != null && topOption.HasValue;
bool hasSkipvalue = skipOption != null && skipOption.HasValue;

IEdmEntityType entityType = structuredType as IEdmEntityType;
if (entityType != null)
if (structuredType is IEdmEntityType entityType)
{
if (settings.PageSize.HasValue || modelBoundPageSize.HasValue || hasTopValue || hasSkipvalue)
{
Expand Down Expand Up @@ -1242,20 +1230,23 @@ private Expression ProjectCollection(QueryBinderContext context, Expression sour
settings.EnableConstantParameterization);
}

if (settings.PageSize.HasValue || modelBoundPageSize.HasValue || hasTopValue || hasSkipvalue)
if (structuredType is IEdmEntityType)
{
// don't page nested collections if EnableCorrelatedSubqueryBuffering is enabled
if (!settings.EnableCorrelatedSubqueryBuffering)
if (settings.PageSize.HasValue || modelBoundPageSize.HasValue)
{
if (settings.PageSize.HasValue)
// don't page nested collections if EnableCorrelatedSubqueryBuffering is enabled
if (!settings.EnableCorrelatedSubqueryBuffering)
{
source = ExpressionHelpers.Take(source, settings.PageSize.Value + 1, elementType,
settings.EnableConstantParameterization);
}
else if (settings.ModelBoundPageSize.HasValue)
{
source = ExpressionHelpers.Take(source, modelBoundPageSize.Value + 1, elementType,
settings.EnableConstantParameterization);
if (settings.PageSize.HasValue)
{
source = ExpressionHelpers.Take(source, settings.PageSize.Value + 1, elementType,
settings.EnableConstantParameterization);
}
else if (settings.ModelBoundPageSize.HasValue)
{
source = ExpressionHelpers.Take(source, modelBoundPageSize.Value + 1, elementType,
settings.EnableConstantParameterization);
}
}
}
}
Expand Down
28 changes: 28 additions & 0 deletions test/Microsoft.AspNetCore.OData.E2E.Tests/ServerSidePaging/ServerSidePagingControllers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,4 +289,32 @@ public ActionResult GetTabsFromContainmentPagingExtendedMenu()
return Ok((ribbon as ContainmentPagingExtendedMenu).Tabs);
}
}

public class CollectionPagingCustomersController : ODataController
{
private const int TargetSize = 3;
private static readonly List<CollectionPagingCustomer> customers = new List<CollectionPagingCustomer>(
Enumerable.Range(1, TargetSize).Select(idx => new CollectionPagingCustomer
{
Id = idx,
Tags = new List<string> { "Tier 1", "Gen-Z", "HNW" },
Categories = new List<CollectionPagingCategory>
{
CollectionPagingCategory.Retailer,
CollectionPagingCategory.Wholesaler,
CollectionPagingCategory.Distributor
},
Locations = new List<CollectionPagingLocation>(
Enumerable.Range(1, TargetSize).Select(dx => new CollectionPagingLocation
{
Street = $"Street {idx}{dx}"
}))
}));

[EnableQuery(PageSize = 2)]
public ActionResult Get()
{
return Ok(customers);
}
}
}
20 changes: 20 additions & 0 deletions test/Microsoft.AspNetCore.OData.E2E.Tests/ServerSidePaging/ServerSidePagingDataModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,4 +134,24 @@ public class ContainmentPagingExtendedPanel : ContainmentPagingPanel
[Contained]
public List<ContainedPagingItem> Items { get; set; }
}

public enum CollectionPagingCategory
{
Retailer,
Wholesaler,
Distributor
}

public class CollectionPagingLocation
{
public string Street { get; set; }
}

public class CollectionPagingCustomer
{
public int Id { get; set; }
public List<string> Tags { get; set; }
public List<CollectionPagingCategory> Categories { get; set; }
public List<CollectionPagingLocation> Locations { get; set; }
}
}
84 changes: 82 additions & 2 deletions test/Microsoft.AspNetCore.OData.E2E.Tests/ServerSidePaging/ServerSidePagingTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,9 @@ protected static void UpdateConfigureServices(IServiceCollection services)
typeof(ContainmentPagingCompanyController),
typeof(NoContainmentPagingCustomersController),
typeof(ContainmentPagingMenusController),
typeof(ContainmentPagingRibbonController));
services.AddControllers().AddOData(opt => opt.Expand().OrderBy().AddRouteComponents("{a}", edmModel));
typeof(ContainmentPagingRibbonController),
typeof(CollectionPagingCustomersController));
services.AddControllers().AddOData(opt => opt.Expand().OrderBy().Select().SetMaxTop(null).AddRouteComponents("{a}", edmModel));
}

protected static IEdmModel GetEdmModel()
Expand All @@ -60,6 +61,7 @@ protected static IEdmModel GetEdmModel()
builder.EntitySet<ContainmentPagingMenu>("ContainmentPagingMenus");
builder.EntitySet<ContainmentPagingPanel>("ContainmentPagingPanels");
builder.Singleton<ContainmentPagingMenu>("ContainmentPagingRibbon");
builder.EntitySet<CollectionPagingCustomer>("CollectionPagingCustomers");

var getEmployeesHiredInPeriodFunction = builder.EntitySet<ServerSidePagingEmployee>(
"ServerSidePagingEmployees").EntityType.Collection.Function("GetEmployeesHiredInPeriod");
Expand Down Expand Up @@ -438,6 +440,84 @@ public async Task VerifyExpectedNextLinksGeneratedForNonContainedNavigationPrope
Assert.Contains($"/prefix/ContainmentPagingPanels/2/{extendedPanelTypeName}/Items?$expand={extendedItemTypeName}%2FNotes&$skip=2", content);
Assert.Contains($"{menu1ResourcePath}/Panels?$expand={extendedPanelTypeName}%2FItems%28%24expand%3D{extendedItemTypeName}%2FNotes%29&$skip=2", content);
}

[Theory]
[InlineData("")]
[InlineData("?$select=Tags,Categories,Locations")]
public async Task VerifyServerSidePagingNotAppliedToNonEntityCollections(string url)
{
// Arrange
var requestUri = $"/prefix/CollectionPagingCustomers{url}";
var request = new HttpRequestMessage(HttpMethod.Get, requestUri);
var client = CreateClient();

// Act
var response = await client.SendAsync(request);
var content = await response.Content.ReadAsObject<JObject>();

// Assert
var pageResult = content.GetValue("value") as JArray;
Assert.NotNull(pageResult);
Assert.Equal(2, pageResult.Count);

foreach (JObject item in pageResult)
{
var tags = item.GetValue("Tags") as JArray;
Assert.NotNull(tags);
Assert.Equal(3, tags.Count);

var categories = item.GetValue("Categories") as JArray;
Assert.NotNull(categories);
Assert.Equal(3, categories.Count);

var locations = item.GetValue("Locations") as JArray;
Assert.NotNull(locations);
Assert.Equal(3, locations.Count);
}
}

[Fact]
public async Task VerifyClientSidePagingAppliedToNonEntityCollections()
{
// Arrange
var requestUri = "/prefix/CollectionPagingCustomers?$select=Tags($skip=1;$top=1),Categories($skip=1;$top=1),Locations($skip=1;$top=1)";
var request = new HttpRequestMessage(HttpMethod.Get, requestUri);
var client = CreateClient();

// Act
var response = await client.SendAsync(request);
var content = await response.Content.ReadAsObject<JObject>();

// Assert
var pageResult = content.GetValue("value") as JArray;
Assert.NotNull(pageResult);
Assert.Equal(2, pageResult.Count);

JObject page;
string tag;
CollectionPagingCategory? category;
CollectionPagingLocation location;

page = pageResult[0] as JObject;
tag = Assert.Single(page.GetValue("Tags")).ToObject<string>();
category = Assert.Single(page.GetValue("Categories")).ToObject<CollectionPagingCategory?>();
location = Assert.Single(page.GetValue("Locations")).ToObject<CollectionPagingLocation>();

Assert.Equal("Gen-Z", tag);
Assert.Equal(CollectionPagingCategory.Wholesaler, category);
Assert.NotNull(location);
Assert.Equal("Street 12", location.Street);

page = pageResult[1] as JObject;
tag = Assert.Single(page.GetValue("Tags")).ToObject<string>();
category = Assert.Single(page.GetValue("Categories")).ToObject<CollectionPagingCategory?>();
location = Assert.Single(page.GetValue("Locations")).ToObject<CollectionPagingLocation>();

Assert.Equal("Gen-Z", tag);
Assert.Equal(CollectionPagingCategory.Wholesaler, category);
Assert.NotNull(location);
Assert.Equal("Street 22", location.Street);
}
}

public class SkipTokenPagingTests : WebApiTestBase<SkipTokenPagingTests>
Expand Down