OpenAPI: Generalization of naming conventions #1098

maurei · 2021-10-28T11:12:18Z

Closed in favour of #1123

bart-degreed

Done a quick pass over the changes via GitHub. Haven't looked into json-path assertions or the project-level changes yet.

bart-degreed · 2021-10-28T15:24:19Z

test/OpenApiClientTests/ApiException.cs

+                message + "\n\nStatus: " + statusCode + "\nResponse: \n" +
+                (response == null ? "(null)" : response.Substring(0, response.Length >= 512 ? 512 : response.Length)), innerException)


Use string interpolation, this is optimized at compile-time in .NET 6.
Related: why cut off the message beyond the first 512 bytes?

This is a copy paste from the generated code. I don't think we should refactor that

bart-degreed · 2021-10-28T15:25:14Z

test/OpenApiClientTests/NamingConvention/KebabCase/KebabCaseRoundTripTests.cs

+                await dbContext.SaveChangesAsync();
+            });
+
+            KebabCaseClient apiClient = new(_testContext.Factory.CreateClient());


bart-degreed · 2021-10-28T15:26:25Z

test/OpenApiClientTests/NamingConvention/KebabCase/KebabCaseRoundTripTests.cs

+    public sealed class KebabCaseRoundTripTests
+        : IClassFixture<IntegrationTestContext<KebabCaseNamingConventionStartup<NamingConventionDbContext>, NamingConventionDbContext>>
+    {
+        private const string HostPrefix = "http://localhost/";


This should not end in /, so you can reuse it for absolute/relative paths.
See https://github.com/json-api-dotnet/JsonApiDotNetCore/blob/nullable-reference-types/test/JsonApiDotNetCoreTests/IntegrationTests/Links/AbsoluteLinksWithNamespaceTests.cs#L21

bart-degreed · 2021-10-28T15:28:09Z

test/OpenApiClientTests/NamingConvention/KebabCase/KebabCaseRoundTripTests.cs

+            // Assert
+            resourceCollection.Links.First.Should().Be($"{HostPrefix}supermarkets");
+            resourceCollection.Links.Self.Should().Be($"{HostPrefix}supermarkets");
+            resourceCollection.Data.Count.Should().Be(1);


Should().HaveCount()

bart-degreed · 2021-10-28T15:31:21Z

test/OpenApiClientTests/NamingConvention/KebabCase/NamingConventionFakers.cs

+{
+    internal sealed class NamingConventionFakers : FakerContainer
+    {
+        private readonly Lazy<Faker<Supermarket>> _lazySupermarketFaker = new(() => new Faker<Supermarket>().UseSeed(GetFakerSeed())


Copy formatter directive from other fakers and align line breaks likewise

bart-degreed · 2021-10-28T15:48:43Z

test/TestBuildingBlocks/JsonElementAssertions.cs

+        where TAssertions : JsonElementAssertions<TAssertions>
+    {
+        /// <summary>
+        /// - Gets the object which value is being asserted.


whose value, and no leading minus

bart-degreed · 2021-10-28T15:50:03Z

test/TestBuildingBlocks/JsonElementExtensions.cs

+#pragma warning disable AV1210
+                catch
+#pragma warning restore AV1210


Can't you reason about which errors are to be expected here and just catch those?

bart-degreed · 2021-10-28T15:52:24Z

src/JsonApiDotNetCore.OpenApi/JsonApiSchemaIdSelector.cs

+
+        private static string Capitalize(string term)
+        {
+            return string.Concat(term[0].ToString().ToUpper(), term.AsSpan(1));


why use Span<T> here?

bart-degreed · 2021-10-28T15:55:05Z

src/JsonApiDotNetCore.OpenApi/JsonApiSchemaIdSelector.cs

+                // @formatter:keep_existing_linebreaks restore
+                // @formatter:wrap_chained_method_calls restore
+
+                return _namingPolicy.ConvertName(pascalCaseSchemaId);
            }

            // Used for a fixed set of types, such as jsonapi-object, links-in-many-resource-document etc.
            return _formatter.FormatResourceName(type).Singularize();


The only thing that can be expected from FormatResourceName is that applies the casing convention and pluralizes. Given you now have access to the naming policy and don't want plural, why still use this method?

bart-degreed · 2021-10-28T15:55:47Z

src/JsonApiDotNetCore.OpenApi/JsonApiSchemaIdSelector.cs

+                // @formatter:wrap_chained_method_calls chop_always
+                // @formatter:keep_existing_linebreaks true


move these down to where these are intended for

bart-degreed

More feedback

bart-degreed · 2021-10-29T08:56:19Z

test/OpenApiClientTests/OpenApiClientTests.csproj


  <ItemGroup>
    <ProjectReference Include="..\..\src\JsonApiDotNetCore.OpenApi.Client\JsonApiDotNetCore.OpenApi.Client.csproj" />
+    <ProjectReference Include="..\OpenApiTests\OpenApiTests.csproj" />


This dependency can (and should) be removed.

It can't be removed: this project depends on the models, fakers and DbContexts defined in that project.

bart-degreed · 2021-10-29T08:59:00Z

test/OpenApiTests/NamingConvention/KebabCase/KebabCaseNamingConventionStartup.cs

+using Microsoft.EntityFrameworkCore;
+using OpenApiTests.LegacyOpenApiIntegration;
+
+namespace OpenApiTests.NamingConvention.KebabCase


This should be ...NamingConventions... (plural), as there will be multiple.

bart-degreed · 2021-10-29T09:12:50Z

test/OpenApiTests/NamingConvention/KebabCase/KebabCaseTests.cs

+
+                await WriteToSwaggerDocumentsFolderAsync(content);
+
+                return JsonDocument.Parse(content);


This leaks memory. See dotnet/runtime#49207.

bart-degreed · 2021-10-29T09:15:15Z

test/OpenApiTests/NamingConvention/KebabCase/KebabCaseTests.cs

+        public async Task Kebab_naming_policy_is_applied_to_get_collection_endpoint()
+        {
+            // Arrange
+            const string expectedReferenceIdForResponseDocument = "supermarket-collection-response-document";


Most of these constants are used only once, I think its easier to read when they are inlined.

Related to this, I've come up with a different approach for these assertions, which I'll elaborate on later.

bart-degreed · 2021-10-29T09:34:53Z

test/TestBuildingBlocks/JsonElementAssertions.cs

+        public void HaveProperty(string propertyName, string because = "", params object[] becauseArgs)
+        {
+            Execute.Assertion.ForCondition(Subject.TryGetProperty(propertyName, out _)).BecauseOf(because, becauseArgs)
+                .FailWith($"Expected element to have property with name '{propertyName}, but did not find it.");


string escapedJson = Subject.ToString()?.Replace("{", "{{").Replace("}", "}}"); Execute.Assertion.ForCondition(Subject.TryGetProperty(propertyName, out _)) .FailWith($"Expected JSON element '{escapedJson}' to contain a property named '{propertyName}'.");

And you can remove the because/becauseArgs parameters because you're not using them. And should rename this method to ContainsProperty.

bart-degreed · 2021-10-29T11:24:54Z

test/OpenApiTests/OpenApiTests.csproj

+        <PackageReference Include="Microsoft.AspNetCore.Mvc.Testing" Version="$(AspNetCoreVersion)"/>
+        <PackageReference Include="Microsoft.NET.Test.Sdk" Version="$(TestSdkVersion)"/>
+        <PackageReference Include="BlushingPenguin.JsonPath" Version="$(BlushingPenguinVersion)"/>
+        <PackageReference Include="Microsoft.OpenApi.Readers" Version="1.2.3"/>


I think this can be removed.

bart-degreed · 2021-10-29T11:28:19Z

test/TestBuildingBlocks/TestBuildingBlocks.csproj

-    <PackageReference Include="xunit.runner.visualstudio" Version="$(XUnitVersion)" PrivateAssets="All" />
-  </ItemGroup>
+    <ItemGroup>
+        <PackageReference Include="BlushingPenguin.JsonPath" Version="$(BlushingPenguinVersion)"/>


this reference can be removed.

We use the SelectToken method in this project.

maurei · 2021-12-06T14:42:08Z

See #1123

maurei marked this pull request as draft October 28, 2021 11:12

maurei force-pushed the openapi-naming-convention branch from ce76eb2 to 03af5ab Compare October 28, 2021 14:07

bart-degreed suggested changes Oct 28, 2021

View reviewed changes

maurei force-pushed the openapi-naming-convention branch from 03af5ab to 0e6f5a0 Compare October 29, 2021 09:16

bart-degreed suggested changes Oct 29, 2021

View reviewed changes

bart-degreed changed the title ~~Generalization of naming conventions~~ OpenAPI: Generalization of naming conventions Nov 1, 2021

maurei force-pushed the openapi-naming-convention branch 2 times, most recently from 453ab4f to 6b788bb Compare December 6, 2021 14:18

maurei force-pushed the openapi-naming-convention branch from 6b788bb to 68f3d6c Compare December 6, 2021 14:19

closing down PR

3fe4e2e

maurei force-pushed the openapi-naming-convention branch from 68f3d6c to 3fe4e2e Compare December 6, 2021 14:26

maurei closed this Dec 6, 2021

maurei deleted the openapi-naming-convention branch December 6, 2021 14:40

		message + "\n\nStatus: " + statusCode + "\nResponse: \n" +
		(response == null ? "(null)" : response.Substring(0, response.Length >= 512 ? 512 : response.Length)), innerException)

		// @formatter:wrap_chained_method_calls chop_always
		// @formatter:keep_existing_linebreaks true


		await WriteToSwaggerDocumentsFolderAsync(content);

		return JsonDocument.Parse(content);

Uh oh!

OpenAPI: Generalization of naming conventions #1098

OpenAPI: Generalization of naming conventions #1098

Uh oh!

Conversation

maurei commented Oct 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bart-degreed left a comment

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!

Choose a reason for hiding this comment

Uh oh!

bart-degreed left a comment

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!

Choose a reason for hiding this comment

Uh oh!

maurei commented Dec 6, 2021

Uh oh!

Uh oh!

maurei commented Oct 28, 2021 •

edited

Loading