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

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 <[email protected]>

…description Signed-off-by: Vincent Biret <[email protected]>

Signed-off-by: Vincent Biret <[email protected]>

…e unique Signed-off-by: Vincent Biret <[email protected]>

Signed-off-by: Vincent Biret <[email protected]>

…nt path Signed-off-by: Vincent Biret <[email protected]>

Signed-off-by: Vincent Biret <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

- adds launch configuration for update docs tool

d2bb4bd

Signed-off-by: Vincent Biret <[email protected]>

- adds update docs to solution file so debug works properly

70aefe9

Signed-off-by: Vincent Biret <[email protected]>

- code linting

960b31b

Signed-off-by: Vincent Biret <[email protected]>

- code linting

f08be64

Signed-off-by: Vincent Biret <[email protected]>

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

ae11d65

…description Signed-off-by: Vincent Biret <[email protected]>

- code linting

2fee91b

Signed-off-by: Vincent Biret <[email protected]>

- switches to a referenced schema for count operations

76cfc30

Signed-off-by: Vincent Biret <[email protected]>

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 <[email protected]>

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

032fbae

…e unique Signed-off-by: Vincent Biret <[email protected]>

- updates basic open api integration test for $count

f87c2b4

Signed-off-by: Vincent Biret <[email protected]>

- adds a setting to globally disable dollar count path items

4093249

Signed-off-by: Vincent Biret <[email protected]>

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

5d910fd

…nt path Signed-off-by: Vincent Biret <[email protected]>

- updates integration test files

892003e

Signed-off-by: Vincent Biret <[email protected]>

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