Handle shortened JSON file in `dotnet openapi` #36171

dougbu · 2021-09-04T01:04:19Z

dotnet-openapi refresh does not truncate json documents. #35767
an existing JSON file must be truncated

- #35767 - an existing JSON file must be truncated

dougbu · 2021-09-04T01:05:42Z

@ryanbrandenburg are you available to check this out❔

@mkArtakMSFT please add a reviewer familiar with FileMode differences

dougbu · 2021-09-04T01:08:40Z

@mkArtakMSFT @bradygaster should we backport this to any of our servicing branches or release/6.0❔ If yes, which❔

dougbu · 2021-09-04T01:09:53Z

PR needs a test for the #35767 scenario

- take VS suggestions

- .NET SDKs and Visual Studio's `msbuild` avoid #32686

- take VS suggestions

- include regression test for #35767

dougbu · 2021-09-04T04:21:51Z

src/Tools/Microsoft.dotnet-openapi/src/Commands/BaseCommand.cs

-            CancellationToken cancellationToken = default,
-            int retryCount = 60)
+            int retryCount = 60,
+            CancellationToken cancellationToken = default)


This is a bit odd. The method is only ever called w/ a single argument.

dougbu · 2021-09-04T04:24:53Z

src/Tools/Microsoft.dotnet-openapi/src/Commands/BaseCommand.cs

@@ -572,12 +557,13 @@ private async Task WriteToFileAsync(Stream content, string destinationPath, bool

                // Create or overwrite the destination file.
                reachedCopy = true;
-                using var fileStream = new FileStream(destinationPath, FileMode.OpenOrCreate, FileAccess.Write);
+                using var fileStream = new FileStream(destinationPath, FileMode.Create, FileAccess.Write);


Most important parts of the PR are this and changes in src/Tools/Microsoft.dotnet-openapi/test/OpenApiRefreshTests.cs

dougbu · 2021-09-04T04:25:43Z

src/Tools/Microsoft.dotnet-openapi/test/OpenApiAddFileTests.cs

@@ -16,18 +12,18 @@ public class OpenApiAddFileTests : OpenApiTestBase
    {
        public OpenApiAddFileTests(ITestOutputHelper output) : base(output) { }

-        [Fact(Skip = "https://github.com/dotnet/aspnetcore/issues/32686")]


Will close this issue if tests run smoothly on CI. They're fine in VS locally 😀

Good news…

dougbu · 2021-09-09T00:33:21Z

@mkArtakMSFT could you review or assign a reviewer please❔

@mkArtakMSFT @bradygaster should we backport this to any of our servicing branches or release/6.0❔ If yes, which❔

dougbu · 2021-09-09T00:35:19Z

/fyi @captainsafia this PR definitely and successfully cleans up the last of #32686 😀

dougbu · 2021-09-09T00:37:20Z

src/Tools/Microsoft.dotnet-openapi/test/OpenApiTestBase.cs

-            switch(StatusCode)
+            return StatusCode switch
            {
-                case HttpStatusCode.OK:
-                case HttpStatusCode.Created:
-                case HttpStatusCode.NoContent:
-                case HttpStatusCode.Accepted:
-                    return true;
-                case HttpStatusCode.NotFound:
-                default:
-                    return false;
-            }
+                HttpStatusCode.OK or HttpStatusCode.Created or HttpStatusCode.NoContent or HttpStatusCode.Accepted => true,
+                _ => false,
+            };


While I rarely remember how to write a switch expression, they're certainly easy to read 😺

bradygaster · 2021-09-09T01:01:57Z

@mkArtakMSFT @bradygaster should we backport this to any of our servicing branches or release/6.0❔ If yes, which❔

How far back would be far-enough back to start causing you headaches? I'd definitely support going back to support 3.1, but 5.0, not so sure if there'd be a strong-enough case.

captainsafia · 2021-09-09T01:02:20Z

src/Tools/Microsoft.dotnet-openapi/test/OpenApiRefreshTests.cs

-            var json = await File.ReadAllTextAsync(expectedJsonPath);
-            json += "trash";
-            await File.WriteAllTextAsync(expectedJsonPath, json);
+            await File.WriteAllTextAsync(expectedJsonPath, "trash");


Does this mean it is safe to override the contents of expectedJsonPath instead of appending?

The file is only read to check its SHA in this test. Put another way, the intent is to demonstrate downloading a longer JSON file than the original. New tests demonstrate identical and shorter downloads.

(The dotnet openapi tool does not itself parse OpenAPI files. That comes later, when the changed project is built.)

dougbu · 2021-09-09T01:25:10Z

@mkArtakMSFT @bradygaster should we backport this to any of our servicing branches or release/6.0❔ If yes, which❔

How far back would be far-enough back to start causing you headaches? I'd definitely support going back to support 3.1, but 5.0, not so sure if there'd be a strong-enough case.

The main differences between branches appear to be slight improvements in syntax and test code. I suspect the only trip-ups will be forced returns to older C# syntax. Note: I would cherry-pick the final command rather than blindly take whatever has changed in 'main'.

I also recommend we take this to release/5.0 and release/6.0 if we decide to update release/3.1.

dougbu · 2021-09-09T18:22:06Z

Adding @rafikiassumanimsft and removing @mkArtakMSFT given ownership

davidfowl · 2021-09-09T22:08:07Z

src/Tools/Microsoft.dotnet-openapi/test/OpenApiTestBase.cs

-                default:
-                    return false;
-            }
+                HttpStatusCode.OK or HttpStatusCode.Created or HttpStatusCode.NoContent or HttpStatusCode.Accepted => true,


You get 100 @pranavkm points

dougbu · 2021-09-10T00:13:45Z

Thanks all❕

dougbu · 2021-09-10T00:14:01Z

/backport to release/6.0

github-actions · 2021-09-10T00:14:14Z

Started backporting to release/6.0: https://github.com/dotnet/aspnetcore/actions/runs/1219400869

* backport of #36171 * handle shortened JSON file in `dotnet openapi` - #35767 - an existing JSON file must be truncated * Extend `dotnet openapi refresh` tests - include regression test for #35767

Handle shortened JSON file in dotnet openapi

9a5bf6a

- #35767 - an existing JSON file must be truncated

dougbu added area-commandlinetools Includes: Command line tools, dotnet-dev-certs, dotnet-user-jwts, and OpenAPI feature-openapi labels Sep 4, 2021

dougbu requested a review from mkArtakMSFT September 4, 2021 01:04

dougbu requested a review from Pilchie as a code owner September 4, 2021 01:04

dougbu added 4 commits September 3, 2021 21:17

!fixup! Address nits in changed file

47bf0fa

- take VS suggestions

Stop skipping dotnet openapi

3efb927

- .NET SDKs and Visual Studio's `msbuild` avoid #32686

!fixup! Address nits in dotnet openapi test files

bac9c02

- take VS suggestions

Extend dotnet openapi refresh tests

33e855b

- include regression test for #35767

dougbu commented Sep 4, 2021

View reviewed changes

dougbu mentioned this pull request Sep 4, 2021

Resolve issues with dotnet-openapi tests in latest SDK #32686

Closed

dougbu enabled auto-merge (squash) September 4, 2021 06:01

dougbu commented Sep 9, 2021

View reviewed changes

captainsafia reviewed Sep 9, 2021

View reviewed changes

dougbu requested review from rafikiassumani-msft and removed request for mkArtakMSFT September 9, 2021 18:21

rafikiassumani-msft requested a review from halter73 September 9, 2021 21:34

davidfowl reviewed Sep 9, 2021

View reviewed changes

pranavkm approved these changes Sep 10, 2021

View reviewed changes

dougbu merged commit e23fd04 into main Sep 10, 2021

dougbu deleted the dougbu/json.the.shorter.35767 branch September 10, 2021 00:11

ghost added this to the 7.0-preview1 milestone Sep 10, 2021

github-actions bot mentioned this pull request Sep 10, 2021

[release/6.0] Handle shortened JSON file in dotnet openapi #36355

Merged

dougbu mentioned this pull request Sep 10, 2021

dotnet-openapi refresh does not truncate json documents. #35767

Closed

Handle shortened JSON file in dotnet openapi #36171

Handle shortened JSON file in dotnet openapi #36171

Uh oh!

Conversation

dougbu commented Sep 4, 2021

Uh oh!

dougbu commented Sep 4, 2021

Uh oh!

dougbu commented Sep 4, 2021

Uh oh!

dougbu commented Sep 4, 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!

dougbu commented Sep 9, 2021

Uh oh!

dougbu commented Sep 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bradygaster commented Sep 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougbu Sep 9, 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!

dougbu commented Sep 9, 2021

Uh oh!

dougbu commented Sep 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougbu commented Sep 10, 2021

Uh oh!

dougbu commented Sep 10, 2021

Uh oh!

github-actions bot commented Sep 10, 2021

Uh oh!

Uh oh!

Handle shortened JSON file in `dotnet openapi` #36171

Handle shortened JSON file in `dotnet openapi` #36171

dougbu Sep 9, 2021 •

edited

Loading