Skip to content

fixes a bug where OData Count path items would be missing from the description #141

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

Merged
merged 16 commits into from
Nov 24, 2021
Merged

fixes a bug where OData Count path items would be missing from the description #141

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
883ca92
- pattern matching refactoring
baywet Nov 19, 2021
d2bb4bd
- adds launch configuration for update docs tool
baywet Nov 19, 2021
70aefe9
- adds update docs to solution file so debug works properly
baywet Nov 22, 2021
960b31b
- code linting
baywet Nov 22, 2021
f08be64
- code linting
baywet Nov 22, 2021
ae11d65
- fixes #129 a bug where odata count paths would be missing from the …
baywet Nov 22, 2021
2fee91b
- code linting
baywet Nov 22, 2021
76cfc30
- switches to a referenced schema for count operations
baywet Nov 22, 2021
10b99ca
- updates unit tests for dolarr count fix
baywet Nov 22, 2021
032fbae
- fixes a bug where the operation id for count operations would not b…
baywet Nov 22, 2021
f87c2b4
- updates basic open api integration test for $count
baywet Nov 22, 2021
4093249
- adds a setting to globally disable dollar count path items
baywet Nov 23, 2021
5d910fd
- adds support for reading annotations for count restrictions for cou…
baywet Nov 23, 2021
892003e
- updates integration test files
baywet Nov 23, 2021
a8bb14f
- applies code review suggestions
baywet Nov 23, 2021
00b605f
- renames dollar count segment setting
baywet Nov 23, 2021
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
10 changes: 10 additions & 0 deletions .vscode/launch.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,16 @@
{
"version": "0.2.0",
"configurations": [
{
"name": "Launch Update Docs",
"type": "coreclr",
"request": "launch",
"preLaunchTask": "build",
"program": "${workspaceFolder}/tool/UpdateDocs/bin/Debug/net6.0/UpdateDocs.dll",
"cwd": "${workspaceFolder}/tool/UpdateDocs/bin/Debug/net6.0",
"console": "internalConsole",
"stopAtEntry": false
},
{
"name": ".NET Core Attach",
"type": "coreclr",
Expand Down
11 changes: 11 additions & 0 deletions Microsoft.OpenApi.OData.sln
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Solution Items", "Solution
.editorconfig = .editorconfig
EndProjectSection
EndProject
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "tool", "tool", "{DE8F8E75-A119-4CF3-AFDD-4132B55DAE76}"
Copy link
Contributor

Choose a reason for hiding this comment

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

I remember I have a separate solution for updateDocs.

Do you think it's convenient to have it into the main solution? Because it's only a helper tooling.

Copy link
Member Author

Choose a reason for hiding this comment

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

Having it in the main solution makes f5 debug work for VS code

Copy link
Member Author

Choose a reason for hiding this comment

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

Also enables static analysis and more for that project with CI

EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "UpdateDocs", "tool\UpdateDocs\UpdateDocs.csproj", "{AAC31ECB-05F9-444A-9B86-42ECD50AA468}"
EndProject
Global
GlobalSection(SolutionConfigurationPlatforms) = preSolution
Debug|Any CPU = Debug|Any CPU
Expand All @@ -38,11 +42,18 @@ Global
{79B190E8-EDB0-4C03-8FD8-EB48E4807CFB}.Debug|Any CPU.Build.0 = Debug|Any CPU
{79B190E8-EDB0-4C03-8FD8-EB48E4807CFB}.Release|Any CPU.ActiveCfg = Release|Any CPU
{79B190E8-EDB0-4C03-8FD8-EB48E4807CFB}.Release|Any CPU.Build.0 = Release|Any CPU
{AAC31ECB-05F9-444A-9B86-42ECD50AA468}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{AAC31ECB-05F9-444A-9B86-42ECD50AA468}.Debug|Any CPU.Build.0 = Debug|Any CPU
{AAC31ECB-05F9-444A-9B86-42ECD50AA468}.Release|Any CPU.ActiveCfg = Release|Any CPU
{AAC31ECB-05F9-444A-9B86-42ECD50AA468}.Release|Any CPU.Build.0 = Release|Any CPU
EndGlobalSection
GlobalSection(SolutionProperties) = preSolution
HideSolutionNode = FALSE
EndGlobalSection
GlobalSection(ExtensibilityGlobals) = postSolution
SolutionGuid = {9AE22713-F94E-45CA-81F4-0806CA195B69}
EndGlobalSection
GlobalSection(NestedProjects) = preSolution
{AAC31ECB-05F9-444A-9B86-42ECD50AA468} = {DE8F8E75-A119-4CF3-AFDD-4132B55DAE76}
EndGlobalSection
EndGlobal
5 changes: 5 additions & 0 deletions src/Microsoft.OpenApi.OData.Reader/Common/Constants.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,5 +79,10 @@ internal static class Constants
/// extension for discriminator value support
/// </summary>
public static string xMsDiscriminatorValue = "x-ms-discriminator-value";

/// <summary>
/// Name used for the OpenAPI referenced schema for OData Count operations responses.
/// </summary>
public static string DollarCountSchemaName = "ODataCountResponse";
}
}
5 changes: 5 additions & 0 deletions src/Microsoft.OpenApi.OData.Reader/Edm/ODataDollarCountSegment.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@ namespace Microsoft.OpenApi.OData.Edm
/// </summary>
public class ODataDollarCountSegment : ODataSegment
{
/// <summary>
/// Get the static instance of $count segment.
/// </summary>
internal static ODataDollarCountSegment Instance = new();

/// <inheritdoc />
public override ODataSegmentKind Kind => ODataSegmentKind.DollarCount;

Expand Down
4 changes: 2 additions & 2 deletions src/Microsoft.OpenApi.OData.Reader/Edm/ODataPath.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,8 +146,8 @@ public string GetPathItemName(OpenApiConvertSettings settings)

// From Open API spec, parameter name is case sensitive, so don't use the IgnoreCase HashSet.
// HashSet<string> parameters = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
HashSet<string> parameters = new HashSet<string>();
StringBuilder sb = new StringBuilder();
HashSet<string> parameters = new();
Copy link
Contributor

Choose a reason for hiding this comment

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

If possible, let's keep it unchanged?

Copy link
Member Author

Choose a reason for hiding this comment

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

what's the harm in using the newer syntax that saves code writing and removes squiggles when we're editing the code?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it can save a lot of time. It's the same as:

var parameters = new HashSet<string>();

Actually, it's no harm, but for readability.

StringBuilder sb = new();

if (!string.IsNullOrWhiteSpace(settings.PathPrefix))
{
Expand Down
90 changes: 58 additions & 32 deletions src/Microsoft.OpenApi.OData.Reader/Edm/ODataPathProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ public virtual IEnumerable<ODataPath> GetPaths(IEdmModel model, OpenApiConvertSe
{
if (CanFilter(entitySet))
{
RetrieveNavigationSourcePaths(entitySet);
RetrieveNavigationSourcePaths(entitySet, settings);
}
}

Expand All @@ -66,7 +66,7 @@ public virtual IEnumerable<ODataPath> GetPaths(IEdmModel model, OpenApiConvertSe
{
if (CanFilter(singleton))
{
RetrieveNavigationSourcePaths(singleton);
RetrieveNavigationSourcePaths(singleton, settings);
}
}

Expand Down Expand Up @@ -102,7 +102,7 @@ protected virtual void Initialize(IEdmModel model)

private IEnumerable<ODataPath> MergePaths()
{
List<ODataPath> allODataPaths = new List<ODataPath>();
List<ODataPath> allODataPaths = new();
foreach (var item in _allNavigationSourcePaths.Values)
{
allODataPaths.AddRange(item);
Expand All @@ -127,6 +127,7 @@ private void AppendPath(ODataPath path)
ODataPathKind kind = path.Kind;
switch(kind)
{
case ODataPathKind.DollarCount:
case ODataPathKind.Entity:
case ODataPathKind.EntitySet:
case ODataPathKind.Singleton:
Expand All @@ -143,8 +144,7 @@ private void AppendPath(ODataPath path)

case ODataPathKind.NavigationProperty:
case ODataPathKind.Ref:
ODataNavigationPropertySegment navigationPropertySegment = path.Last(p => p is ODataNavigationPropertySegment)
as ODataNavigationPropertySegment;
ODataNavigationPropertySegment navigationPropertySegment = path.OfType<ODataNavigationPropertySegment>().Last();

if (!_allNavigationPropertyPaths.TryGetValue(navigationPropertySegment.EntityType, out IList<ODataPath> npList))
{
Expand All @@ -169,20 +169,26 @@ private void AppendPath(ODataPath path)
/// Retrieve the paths for <see cref="IEdmNavigationSource"/>.
/// </summary>
/// <param name="navigationSource">The navigation source.</param>
private void RetrieveNavigationSourcePaths(IEdmNavigationSource navigationSource)
/// <param name="convertSettings">The settings for the current conversion.</param>
private void RetrieveNavigationSourcePaths(IEdmNavigationSource navigationSource, OpenApiConvertSettings convertSettings)
{
Debug.Assert(navigationSource != null);

// navigation source itself
ODataPath path = new ODataPath(new ODataNavigationSourceSegment(navigationSource));
ODataPath path = new(new ODataNavigationSourceSegment(navigationSource));
AppendPath(path.Clone());

IEdmEntitySet entitySet = navigationSource as IEdmEntitySet;
IEdmEntityType entityType = navigationSource.EntityType();
CountRestrictionsType count = null;

// for entity set, create a path with key
// for entity set, create a path with key and a $count path
if (entitySet != null)
{
count = _model.GetRecord<CountRestrictionsType>(entitySet, CapabilitiesConstants.CountRestrictions);
Copy link
Contributor

Choose a reason for hiding this comment

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

shall we add a config in the settings to allow/disallow the $count segment

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I see that. IncludeDollarCountPathSegments.
But, why don't you check it then call "GetRecord". If it's not allowed, we don't need to call "GetRecord".

Copy link
Member Author

Choose a reason for hiding this comment

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

the setting is already read in the CreateCountPath method

Copy link
Contributor

Choose a reason for hiding this comment

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

if (setting.IncludeDollarCountPathSegments)
{
call CreateCountPath(...)
}

in the CreateCountPath()
{
Get CountRestirction()
then do something based on the CountRestriciton?
}

Copy link
Member Author

Choose a reason for hiding this comment

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

why read the setting in the before the call to CreateCountPath? It's only going to lead to code/logic duplication.

if(count?.Countable ?? true)
CreateCountPath(path, convertSettings);

path.Push(new ODataKeySegment(entityType));
AppendPath(path.Clone());
}
Expand All @@ -195,7 +201,7 @@ private void RetrieveNavigationSourcePaths(IEdmNavigationSource navigationSource
{
if (CanFilter(np))
{
RetrieveNavigationPropertyPaths(np, path);
RetrieveNavigationPropertyPaths(np, count, path, convertSettings);
}
}

Expand Down Expand Up @@ -249,8 +255,10 @@ private void RetrieveMediaEntityStreamPaths(IEdmEntityType entityType, ODataPath
/// Retrieve the path for <see cref="IEdmNavigationProperty"/>.
/// </summary>
/// <param name="navigationProperty">The navigation property.</param>
/// <param name="count">The count restrictions.</param>
/// <param name="currentPath">The current OData path.</param>
private void RetrieveNavigationPropertyPaths(IEdmNavigationProperty navigationProperty, ODataPath currentPath)
/// <param name="convertSettings">The settings for the current conversion.</param>
private void RetrieveNavigationPropertyPaths(IEdmNavigationProperty navigationProperty, CountRestrictionsType count, ODataPath currentPath, OpenApiConvertSettings convertSettings)
{
Debug.Assert(navigationProperty != null);
Debug.Assert(currentPath != null);
Expand All @@ -275,6 +283,15 @@ private void RetrieveNavigationPropertyPaths(IEdmNavigationProperty navigationPr
if (restriction == null || restriction.IndexableByKey == true)
{
IEdmEntityType navEntityType = navigationProperty.ToEntityType();
var targetsMany = navigationProperty.TargetMultiplicity() == EdmMultiplicity.Many;
var propertyPath = navigationProperty.GetPartnerPath()?.Path;

if (targetsMany && (string.IsNullOrEmpty(propertyPath) ||
(count?.IsNonCountableNavigationProperty(propertyPath) ?? true)))
{
// ~/entityset/{key}/collection-valued-Nav/$count
CreateCountPath(currentPath, convertSettings);
}

if (!navigationProperty.ContainsTarget)
{
Expand All @@ -283,7 +300,7 @@ private void RetrieveNavigationPropertyPaths(IEdmNavigationProperty navigationPr
// Collection-valued: ~/entityset/{key}/collection-valued-Nav/$ref?$id ={navKey}
CreateRefPath(currentPath);

if (navigationProperty.TargetMultiplicity() == EdmMultiplicity.Many)
if (targetsMany)
{
// Collection-valued: DELETE ~/entityset/{key}/collection-valued-Nav/{key}/$ref
currentPath.Push(new ODataKeySegment(navEntityType));
Expand All @@ -296,7 +313,7 @@ private void RetrieveNavigationPropertyPaths(IEdmNavigationProperty navigationPr
else
{
// append a navigation property key.
if (navigationProperty.TargetMultiplicity() == EdmMultiplicity.Many)
if (targetsMany)
{
currentPath.Push(new ODataKeySegment(navEntityType));
AppendPath(currentPath.Clone());
Expand All @@ -312,13 +329,13 @@ private void RetrieveNavigationPropertyPaths(IEdmNavigationProperty navigationPr
{
if (CanFilter(subNavProperty))
{
RetrieveNavigationPropertyPaths(subNavProperty, currentPath);
RetrieveNavigationPropertyPaths(subNavProperty, count, currentPath, convertSettings);
}
}
}
}

if (navigationProperty.TargetMultiplicity() == EdmMultiplicity.Many)
if (targetsMany)
{
currentPath.Pop();
}
Expand Down Expand Up @@ -361,6 +378,21 @@ private void CreateRefPath(ODataPath currentPath)
AppendPath(newPath);
}

/// <summary>
/// Create $count paths.
/// </summary>
/// <param name="currentPath">The current OData path.</param>
/// <param name="convertSettings">The settings for the current conversion.</param>
private void CreateCountPath(ODataPath currentPath, OpenApiConvertSettings convertSettings)
{
if(currentPath == null) throw new ArgumentNullException(nameof(currentPath));
Copy link
Contributor

Choose a reason for hiding this comment

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

make a better format

Copy link
Contributor

Choose a reason for hiding this comment

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

for others

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm assuming that you're referring to the curly brace not being on the next line. Fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

if(currentPath == null)
{
throw new ArgumentNullException(nameof(currentPath));
}

and, maybe to use Error.ArgumentNull(...) method?

if(convertSettings == null) throw new ArgumentNullException(nameof(convertSettings));
if(!convertSettings.EnableDollarCountPath) return;
var countPath = currentPath.Clone();
countPath.Push(ODataDollarCountSegment.Instance);
AppendPath(countPath);
}

/// <summary>
/// Retrieve all bounding <see cref="IEdmOperation"/>.
/// </summary>
Expand Down Expand Up @@ -436,7 +468,11 @@ private void RetrieveBoundOperationPaths(OpenApiConvertSettings convertSettings)
}
}
}

private static readonly HashSet<ODataPathKind> _oDataPathKindsToSkipForOperations = new HashSet<ODataPathKind>() {
ODataPathKind.EntitySet,
ODataPathKind.MediaEntity,
ODataPathKind.DollarCount
};
private bool AppendBoundOperationOnNavigationSourcePath(IEdmOperation edmOperation, bool isCollection, IEdmEntityType bindingEntityType)
{
bool found = false;
Expand All @@ -448,8 +484,7 @@ private bool AppendBoundOperationOnNavigationSourcePath(IEdmOperation edmOperati
foreach (var subPath in value)
{
if ((isCollection && subPath.Kind == ODataPathKind.EntitySet) ||
(!isCollection && subPath.Kind != ODataPathKind.EntitySet &&
subPath.Kind != ODataPathKind.MediaEntity))
(!isCollection && !_oDataPathKindsToSkipForOperations.Contains(subPath.Kind)))
{
ODataPath newPath = subPath.Clone();
newPath.Push(new ODataOperationSegment(edmOperation, isEscapedFunction));
Expand All @@ -461,21 +496,18 @@ private bool AppendBoundOperationOnNavigationSourcePath(IEdmOperation edmOperati

return found;
}

private static readonly HashSet<ODataPathKind> _pathKindToSkipForNavigationProperties = new () {
ODataPathKind.Ref,
};
private bool AppendBoundOperationOnNavigationPropertyPath(IEdmOperation edmOperation, bool isCollection, IEdmEntityType bindingEntityType)
{
bool found = false;
bool isEscapedFunction = _model.IsUrlEscapeFunction(edmOperation);

if (_allNavigationPropertyPaths.TryGetValue(bindingEntityType, out IList<ODataPath> value))
{
foreach (var path in value)
foreach (var path in value.Where(x => !_pathKindToSkipForNavigationProperties.Contains(x.Kind)))
{
if (path.Kind == ODataPathKind.Ref)
{
continue;
}

ODataNavigationPropertySegment npSegment = path.Segments.Last(s => s is ODataNavigationPropertySegment) as ODataNavigationPropertySegment;

if (!npSegment.NavigationProperty.ContainsTarget)
Expand Down Expand Up @@ -596,15 +628,9 @@ private bool AppendBoundOperationOnDerivedNavigationPropertyPath(
{
if (_allNavigationPropertyPaths.TryGetValue(baseType, out IList<ODataPath> paths))
{
foreach (var path in paths)
foreach (var path in paths.Where(x => !_pathKindToSkipForNavigationProperties.Contains(x.Kind)))
{
if (path.Kind == ODataPathKind.Ref)
{
continue;
}

var npSegment = path.Segments.Last(s => s is ODataNavigationPropertySegment)
as ODataNavigationPropertySegment;
var npSegment = path.Segments.OfType<ODataNavigationPropertySegment>().LastOrDefault();
if (npSegment == null)
{
continue;
Expand Down
Loading