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

Add support for $count in navigation properties #2051

Merged
merged 9 commits into from
Apr 22, 2021
Merged
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
2 changes: 2 additions & 0 deletions src/Microsoft.OData.Core/Microsoft.OData.Core.cs
Original file line number Diff line number Diff line change
Expand Up @@ -791,7 +791,9 @@ internal sealed class TextRes {
internal const string ExceptionUtils_CheckLongPositive = "ExceptionUtils_CheckLongPositive";
internal const string ExceptionUtils_ArgumentStringNullOrEmpty = "ExceptionUtils_ArgumentStringNullOrEmpty";
internal const string ExpressionToken_OnlyRefAllowWithStarInExpand = "ExpressionToken_OnlyRefAllowWithStarInExpand";
internal const string ExpressionToken_DollarCountNotAllowedInSelect = "ExpressionToken_DollarCountNotAllowedInSelect";
internal const string ExpressionToken_NoPropAllowedAfterRef = "ExpressionToken_NoPropAllowedAfterRef";
internal const string ExpressionToken_NoPropAllowedAfterDollarCount = "ExpressionToken_NoPropAllowedAfterDollarCount";
internal const string ExpressionToken_NoSegmentAllowedBeforeStarInExpand = "ExpressionToken_NoSegmentAllowedBeforeStarInExpand";
internal const string ExpressionToken_IdentifierExpected = "ExpressionToken_IdentifierExpected";
internal const string ExpressionLexer_UnterminatedStringLiteral = "ExpressionLexer_UnterminatedStringLiteral";
Expand Down
2 changes: 2 additions & 0 deletions src/Microsoft.OData.Core/Microsoft.OData.Core.txt
Original file line number Diff line number Diff line change
Expand Up @@ -915,6 +915,8 @@ ExceptionUtils_CheckLongPositive=A positive long value was expected; however, th
ExceptionUtils_ArgumentStringNullOrEmpty=Value cannot be null or empty.

ExpressionToken_OnlyRefAllowWithStarInExpand=Only $ref is allowed with star in $expand option.
ExpressionToken_DollarCountNotAllowedInSelect=$count is not allowed in $select option.
ExpressionToken_NoPropAllowedAfterDollarCount=No property is allowed after $count segment.
ExpressionToken_NoPropAllowedAfterRef=No property is allowed after $ref segment.
ExpressionToken_NoSegmentAllowedBeforeStarInExpand=No segment is allowed before star in $expand.
ExpressionToken_IdentifierExpected=An identifier was expected at position {0}.
Expand Down
22 changes: 22 additions & 0 deletions src/Microsoft.OData.Core/Parameterized.Microsoft.OData.Core.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6928,6 +6928,28 @@ internal static string ExpressionToken_NoPropAllowedAfterRef
}
}

/// <summary>
/// A string like "No property is allowed after $count segment."
/// </summary>
internal static string ExpressionToken_NoPropAllowedAfterDollarCount
{
get
{
return Microsoft.OData.TextRes.GetString(Microsoft.OData.TextRes.ExpressionToken_NoPropAllowedAfterDollarCount);
}
}

/// <summary>
/// A string like "$count is not allowed in $select option."
/// </summary>
internal static string ExpressionToken_DollarCountNotAllowedInSelect
{
get
{
return Microsoft.OData.TextRes.GetString(Microsoft.OData.TextRes.ExpressionToken_DollarCountNotAllowedInSelect);
}
}

/// <summary>
/// A string like "No segment is allowed before star in $expand."
/// </summary>
Expand Down
11 changes: 11 additions & 0 deletions src/Microsoft.OData.Core/UriParser/Binders/SelectExpandBinder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -376,13 +376,19 @@ private SelectItem GenerateExpandItem(ExpandTermToken tokenIn)
}

bool isRef = false;
bool isCount = false;

if (firstNonTypeToken.NextToken != null)
{
// lastly... make sure that, since we're on a NavProp, that the next token isn't null.
if (firstNonTypeToken.NextToken.Identifier == UriQueryConstants.RefSegment)
{
isRef = true;
}
else if (firstNonTypeToken.NextToken.Identifier == UriQueryConstants.CountSegment)
{
isCount = true;
}
else
{
throw new ODataException(ODataErrorStrings.ExpandItemBinder_TraversingMultipleNavPropsInTheSamePath);
Expand Down Expand Up @@ -428,6 +434,11 @@ private SelectItem GenerateExpandItem(ExpandTermToken tokenIn)
return new ExpandedReferenceSelectItem(pathToNavProp, targetNavigationSource, filterOption, orderbyOption, tokenIn.TopOption, tokenIn.SkipOption, tokenIn.CountQueryOption, searchOption, computeOption, applyOption);
}

if (isCount)
{
return new ExpandedCountSelectItem(pathToNavProp, targetNavigationSource, filterOption, searchOption);
}

// $select & $expand
SelectExpandClause subSelectExpand = BindSelectExpand(tokenIn.ExpandOption, tokenIn.SelectOption, parsedPath, this.ResourcePathNavigationSource, targetNavigationSource, null, generatedProperties, collapsed);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,25 +109,37 @@ private void CheckPathLength(int pathLength)
/// <returns>A parsed PathSegmentToken representing the next segment in this path.</returns>
private PathSegmentToken ParseSegment(PathSegmentToken previousSegment, bool allowRef)
{
// TODO $count is defined in specification for expand, it is not supported now. Also note $count is not supported with star as expand option.
if (this.lexer.CurrentToken.Text.StartsWith("$", StringComparison.Ordinal) && (!allowRef || this.lexer.CurrentToken.Text != UriQueryConstants.RefSegment))
if (this.lexer.CurrentToken.Text.StartsWith("$", StringComparison.Ordinal)
&& (!allowRef || this.lexer.CurrentToken.Text != UriQueryConstants.RefSegment)
&& this.lexer.CurrentToken.Text != UriQueryConstants.CountSegment)
Comment on lines +112 to +114
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain the logic here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If Path segment starts with $, it should only have $count or $ref when allowRef is true
e.g
$expand=NavProp/$ref
$expand=NavProp/$count

{
throw new ODataException(ODataErrorStrings.UriSelectParser_SystemTokenInSelectExpand(this.lexer.CurrentToken.Text, this.lexer.ExpressionText));
}

// Some check here to throw exception, both prop1/*/prop2 and */$ref/prop will throw exception, both are for $expand cases
// Some check here to throw exception, prop1/*/prop2 and */$ref/prop and prop1/$count/prop2 will throw exception, all are $expand cases.
if (!isSelect)
{
if (previousSegment != null && previousSegment.Identifier == UriQueryConstants.Star && this.lexer.CurrentToken.GetIdentifier() != UriQueryConstants.RefSegment)
{
// Star can only be followed with $ref
// Star can only be followed with $ref. $count is not supported with star as expand option
throw new ODataException(ODataErrorStrings.ExpressionToken_OnlyRefAllowWithStarInExpand);
}
else if (previousSegment != null && previousSegment.Identifier == UriQueryConstants.RefSegment)
{
// $ref should not have more property followed.
throw new ODataException(ODataErrorStrings.ExpressionToken_NoPropAllowedAfterRef);
}
else if (previousSegment != null && previousSegment.Identifier == UriQueryConstants.CountSegment)
{
// $count should not have more property followed. e.g $expand=NavProperty/$count/MyProperty
throw new ODataException(ODataErrorStrings.ExpressionToken_NoPropAllowedAfterDollarCount);
}
}

if (this.lexer.CurrentToken.Text == UriQueryConstants.CountSegment && isSelect)
{
// $count is not allowed in $select e.g $select=NavProperty/$count
throw new ODataException(ODataErrorStrings.ExpressionToken_DollarCountNotAllowedInSelect);
}

string propertyName;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
//---------------------------------------------------------------------
// <copyright file="ExpandedCountSelectItem.cs" company="Microsoft">
// Copyright (C) Microsoft Corporation. All rights reserved. See License.txt in the project root for license information.
// </copyright>
//---------------------------------------------------------------------

namespace Microsoft.OData.UriParser
{
using Aggregation;
using Microsoft.OData.Edm;

/// <summary>
/// This represents one level of expansion for a particular expansion tree.
/// </summary>
public sealed class ExpandedCountSelectItem : ExpandedReferenceSelectItem
Copy link
Member

Choose a reason for hiding this comment

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

Do you think it's better to derive from ExpandedReferenceSelectItem?

Or shall we have a base NavigationSelectItem, and others derived from it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The semantics are the same though.
Also, ExpandedNavigationSelectItem inherits from the ExpandedReferenceSelectItem

{
/// <summary>
/// Create an Expand item using a nav prop, its entity set and a SelectExpandClause
/// </summary>
/// <param name="pathToNavigationProperty">the path to the navigation property for this expand item, including any type segments</param>
/// <param name="navigationSource">the navigation source for this ExpandItem</param>
/// <param name="filterOption">A filter clause for this expand (can be null)</param>
/// <param name="searchOption">A search clause for this expand (can be null)</param>
/// <exception cref="System.ArgumentNullException">Throws if input pathToNavigationProperty is null.</exception>
public ExpandedCountSelectItem(ODataExpandPath pathToNavigationProperty, IEdmNavigationSource navigationSource, FilterClause filterOption, SearchClause searchOption)
Copy link
Member

Choose a reason for hiding this comment

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

Only $filter and $search are allowed within nav/$count?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes: From the protocol http://docs.oasis-open.org/odata/odata/v4.01/cs01/part1-protocol/odata-v4.01-cs01-part1-protocol.html#sec_RequestingtheNumberofItemsinaCollect

On success, the response body MUST contain the exact count of items matching the request after applying any $filter or $search system query options, formatted as a simple primitive integer value with media type text/plain. Clients SHOULD NOT combine the system query options $top, $skip, $orderby, $expand, and $format with the path suffix /$count. The result of such a request is undefined.

Comment on lines +17 to +25
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be helpful if you could provide an concrete example of a path that this class represents, and how the parts of the path that the different parameters represent.

: base(pathToNavigationProperty, navigationSource, filterOption, null, null, null, null, searchOption)
{
ExceptionUtils.CheckArgumentNotNull(pathToNavigationProperty, "pathToNavigationProperty");
}

/// <summary>
/// Translate using a <see cref="SelectItemTranslator{T}"/>.
/// </summary>
/// <typeparam name="T">Type that the translator will return after visiting this item.</typeparam>
/// <param name="translator">An implementation of the translator interface.</param>
/// <returns>An object whose type is determined by the type parameter of the translator.</returns>
/// <exception cref="System.ArgumentNullException">Throws if the input translator is null.</exception>
public override T TranslateWith<T>(SelectItemTranslator<T> translator)
{
return translator.Translate(this);
}

/// <summary>
/// Handle using a <see cref="SelectItemHandler"/>.
/// </summary>
/// <param name="handler">An implementation of the handler interface.</param>
/// <exception cref="System.ArgumentNullException">Throws if the input handler is null.</exception>
public override void HandleWith(SelectItemHandler handler)
{
handler.Handle(this);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,13 @@ public void SelectPropertiesWithRefOperationThrows()
readResult.Throws<ODataException>(ODataErrorStrings.UriSelectParser_SystemTokenInSelectExpand("$ref", "MyLions/$ref"));
}

[Fact]
public void SelectPropertiesWithDollarCountOperationThrows()
{
Action readResult = () => RunParseSelectExpand("MyLions/$count", null, HardCodedTestModel.GetPersonType(), HardCodedTestModel.GetPeopleSet());
readResult.Throws<ODataException>(ODataErrorStrings.ExpressionToken_DollarCountNotAllowedInSelect);
}

[Fact]
public void SelectWithAsteriskMeansWildcard()
{
Expand Down Expand Up @@ -572,6 +579,14 @@ public void ExpandNavigationWithNavigationAfterRefOperationThrows()
readResult.Throws<ODataException>(ODataErrorStrings.ExpressionToken_NoPropAllowedAfterRef);
}

[Fact]
public void ExpandNavigationWithNavigationAfterDollarCountOperationThrows()
{
const string expandClauseText = "MyDog/$count/MyPeople";
Action readResult = () => RunParseSelectExpand(null, expandClauseText, HardCodedTestModel.GetPersonType(), HardCodedTestModel.GetPeopleSet());
readResult.Throws<ODataException>(ODataErrorStrings.ExpressionToken_NoPropAllowedAfterDollarCount);
}

[Fact]
public void ExpandNavigationWithNestedQueryOptionOnRef()
{
Expand Down Expand Up @@ -1582,10 +1597,76 @@ public void SelectWithNestedTopSkipAndCountWorks()
Assert.NotNull(pathSelectItem.TopOption);
Assert.Equal(4, pathSelectItem.TopOption);

Assert.NotNull(pathSelectItem.TopOption);
Assert.NotNull(pathSelectItem.SkipOption);
Assert.Equal(2, pathSelectItem.SkipOption);
}

// $expand=navProp/$count
[Fact]
public void ExpandWithNavigationPropCountWorks()
{
// Arrange
var odataQueryOptionParser = new ODataQueryOptionParser(HardCodedTestModel.TestModel,
HardCodedTestModel.GetPersonType(), HardCodedTestModel.GetPeopleSet(),
new Dictionary<string, string>()
{
{"$expand", "MyPaintings/$count"}
});

// Act
var selectExpandClause = odataQueryOptionParser.ParseSelectAndExpand();

// Assert
Assert.NotNull(selectExpandClause);
ExpandedCountSelectItem expandedCountSelectItem = Assert.IsType<ExpandedCountSelectItem>(Assert.Single(selectExpandClause.SelectedItems));
Assert.Null(expandedCountSelectItem.FilterOption);
Assert.Null(expandedCountSelectItem.SearchOption);
}

// $expand=navProp/$count($filter=prop)
[Fact]
public void ExpandWithNavigationPropCountWithFilterOptionWorks()
{
// Arrange
var odataQueryOptionParser = new ODataQueryOptionParser(HardCodedTestModel.TestModel,
HardCodedTestModel.GetPersonType(), HardCodedTestModel.GetPeopleSet(),
new Dictionary<string, string>()
{
{"$expand", "MyPaintings/$count($filter=Artist eq 'Artist One')"}
});

// Act
var selectExpandClause = odataQueryOptionParser.ParseSelectAndExpand();

// Assert
Assert.NotNull(selectExpandClause);
ExpandedCountSelectItem expandedCountSelectItem = Assert.IsType<ExpandedCountSelectItem>(Assert.Single(selectExpandClause.SelectedItems));
Assert.NotNull(expandedCountSelectItem.FilterOption);
Assert.Null(expandedCountSelectItem.SearchOption);
}

// $expand=navProp/$count($search=prop)
[Fact]
public void ExpandWithNavigationPropCountWithSearchOptionWorks()
{
// Arrange
var odataQueryOptionParser = new ODataQueryOptionParser(HardCodedTestModel.TestModel,
HardCodedTestModel.GetPersonType(), HardCodedTestModel.GetPeopleSet(),
new Dictionary<string, string>()
{
{"$expand", "MyPaintings/$count($search=Blue)"}
});

// Act
var selectExpandClause = odataQueryOptionParser.ParseSelectAndExpand();

// Assert
Assert.NotNull(selectExpandClause);
ExpandedCountSelectItem expandedCountSelectItem = Assert.IsType<ExpandedCountSelectItem>(Assert.Single(selectExpandClause.SelectedItems));
Assert.Null(expandedCountSelectItem.FilterOption);
Assert.NotNull(expandedCountSelectItem.SearchOption);
}

[Fact]
public void SelectWithNestedSelectWorks()
{
Expand Down