Skip to content
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.

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

openapi: support validation attributes #1477

Draft
wants to merge 9 commits into
base: openapi
Choose a base branch
from

Conversation

verdie-g
Copy link
Sponsor Contributor

@verdie-g verdie-g commented Feb 24, 2024

Closes #1055

.NET Applicable types OpenAPI NSwag Kiota MVC EF Core Notes
Guid uuid ? ? ? ? ?
[Length] string, ICollection N ? ? Y ? Added in .NET 8
[Compare] - N ? ? Y ? Works on any type that implements Equals
[Required(AllowEmptyStrings = true)] string required: [] ? ? Y ? Only applies to JSON:API create resource
[RegularExpression] string format: pattern Y ? ? ? Potential differences in regex dialects, don't care
[StringLength] string minLength / maxLength Y ? ? ? Omitting MinimumLength implies 0
[CreditCard] or [DataType(DataType.CreditCard)] string format: credit-card Y ? ? ?
[EmailAddress] or [DataType(DataType.EmailAddress)] string format: email Y ? ? ?
[Base64String] string ? ? ? ? ? Added in .NET 8
[Phone] or [DataType(DataType.PhoneNumber)] string format: tel Y ? ? ?
[Range] int, double, IComparable minimum, maximum (both inclusive) Y ? ? ? .NET 8 adds MinimumIsExclusive, MaximumIsExclusive
Uri ? ? ? ? ? ?
[Url] string format: uri Y ? ? ? Only http/https/ftp, non-relative
[AllowedValues] any ? ? ? ? ? Added in .NET 8
[DeniedValues] any ? ? ? ? ? Added in .NET 8
[Required] any required: [], minLength: 1 Y ? ? ? Only applies to JSON:API create resource
[Remote] ? ? Ignored by Swashbuckle
[DataType(DataType.Currency)] ? format: currency ? ? ? ?
[DataType(DataType.Date)] ? format: date ? ? ? ?
[DataType(DataType.DateTime)] ? format: date-time ? ? ? ?
[DataType(DataType.Duration)] ? format: duration ? ? ? ?
[DataType(DataType.Html)] ? format: html ? ? ? ?
[DataType(DataType.ImageUrl)] ? format: uri ? ? ? ?
[DataType(DataType.MultilineText)] ? format: multiline ? ? ? ?
[DataType(DataType.Password)] ? format: password ? ? ? ?
[DataType(DataType.PostalCode)] ? format: postal-code ? ? ? ?
[DataType(DataType.Time)] ? format: time ? ? ? ?
[DataType(DataType.Upload)] ? format: binary ? ? ? ?
[DataType(DataType.Url)] ? format: uri ? ? ? ?
- multipleOf
- hostname
- minItems, maxItems
HashSet uniqueItems Y ? ? Y N
- ipv4, ipv6
get; readOnly ? ? ? ? ?
set; writeOnly ? ? ? ? ?
[ValidateNever] ? ? Ignored by Swashbuckle

QUALITY CHECKLIST

tagsEl.Should().ContainPath("items").With(itemsEl =>
{
itemsEl.Should().HaveProperty("type", "string");
// TODO: no length constraint?
Copy link
Sponsor Contributor Author

@verdie-g verdie-g Feb 24, 2024

Choose a reason for hiding this comment

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

It seems like the library used to generate the OpenAPI document doesn't support Length and Base64String which were both introduced in .NET 8.

Also, even if they were supported, I'm not sure would be be the behavior in the OpenApiClientTests because both .NET 6 and .NET 8 OpenApiTests will generate the swagger file.

Copy link
Member

@bkoelman bkoelman Feb 26, 2024

Choose a reason for hiding this comment

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

Swashbuckle's type mappings are defined here. ModelState attributes are mapped here.

I suppose we could fill the gap ourselves, as Swashbuckle isn't maintained anymore. Or just postpone adding support for the .NET 8 annotations. Either way, [StringLength(10)] can still be used on both frameworks.

It all depends on what .NET 8 adds and how useful that is, how much effort it takes to implement, and how messy the codebase becomes when doing so. If it's just Length and Base64String, I don't think it's worth the effort right now.

We could adapt the write logic to emit a swagger.g.json per target framework (not saying we should, but something to consider).

Copy link
Member

Choose a reason for hiding this comment

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

It's also unclear at this point if our OpenAPI support is going to support .NET 6, which EOLs in less than a year.

Copy link
Member

Choose a reason for hiding this comment

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

The good news is that Swashbuckle got revived and they take PRs again. New .NET 8 support was added recently. I think it is worth testing the new .NET 8 support, which requires making the SwaggerDocumentOutputDirectory conditional on the TFM.

I've made a breakdown of the validation support to consider (based on here, here, here and here), see below. Can you verify and fill in the blanks? Something along these lines should be added to the PR description, which enables me to validate the implementation against the intended behavior. For example, I don't know if you intentionally left out Guid or just forgot about it. I'm not saying that everything in the table must be implemented, but the considerations and tradeoffs should be made explicit. I also came to realize there's a fine line between validation and data types. Ideally, #1051 should have been done before this, so perhaps the [DataType] entries are better left out of scope. It depends on whether validation logic is involved. The server-side validation tests in JADNC are limited, so it would be interesting to know how ASP.NET ModelState validation and EF Core handle these attributes, identifying potential conflicts.

.NET Applicable types OpenAPI NSwag Kiota MVC EF Core Notes
[ValidateNever] ? ? Ignored by Swashbuckle
[CreditCard] or [DataType(DataType.CreditCard)] string format: credit-card Y ? ? ?
[Compare] ? ? Ignored by Swashbuckle
[EmailAddress] or [DataType(DataType.EmailAddress)] string format: email Y ? ? ?
[Phone] or [DataType(DataType.PhoneNumber)] string format: tel Y ? ? ?
[Range] int, double, IComparable minimum, maximum (both inclusive) Y ? ? ? .NET 8 adds MinimumIsExclusive, MaximumIsExclusive
[RegularExpression] string format: pattern Y ? ? ? Potential differences in regex dialects, don't care
[Required] any required: [], minLength: 1 Y ? ? ? Only applies to JSON:API create resource
[Required(AllowEmptyStrings = true)] any required: [] ? ? ? ? Only applies to JSON:API create resource
[StringLength] string minLength / maxLength Y ? ? ? Omitting MinimumLength implies 0
[Url] string format: uri Y ? ? ? Only http/https/ftp, non-relative
[Remote] ? ? Ignored by Swashbuckle
[Length] string, ICollection ? ? ? ? ? Added in .NET 8
[Base64String] byte? ? ? ? ? ? Added in .NET 8
[AllowedValues] any ? ? ? ? ? Added in .NET 8
[DeniedValues] any ? ? ? ? ? Added in .NET 8
[DataType(DataType.Currency)] ? format: currency ? ? ? ?
[DataType(DataType.Date)] ? format: date ? ? ? ?
[DataType(DataType.DateTime)] ? format: date-time ? ? ? ?
[DataType(DataType.Duration)] ? format: duration ? ? ? ?
[DataType(DataType.Html)] ? format: html ? ? ? ?
[DataType(DataType.ImageUrl)] ? format: uri ? ? ? ?
[DataType(DataType.MultilineText)] ? format: multiline ? ? ? ?
[DataType(DataType.Password)] ? format: password ? ? ? ?
[DataType(DataType.PostalCode)] ? format: postal-code ? ? ? ?
[DataType(DataType.Time)] ? format: time ? ? ? ?
[DataType(DataType.Upload)] ? format: binary ? ? ? ?
[DataType(DataType.Url)] ? format: uri ? ? ? ?
Guid uuid ? ? ? ? ?
- multipleOf
- hostname
- minItems, maxItems
- uniqueItems
- ipv4, ipv6
get; readOnly ? ? ? ? ?
set; writeOnly ? ? ? ? ?

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

I've added the table to the PR description and will complete it later.

Copy link
Member

Choose a reason for hiding this comment

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

I've re-pasted the table in the PR description from the above comment's source to preserve formatting. I noticed the DataType entries were removed, was that intentional? They do affect the openapi.json output when used.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

I noticed the DataType entries were removed, was that intentional?

I did remove them because of your comment

so perhaps the [DataType] entries are better left out of scope

I'm not sure how to fill that table. What does "yes" means for NSwag, MVC, and EF Core?

Copy link
Member

@bkoelman bkoelman May 30, 2024

Choose a reason for hiding this comment

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

I noticed the DataType entries were removed, was that intentional?

I did remove them because of your comment

The point of the table is to document the outcome of experiments and decisions. For example, if we decide that [DataType(DataType.Html)] is out of scope because it doesn't affect validation, the notes column should indicate so. This makes it possible to distinguish between "there's intentionally no test coverage" vs "we simply forgot it".

I'm not sure how to fill that table. What does "yes" means for NSwag, MVC, and EF Core?

That it behaves the way JADNC users would expect, without oddities/workarounds. It indicates if we need to warn users in the docs, log warnings, etc.

  • NSwag/Kiota means a working C# client is generated (with test coverage for valid/invalid input, part of this PR).
  • MVC means the expected server-side validation error is produced. Sometimes NSwag already catches bad input, so it never hits the server. We don't need to add tests for that in this PR, but it should be tried out. For example, if adding [StringLength] does NOT validate server-side, but only NSwag catches it, we should document that. Because only client-side validation is insecure.
  • EF Core means checking what SQL data type and SQL constraints are being generated, and whether they make sense. For example, does [Required(AllowEmptyStrings = false)] generate just a non-nullable column, or does it also add the minimum length constraint? If not, should it do so, if possible at all? Google for it, are there open issues, StackOverflow questions, etc? We need to understand how it works and is supposed to be used, and reason about the feature from an API developer perspective. Is there anything to watch out for? Similar to the previous bullet, this is exploration.

Copy link
Member

Choose a reason for hiding this comment

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

According to https://learn.microsoft.com/en-us/dotnet/api/system.componentmodel.dataannotations.validationattribute?view=net-8.0, there's also [MinLength] and [MaxLength]. They should be included in the table.

@verdie-g
Copy link
Sponsor Contributor Author

verdie-g commented Feb 24, 2024

@bkoelman could you give a preliminary review on just that?

@bkoelman
Copy link
Member

@verdie-g Do you still intend to complete this?

@verdie-g
Copy link
Sponsor Contributor Author

Unfortunately I don't think I'll find the time.

@verdie-g
Copy link
Sponsor Contributor Author

I pushed some old changes that were still on my machine in case someone wants to continue the PR.

@verdie-g verdie-g force-pushed the 1055-openapi-model-validation branch from 3f781fb to 9dee70f Compare May 27, 2024 01:52
@verdie-g verdie-g force-pushed the 1055-openapi-model-validation branch from f46fc65 to 07c9d7d Compare May 27, 2024 21:22
BackgroundPicture = new Uri(fingerprint.BackgroundPicture!),
NextRevalidation = "02:00:00",
ValidatedAt = fingerprint.ValidatedAt!.Value.ToUniversalTime(),
// TODO: ValidatedDateAt = new DateTimeOffset(fingerprint.ValidatedDateAt!.Value.ToDateTime(new TimeOnly()).ToUniversalTime()),
Copy link
Sponsor Contributor Author

@verdie-g verdie-g May 27, 2024

Choose a reason for hiding this comment

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

If uncommented, fails with

 "JsonApiDotNetCore.Repositories.DataStoreUpdateException: Failed to persist changes in the underlying data store.",
          " ---> Microsoft.EntityFrameworkCore.DbUpdateException: An error occurred while saving the entity changes. See the inner exception for details.",
          " ---> System.ArgumentException: Cannot write DateTime with Kind=Local to PostgreSQL type 'timestamp with time zone', only UTC is supported. Note that it's not possible to mix DateTimes with different Kinds in an array, range, or multirange. (Parameter 'value')",
          "   at PgConverterResolution? Npgsql.Internal.Converters.DateTimeConverterResolver<T>.Get(DateTime value, PgTypeId? expectedPgTypeId, bool validateOnly)",
          "   at DateTimeConverterResolver<DateTime> Npgsql.Internal.Converters.DateTimeConverterResolver.CreateResolver(PgSerializerOptions options, PgTypeId timestampTz, PgTypeId timestamp, bool dateTimeInfinityConversions)+(DateTimeConverterResolver<DateTime> resolver, DateTime value, PgTypeId? expectedPgTypeId) => { }",
          "   at PgConverterResolution? Npgsql.Internal.Converters.DateTimeConverterResolver<T>.Get(T value, PgTypeId? expectedPgTypeId)",
          "   at PgConverterResolution? Npgsql.Internal.PgConverterResolver<T>.GetAsObjectInternal(PgTypeInfo typeInfo, object value, PgTypeId? expectedPgTypeId)",
          "   at PgConverterResolution? Npgsql.Internal.PgResolverTypeInfo.GetResolutionAsObject(object value, PgTypeId? expectedPgTypeId)",
          "   at PgConverterResolution Npgsql.Internal.PgTypeInfo.GetObjectResolution(object value)",
          "   at PgConverterResolution Npgsql.NpgsqlParameter.ResolveConverter(PgTypeInfo typeInfo)",
          "   at void Npgsql.NpgsqlParameter.ResolveTypeInfo(PgSerializerOptions options)",
          "   at void Npgsql.NpgsqlParameterCollection.ProcessParameters(PgSerializerOptions options, bool validateValues, CommandType commandType)",
          "   at async ValueTask<NpgsqlDataReader> Npgsql.NpgsqlCommand.ExecuteReader(bool async, CommandBehavior behavior, CancellationToken cancellationToken) x 2",
          "   at async Task<DbDataReader> Npgsql.NpgsqlCommand.ExecuteDbDataReaderAsync(CommandBehavior behavior, CancellationToken cancellationToken)",
          "   at async Task<RelationalDataReader> Microsoft.EntityFrameworkCore.Storage.RelationalCommand.ExecuteReaderAsync(RelationalCommandParameterObject parameterObject, CancellationToken cancellationToken) x 2",
          "   at async Task Microsoft.EntityFrameworkCore.Update.ReaderModificationCommandBatch.ExecuteAsync(IRelationalConnection connection, CancellationToken cancellationToken)",
          "   --- End of inner exception stack trace ---",
          "   at async Task Microsoft.EntityFrameworkCore.Update.ReaderModificationCommandBatch.ExecuteAsync(IRelationalConnection connection, CancellationToken cancellationToken)",
          "   at async Task<int> Microsoft.EntityFrameworkCore.Update.Internal.BatchExecutor.ExecuteAsync(IEnumerable<ModificationCommandBatch> commandBatches, IRelationalConnection connection, CancellationToken cancellationToken) x 3",
          "   at async Task<int> Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.SaveChangesAsync(IList<IUpdateEntry> entriesToSave, CancellationToken cancellationToken) x 2",
          "   at async Task<TResult> Npgsql.EntityFrameworkCore.PostgreSQL.Storage.Internal.NpgsqlExecutionStrategy.ExecuteAsync<TState, TResult>(TState state, Func<DbContext, TState, CancellationToken, Task<TResult>> operation, Func<DbContext, TState, CancellationToken, Task<ExecutionResult<TResult>>> verifySucceeded, CancellationToken cancellationToken)",
          "   at async Task<int> Microsoft.EntityFrameworkCore.DbContext.SaveChangesAsync(bool acceptAllChangesOnSuccess, CancellationToken cancellationToken) x 2",
          "   at async Task JsonApiDotNetCore.Repositories.EntityFrameworkCoreRepository<TResource, TId>.SaveChangesAsync(CancellationToken cancellationToken) in JsonApiDotNetCore/src/JsonApiDotNetCore/Repositories/EntityFrameworkCoreRepository.cs:line 651",
          "   --- End of inner exception stack trace ---",
          "   at async Task JsonApiDotNetCore.Repositories.EntityFrameworkCoreRepository<TResource, TId>.SaveChangesAsync(CancellationToken cancellationToken) in JsonApiDotNetCore/src/JsonApiDotNetCore/Repositories/EntityFrameworkCoreRepository.cs:line 657",
          "   at async Task JsonApiDotNetCore.Repositories.EntityFrameworkCoreRepository<TResource, TId>.CreateAsync(TResource resourceFromRequest, TResource resourceForDatabase, CancellationToken cancellationToken) in JsonApiDotNetCore/src/JsonApiDotNetCore/Repositories/EntityFrameworkCoreRepository.cs:line 232",
          "   at void CallSite.Target(Closure, CallSite, object)",
          "   at void System.Dynamic.UpdateDelegates.UpdateAndExecuteVoid1<T0>(CallSite site, T0 arg0)",
          "   at async Task JsonApiDotNetCore.Repositories.ResourceRepositoryAccessor.CreateAsync<TResource>(TResource resourceFromRequest, TResource resourceForDatabase, CancellationToken cancellationToken)",
          "   at async Task<TResource> JsonApiDotNetCore.Services.JsonApiResourceService<TResource, TId>.CreateAsync(TResource resource, CancellationToken cancellationToken) in JsonApiDotNetCore/src/JsonApiDotNetCore/Services/JsonApiResourceService.cs:line 217",
          "   at async Task<TResource> JsonApiDotNetCore.Services.JsonApiResourceService<TResource, TId>.CreateAsync(TResource resource, CancellationToken cancellationToken) in JsonApiDotNetCore/src/JsonApiDotNetCore/Services/JsonApiResourceService.cs:line 223",
          "   at async Task<IActionResult> JsonApiDotNetCore.Controllers.BaseJsonApiController<TResource, TId>.PostAsync(TResource resource, CancellationToken cancellationToken) in JsonApiDotNetCore/src/JsonApiDotNetCore/Controllers/BaseJsonApiController.cs:line 207",
          "   at async Task<IActionResult> JsonApiDotNetCore.Controllers.JsonApiController<TResource, TId>.PostAsync(TResource resource, CancellationToken cancellationToken) in JsonApiDotNetCore/src/JsonApiDotNetCore/Controllers/JsonApiController.cs:line 78",
          "   at async ValueTask<IActionResult> Microsoft.AspNetCore.Mvc.Infrastructure.ActionMethodExecutor+TaskOfIActionResultExecutor.Execute(ActionContext actionContext, IActionResultTypeMapper mapper, ObjectMethodExecutor executor, object controller, object[] arguments)",
          "   at async Task Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeActionMethodAsync()+Logged(?)",
          "   at async Task Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeNextActionFilterAsync()+Awaited(?)",
          "   at void Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Rethrow(ActionExecutedContextSealed context)",
          "   at Task Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Next(ref State next, ref Scope scope, ref object state, ref bool isCompleted)",
          "   at Task Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeInnerFilterAsync()",
          "   at async Task Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeNextExceptionFilterAsync()+Awaited(?)"

It seems like the DateOnly is converted to a DateTime at some point. Any idea?

Copy link
Member

Choose a reason for hiding this comment

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

Fakers generally contain .ToUniversalTime(), so PostgreSQL won't block non-UTC values. Though it's odd you're getting this error on a DateOnly. You can check the generated SQL by running a server-side test in debug mode, which writes to the Output window. Perhaps this is a bug in the PostgreSQL EF Core provider?

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

That's cool

Failed executing DbCommand (34ms) [Parameters=[@p0='43debb5f-abe4-4e44-bad5-b852124e8dd4', @p1='95' (Nullable = true), @p2='https://loremflickr.com/320/240?lock=1798481699', @p3='6759-7554-5043-3270', @p4='[email protected]', @p5='Kellie', @p6='Wilderman' (Nullable = false), @p7='02:00:00' (Nullable = true) (DbType = Object), @p8='(546) 274-9304 x65491', @p9='https://loremflickr.com/320/240?lock=475250130', @p10='ogxiroilgkft', @p11='2019-12-30T20:45:27.0196183-05:00' (Nullable = true) (DbType = DateTime), @p12='12/31/2019' (Nullable = true) (DbType = Date), @p13='19:31' (Nullable = true) (DbType = Time)], CommandType='Text', CommandTimeout='30']
      INSERT INTO "Fingerprints" ("Id", "Age", "BackgroundPicture", "CreditCard", "Email", "FirstName", "LastName", "NextRevalidation", "Phone", "ProfilePicture", "UserName", "ValidatedAt", "ValidatedDateAt", "ValidatedTimeAt")
      VALUES (@p0, @p1, @p2, @p3, @p4, @p5, @p6, @p7, @p8, @p9, @p10, @p11, @p12, @p13);

Looks fine

@p12='12/31/2019' (Nullable = true) (DbType = Date)

I'll dig more

@verdie-g
Copy link
Sponsor Contributor Author

Hi @bkoelman, I'll try finishing that PR. Could you have a pass on its current state, I'll add tests for Kiota once the one for NSwag start looking okay. Also, is there anything worth adding to the doc regarding this change?

@bkoelman
Copy link
Member

Hi @bkoelman, I'll try finishing that PR. Could you have a pass on its current state, I'll add tests for Kiota once the one for NSwag start looking okay. Also, is there anything worth adding to the doc regarding this change?

Cool! Sure, I'll take a look. I don't expect the docs need to be updated, unless there are pitfalls users should know about. For kiota, it's worth removing the --log-level Error switch temporarily to see what comes up. However, kiota is pretty rigid and inflexible, so don't worry too much about its quirks.

test/OpenApiTests/ModelValidation/Fingerprint.cs Outdated Show resolved Hide resolved
tagsEl.Should().ContainPath("items").With(itemsEl =>
{
itemsEl.Should().HaveProperty("type", "string");
// TODO: no length constraint?
Copy link
Member

Choose a reason for hiding this comment

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

The good news is that Swashbuckle got revived and they take PRs again. New .NET 8 support was added recently. I think it is worth testing the new .NET 8 support, which requires making the SwaggerDocumentOutputDirectory conditional on the TFM.

I've made a breakdown of the validation support to consider (based on here, here, here and here), see below. Can you verify and fill in the blanks? Something along these lines should be added to the PR description, which enables me to validate the implementation against the intended behavior. For example, I don't know if you intentionally left out Guid or just forgot about it. I'm not saying that everything in the table must be implemented, but the considerations and tradeoffs should be made explicit. I also came to realize there's a fine line between validation and data types. Ideally, #1051 should have been done before this, so perhaps the [DataType] entries are better left out of scope. It depends on whether validation logic is involved. The server-side validation tests in JADNC are limited, so it would be interesting to know how ASP.NET ModelState validation and EF Core handle these attributes, identifying potential conflicts.

.NET Applicable types OpenAPI NSwag Kiota MVC EF Core Notes
[ValidateNever] ? ? Ignored by Swashbuckle
[CreditCard] or [DataType(DataType.CreditCard)] string format: credit-card Y ? ? ?
[Compare] ? ? Ignored by Swashbuckle
[EmailAddress] or [DataType(DataType.EmailAddress)] string format: email Y ? ? ?
[Phone] or [DataType(DataType.PhoneNumber)] string format: tel Y ? ? ?
[Range] int, double, IComparable minimum, maximum (both inclusive) Y ? ? ? .NET 8 adds MinimumIsExclusive, MaximumIsExclusive
[RegularExpression] string format: pattern Y ? ? ? Potential differences in regex dialects, don't care
[Required] any required: [], minLength: 1 Y ? ? ? Only applies to JSON:API create resource
[Required(AllowEmptyStrings = true)] any required: [] ? ? ? ? Only applies to JSON:API create resource
[StringLength] string minLength / maxLength Y ? ? ? Omitting MinimumLength implies 0
[Url] string format: uri Y ? ? ? Only http/https/ftp, non-relative
[Remote] ? ? Ignored by Swashbuckle
[Length] string, ICollection ? ? ? ? ? Added in .NET 8
[Base64String] byte? ? ? ? ? ? Added in .NET 8
[AllowedValues] any ? ? ? ? ? Added in .NET 8
[DeniedValues] any ? ? ? ? ? Added in .NET 8
[DataType(DataType.Currency)] ? format: currency ? ? ? ?
[DataType(DataType.Date)] ? format: date ? ? ? ?
[DataType(DataType.DateTime)] ? format: date-time ? ? ? ?
[DataType(DataType.Duration)] ? format: duration ? ? ? ?
[DataType(DataType.Html)] ? format: html ? ? ? ?
[DataType(DataType.ImageUrl)] ? format: uri ? ? ? ?
[DataType(DataType.MultilineText)] ? format: multiline ? ? ? ?
[DataType(DataType.Password)] ? format: password ? ? ? ?
[DataType(DataType.PostalCode)] ? format: postal-code ? ? ? ?
[DataType(DataType.Time)] ? format: time ? ? ? ?
[DataType(DataType.Upload)] ? format: binary ? ? ? ?
[DataType(DataType.Url)] ? format: uri ? ? ? ?
Guid uuid ? ? ? ? ?
- multipleOf
- hostname
- minItems, maxItems
- uniqueItems
- ipv4, ipv6
get; readOnly ? ? ? ? ?
set; writeOnly ? ? ? ? ?

Copy link
Member

@bkoelman bkoelman left a comment

Choose a reason for hiding this comment

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

I've added some comments on things that jumped out to me. Before focussing on kiota, we should get the proposed scope table in better shape.

@verdie-g verdie-g requested a review from bkoelman May 30, 2024 02:05
Copy link
Member

@bkoelman bkoelman left a comment

Choose a reason for hiding this comment

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

See comments. The test coverage needs adjustment, based on the table (that isn't finished yet). I would expect to see some net80-only tests too.

ModelStateValidationClient apiClient = new(httpClient);

// Act
Func<Task<SocialMediaAccountPrimaryResponseDocument>> action = () => apiClient.PostSocialMediaAccountAsync(new SocialMediaAccountPostRequestDocument
Copy link
Member

Choose a reason for hiding this comment

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

Please extract the request body in a separate variable named requestBody, as existing tests do. It makes the code a bit more readable.

{
Attributes = new SocialMediaAccountAttributesInPostRequest
{
LastName = "",
Copy link
Member

Choose a reason for hiding this comment

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

Please use the faker for LastName to express that it contains something valid.

}

[Fact]
public async Task Cannot_use_invalid_credit_card()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public async Task Cannot_use_invalid_credit_card()
public async Task Cannot_use_invalid_credit_card_number()

}

[Fact]
public async Task Cannot_use_invalid_email()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public async Task Cannot_use_invalid_email()
public async Task Cannot_use_invalid_email_address()

}

[Fact]
public async Task Cannot_use_invalid_url()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public async Task Cannot_use_invalid_url()
public async Task Cannot_use_relative_url()

Comment on lines +18 to +22
#if NET8_0_OR_GREATER
"net8.0";
#else
"net6.0";
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Please use #NET6_0 instead of NET8_0_OR_GREATER. This makes it easier to cleanup when .NET 6 support will be dropped in the future.

JsonElement document = await _testContext.GetSwaggerDocumentAsync();

// Assert
document.Should().ContainPath($"components.schemas.{modelName}.properties.tags").With(tagsEl =>
Copy link
Member

Choose a reason for hiding this comment

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

Please rename tagsEl to tagsElement, as well as similar variables in this file.

}

public static TheoryData<string> ModelNames =>
// ReSharper disable once UseCollectionExpression
Copy link
Member

Choose a reason for hiding this comment

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

This looks like a Resharper bug. Is there an open issue at https://youtrack.jetbrains.com/issues/RSRP we can link to? If not, can you create one and link to it from here?

public string? Planet { get; set; }

[Attr]
[Range(typeof(TimeSpan), "01:00", "05:00")]
Copy link
Member

Choose a reason for hiding this comment

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

Can you check if TimeSpan is culture-sensitive? If so, should add ConvertValueInInvariantCulture = true, ParseLimitsInInvariantCulture = true.

});
}

public static TheoryData<string> ModelNames =>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public static TheoryData<string> ModelNames =>
public static readonly TheoryData<string> ModelNames =

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants