fixes a bug where OData Count path items would be missing from the description by baywet · Pull Request #141 · microsoft/OpenAPI.NET.OData

baywet · 2021-11-22T18:38:09Z

pattern matching refactoring
adds launch configuration for update docs tool
adds update docs to solution file so debug works properly
code linting
code linting
fixes Count path segments are missing after translation #129 a bug where odata count paths would be missing from the description

TODO

https://github.com/oasis-tcs/odata-vocabularies/blob/main/vocabularies/Org.OData.Capabilities.V1.xml#L210

Signed-off-by: Vincent Biret <vibiret@microsoft.com>

…description Signed-off-by: Vincent Biret <vibiret@microsoft.com>

Signed-off-by: Vincent Biret <vibiret@microsoft.com>

…e unique Signed-off-by: Vincent Biret <vibiret@microsoft.com>

Signed-off-by: Vincent Biret <vibiret@microsoft.com>

…nt path Signed-off-by: Vincent Biret <vibiret@microsoft.com>

Signed-off-by: Vincent Biret <vibiret@microsoft.com>

src/Microsoft.OpenApi.OData.Reader/Common/Constants.cs

src/Microsoft.OpenApi.OData.Reader/Edm/ODataDollarCountSegment.cs

xuzhg · 2021-11-23T18:24:53Z

src/Microsoft.OpenApi.OData.Reader/Edm/ODataPath.cs

            // HashSet<string> parameters = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
-            HashSet<string> parameters = new HashSet<string>();
-            StringBuilder sb = new StringBuilder();
+            HashSet<string> parameters = new();


If possible, let's keep it unchanged?

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

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.

xuzhg · 2021-11-23T18:27:08Z

src/Microsoft.OpenApi.OData.Reader/Edm/ODataPathProvider.cs

+        /// <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));


make a better format

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

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

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

src/Microsoft.OpenApi.OData.Reader/Edm/ODataPathProvider.cs

xuzhg · 2021-11-23T18:27:51Z

src/Microsoft.OpenApi.OData.Reader/Edm/ODataPathProvider.cs

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


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

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".

the setting is already read in the CreateCountPath method

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

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

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

xuzhg · 2021-11-23T18:30:24Z

src/Microsoft.OpenApi.OData.Reader/Edm/RecordExpressionExtensions.cs

-            }
-
-            return null;
+            return (record.Properties?.FirstOrDefault(e => e.Name == propertyName) is IEdmPropertyConstructor property &&


all of these code change are not related to the $count path feature.
Maybe better to have a separate PR for clarification, better understanding.

I'll try to keep that in mind for the next pull requests. I usually try to fix warnings/suggestions on the files I touch, but not the ones I don't touch. As this is my first actual PR in this codebase, I was exploring the code to understand how things work and I made a few modifications at other places. Since the work was already done, and it's only syntax change that doesn't impact the behavior, I suggest we make a one time exception. Thoughts?

xuzhg · 2021-11-23T18:33:39Z

test/Microsoft.OpenAPI.OData.Reader.Tests/Resources/Basic.OpenApi.json

        "x-ms-docs-operation-type": "operation"
      }
    },
+    "/City/$count": {


City looks like a singleton.
It's not allowed to have $count after a single navigation property or singleton.

city is actually also an entity set, arguably it should be named cities but I didn't want to impact the source file.

OpenAPI.NET.OData/test/Microsoft.OpenAPI.OData.Reader.Tests/Common/EdmModelHelper.cs

Line 273 in 1342794

EdmEntitySet cities = new EdmEntitySet(entityContainer, "City", city);

Oh, ok. My error

Signed-off-by: Vincent Biret <vibiret@microsoft.com>

xuzhg · 2021-11-23T18:43:30Z

I'd suggest some "rules" about "coding style":

leave new T() unchanged to use new(), until we have whole separate PR to clean all.
Insert white-line between paragraph.
If (....) xxxx =>
if (....)
{
...
}

even it's only one line code.

xuzhg · 2021-11-23T18:49:07Z

src/Microsoft.OpenApi.OData.Reader/OpenApiConvertSettings.cs

+        /// <summary>
+        /// Gets/sets a value indicating whether or not add OData $count segments in the description for collections.
+        /// </summary>
+        public bool IncludeDollarCountPathSegments { get; set; } = true;


IncludeDollarCountPathSegments -> EnableDollarCountPath

Signed-off-by: Vincent Biret <vibiret@microsoft.com>

baywet · 2021-11-23T19:05:57Z

@xuzhg I'm going to push back on those rules until we have a comprehensive editorconfig + linter to enforce them (and resolve them automatically). It's a poor use of our time to enforce those things manually. And I do agree on the fact that the bulk of changes + configuration should happen in a separate PR.

xuzhg · 2021-11-24T01:31:25Z

Microsoft.OpenApi.OData.sln

 		.editorconfig = .editorconfig
 	EndProjectSection
 EndProject
+Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "tool", "tool", "{DE8F8E75-A119-4CF3-AFDD-4132B55DAE76}"


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.

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

Also enables static analysis and more for that project with CI

xuzhg · 2021-11-24T01:34:01Z

src/Microsoft.OpenApi.OData.Reader/Edm/ODataPath.cs

            // HashSet<string> parameters = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
-            HashSet<string> parameters = new HashSet<string>();
-            StringBuilder sb = new StringBuilder();
+            HashSet<string> parameters = new();


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.

xuzhg · 2021-11-24T01:35:42Z

src/Microsoft.OpenApi.OData.Reader/Edm/ODataPathProvider.cs

+        /// <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));


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

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

xuzhg · 2021-11-24T01:36:28Z

src/Microsoft.OpenApi.OData.Reader/Edm/RecordExpressionExtensions.cs

-            }
-
-            return null;
+            return (record.Properties?.FirstOrDefault(e => e.Name == propertyName) is IEdmPropertyConstructor property &&


xuzhg · 2021-11-24T01:37:10Z

src/Microsoft.OpenApi.OData.Reader/Operation/DollarCountGetOperationHandler.cs

        /// </summary>
        internal ODataSegment LastSecondSegment { get; set; }
-
+        private const int SecondLastSegmentIndex = 2;


insert a new line before and after

xuzhg · 2021-11-24T01:37:23Z

src/Microsoft.OpenApi.OData.Reader/Operation/DollarCountGetOperationHandler.cs

-            {
-                Type = "integer",
-                Format = "int32"
+            OpenApiSchema schema = new()


align the codes

xuzhg · 2021-11-24T01:38:08Z

src/Microsoft.OpenApi.OData.Reader/PathItem/NavigationPropertyPathItemHandler.cs


-        /// <inheritdoc/>
-        protected override void SetOperations(OpenApiPathItem item)
+		/// <inheritdoc/>


xuzhg · 2021-11-24T01:39:00Z

test/Microsoft.OpenAPI.OData.Reader.Tests/Edm/ODataPathProviderTests.cs

+            // Arrange
+            IEdmModel model = GetInheritanceModel(string.Empty);
+            ODataPathProvider provider = new ODataPathProvider();
+            var settings = new OpenApiConvertSettings {


{ in a separate linke

xuzhg · 2021-11-24T01:39:58Z

test/Microsoft.OpenAPI.OData.Reader.Tests/Resources/Basic.OpenApi.json

        "x-ms-docs-operation-type": "operation"
      }
    },
+    "/City/$count": {


Oh, ok. My error

xuzhg · 2021-11-24T01:43:47Z

@baywet I don't like the auto-merge setting. Should turn it off.

Besides, we should merge the changes squash. It's better for one feature/one fix into one commit.

baywet · 2021-11-24T01:57:10Z

Squash: sure we can disable merge and rebase in the branch policy if you prefer linear history. Although it leads to more conflicts.

Auto-merge: what's the issue with it? It saves time to everyone.

baywet · 2021-11-24T02:00:47Z

And I can't reply to your var comment for some reason. But I saw the style was not to use var here, I usually go with the var syntax to avoid duplicating the type name

baywet self-assigned this Nov 22, 2021

baywet linked an issue Nov 22, 2021 that may be closed by this pull request

Count path segments are missing after translation #129

Closed

baywet added this to the 1.0.10 milestone Nov 22, 2021

baywet force-pushed the bugifx/count-segments branch from d09c5e9 to 2335fdd Compare November 22, 2021 19:34

baywet added 8 commits November 22, 2021 14:42

- pattern matching refactoring

883ca92

Signed-off-by: Vincent Biret <vibiret@microsoft.com>

- adds launch configuration for update docs tool

d2bb4bd

Signed-off-by: Vincent Biret <vibiret@microsoft.com>

- adds update docs to solution file so debug works properly

70aefe9

Signed-off-by: Vincent Biret <vibiret@microsoft.com>

- code linting

960b31b

Signed-off-by: Vincent Biret <vibiret@microsoft.com>

- code linting

f08be64

Signed-off-by: Vincent Biret <vibiret@microsoft.com>

- fixes #129 a bug where odata count paths would be missing from the …

ae11d65

…description Signed-off-by: Vincent Biret <vibiret@microsoft.com>

- code linting

2fee91b

Signed-off-by: Vincent Biret <vibiret@microsoft.com>

- switches to a referenced schema for count operations

76cfc30

Signed-off-by: Vincent Biret <vibiret@microsoft.com>

baywet force-pushed the bugifx/count-segments branch from 2335fdd to 76cfc30 Compare November 22, 2021 19:42

baywet added 6 commits November 22, 2021 15:06

- updates unit tests for dolarr count fix

10b99ca

Signed-off-by: Vincent Biret <vibiret@microsoft.com>

- fixes a bug where the operation id for count operations would not b…

032fbae

…e unique Signed-off-by: Vincent Biret <vibiret@microsoft.com>

- updates basic open api integration test for $count

f87c2b4

Signed-off-by: Vincent Biret <vibiret@microsoft.com>

- adds a setting to globally disable dollar count path items

4093249

Signed-off-by: Vincent Biret <vibiret@microsoft.com>

- adds support for reading annotations for count restrictions for cou…

5d910fd

…nt path Signed-off-by: Vincent Biret <vibiret@microsoft.com>

- updates integration test files

892003e

Signed-off-by: Vincent Biret <vibiret@microsoft.com>

baywet marked this pull request as ready for review November 23, 2021 16:51

baywet requested review from darrelmiller, irvinesunday and xuzhg as code owners November 23, 2021 16:51

baywet mentioned this pull request Nov 23, 2021

Support for raw content in text/plain responses microsoft/kiota#878

Closed

4 tasks

Base automatically changed from feature/net5 to master November 23, 2021 18:19

baywet enabled auto-merge November 23, 2021 18:20

xuzhg reviewed Nov 23, 2021

View reviewed changes

src/Microsoft.OpenApi.OData.Reader/Common/Constants.cs Show resolved Hide resolved

xuzhg reviewed Nov 23, 2021

View reviewed changes

src/Microsoft.OpenApi.OData.Reader/Edm/ODataDollarCountSegment.cs Show resolved Hide resolved

xuzhg reviewed Nov 23, 2021

View reviewed changes

src/Microsoft.OpenApi.OData.Reader/Edm/ODataPathProvider.cs Show resolved Hide resolved

xuzhg reviewed Nov 23, 2021

View reviewed changes

- applies code review suggestions

a8bb14f

Signed-off-by: Vincent Biret <vibiret@microsoft.com>

baywet requested a review from xuzhg November 23, 2021 18:41

xuzhg reviewed Nov 23, 2021

View reviewed changes

- renames dollar count segment setting

00b605f

Signed-off-by: Vincent Biret <vibiret@microsoft.com>

baywet requested a review from xuzhg November 23, 2021 19:06

baywet mentioned this pull request Nov 23, 2021

Configure repository for static analysis + code linting #142

Closed

xuzhg reviewed Nov 24, 2021

View reviewed changes

xuzhg approved these changes Nov 24, 2021

View reviewed changes

baywet merged commit 2814714 into master Nov 24, 2021

baywet deleted the bugifx/count-segments branch November 24, 2021 01:41

Conversation

baywet commented Nov 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baywet Nov 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xuzhg commented Nov 23, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baywet commented Nov 23, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xuzhg commented Nov 24, 2021

Uh oh!

baywet commented Nov 24, 2021

Uh oh!

baywet commented Nov 22, 2021 •

edited

Loading

baywet Nov 23, 2021 •

edited

Loading