Inconsistent exceptions thrown when OpenAPI has badly formatted default values

[idg10]

The following scenario outline describes the behaviour when an OpenAPI specification defines default values for parameters, but where those defaults are invalid:

Menes/Solutions/Menes.Specs/Features/OpenApiDefaultParameterParsing.feature

Lines 85 to 94 in 6b2b63d

	Scenario Outline: Incorrect parameter values
	Given I have constructed the OpenAPI specification with a <ParameterLocation> parameter with name <ParameterName>, type <Type>, format <Format> and default value <DefaultValue>
	When I try to parse the default value and expect an error
	Then an '<ExceptionType>' should be thrown
	Examples:
	| ParameterLocation | ParameterName | Type | Format | DefaultValue | ExceptionType |
	| query | openApiDate | string | date | This is certainly not a date | FormatException |
	| header | openApiDateTime | string | date-time | 20170721T173228Z | OpenApiBadRequestException |
	| cookie | openApiLong | integer | int64 | 9223372036854775808123123123 | OpenApiSpecificationException |

The outline defines three examples. The first seems clear enough: a default of This is certainly not a date has been supplied for a string with format date. The next is more subtle: a string with format date-time has been given a default value of 20170721T173228Z which certainly looks like it might be a valid, but it turns out not to be. The set of acceptable formats is given at https://github.com/menes-dotnet/Menes/blob/main/Solutions/Menes.Abstractions/Menes/Validation/OpenApiSchemaValidator.cs#L313-L330 and of the formats that include both date and time, all require dashes between the date components and colons between the time components. So 2017-07-21T17:32:28 would be acceptable but 20170721T173228Z is not.

The third is also not entirely obvious. It's an integer type and the value 9223372036854775808123123123 looks like an integer. However, the format is int64, and that particular value is too large to fit into an int64, so it is invalid.

The baffling part of this spec is that it requires different exception types for each example. The spec executes successfully, but it's not clear whether it should.

All three of these are instances of the same mistake: a particular kind of error in the OpenAPI specification. (In all three cases, the spec generates an OpenAPI spec that defines a parameter with a default value that is valid according to its type but invalid according to its format.)

I don't see a good reason why the correct exception should not be OpenApiSpecificationException in all three cases. As far as I can tell, the specification just describes inconsistent behaviour that is an accident of the implementation details of how these three variations on the mistake happen to be detected.

The reason we see a FormatException in the first case is that we happen to be relying on Newtonsoft.Json to do the conversion. That's happening (in a not entirely obvious way) in this line of code:

Menes/Solutions/Menes.Abstractions/Menes/Converters/DateConverter.cs

Line 42 in 6b2b63d

var result = (DateTimeOffset)token;

If the text isn't in one of the formats that Newtonsoft.Json recognizes for a DateTimeOffset, that line throws a FormatException. The first example in the scenario outline describes this fact correctly, but I'm unconvinced that this is desirable for that to be the exception that ultimately emerges. (The net effect will likely be a 500 server error, which is the correct externally visible behaviour as far as clients of the service are concerned—this scenario is indeed a server misconfiguration. But it's not a particularly helpful error for the owner of the service when diagnosing the problem.)

The second example looks actively wrong: an OpenApiBadRequestException is thrown even if nothing is wrong with the request. The reason this happens is that the following sequence occurs:

1. The test creates a request that does not include the parameter (which is fine because the parameter is optional—the whole point here is that a default value has been specified)
2. The HttpRequestParameterBuilder tries to discover the default for this parameter:

   Menes/Solutions/Menes.Hosting.AspNetCore/Menes/Internal/HttpRequestParameterBuilder.cs

   Line 190 in 6b2b63d

   private object? TryGetDefaultValueFromSchema(OpenApiSchema schema, IOpenApiAny? defaultValue)

3. Since the parameter type is string, the switch expression at the top of that method calls this.ConvertValue which correctly finds the Menes StringConverter, and calls its ConvertFrom method here:

   Menes/Solutions/Menes.Hosting.AspNetCore/Menes/Internal/HttpRequestParameterBuilder.cs

   Line 668 in 6b2b63d

   return converter.ConvertFrom(value, schema);

4. The StringConverter calls validator.ValidateAndThrow:

   Menes/Solutions/Menes.Abstractions/Menes/Converters/StringConverter.cs

   Line 35 in 6b2b63d

   this.validator.ValidateAndThrow(content, schema);

5. The validator detects that the format is not an acceptable date-time here:

   Menes/Solutions/Menes.Abstractions/Menes/Validation/OpenApiSchemaValidator.cs

   Line 331 in 6b2b63d

   if (token.Type != JTokenType.Date && !DateTimeOffset.TryParseExact(value, acceptableFormats, CultureInfo.InvariantCulture, DateTimeStyles.None, out _))

6. The ValidateAndThrow method reports this validation failure by throwing an OpenApiBadRequestException here:

   Menes/Solutions/Menes.Abstractions/Menes/Validation/OpenApiSchemaValidator.cs

   Lines 83 to 89 in 6b2b63d

   	if (errors.Count > 0)
   	{
   	Exception exception = new OpenApiBadRequestException("Schema Validation Error", "The content does not conform to the required schema.")
   	.AddProblemDetailsExtension("validationErrors", errors);
   	throw exception;
   	}

The basic problem seems to be that OpenApiSchemaValidator.ValidateAndThrow presumes it is being asked to validate data that came from a request (a presumption expressed in the fact that it throws an OpenApiBadRequestException) but there are scenarios in which this method is being asked to validate data that did not come from a request.

The reason the third example in this scenario outline does actually produce what seems like the correct exception is that the problem here is detected in an entirely different way, but possibly by accident. It turns out that if you supply Microsoft.OpenApi with a default value that is an integer numeric value that's perfectly valid but too large to fit in a 64-bit int (e.g. "default": 9223372036854775808123123123) it reports it as an OpenApiString! (The OpenApiSchema object will still correctly report that the type was an int64.)

This is detected by this code:

Menes/Solutions/Menes.Hosting.AspNetCore/Menes/Internal/HttpRequestParameterBuilder.cs

Lines 192 to 209 in 6b2b63d

	return defaultValue switch
	{
	//// Convert to DateTimeOffset rather than use the DateTime supplied by default to be consistent with our converters.
	OpenApiDate d when schema.Format == "date" => new DateTimeOffset(DateTime.SpecifyKind(d.Value, DateTimeKind.Utc)),
	//// Convert to string rather than use the DateTimeOffset supplied by default to be consistent with our current (lack of) support for strings with format "date-time".
	OpenApiDateTime dt when schema.Format == "date-time" => dt.Value.ToString("yyyy-MM-ddTHH:mm:ssK"),
	OpenApiPassword p when schema.Format == "password" => p.Value,
	OpenApiByte by when schema.Format == "byte" => by.Value,
	OpenApiString s when schema.Type == "string" => this.ConvertValue(schema, s.Value),
	OpenApiBoolean b when schema.Type == "boolean" => b.Value,
	OpenApiLong l when schema.Format == "int64" => l.Value,
	OpenApiInteger i when schema.Type == "integer" => i.Value,
	OpenApiFloat f when schema.Format == "float" => f.Value,
	OpenApiDouble db when schema.Type == "number" => db.Value,
	OpenApiArray a when schema.Type == "array" => HandleArray(schema.Items, a),
	OpenApiObject o when schema.Type == "object" => HandleObject(schema.Properties, o),
	_ => throw new OpenApiSpecificationException("Default value for parameter not valid."),
	};

This appears to be looking for type mismatches, e.g., mistakes of this kind:

{ "type": "integer", "format": "int64", "default": "foo" }

It so happens that it also picks up the case where the numeric value is too large:

{ "type": "integer", "format": "int64", "default": 9223372036854775808123123123 }

This works only because Microsoft.OpenApi reported something that was a number as though it had been a string. The effect is that this code misdiagnoses this as a type mismatch but then goes on to throw the right kind of exception anyway. Perhaps this was deliberate (although it's such a non-obvious way of achieving this that a comment wouldn't have gone amiss).

In conclusion, the three examples here get caught by three completely different code paths in our current implementation. These happen to use different exception types to report the problems, but it looks like the test just reflects this implementation accident, rather than expressing what we would actually want.

(A side issue is that the test here produces Open API schemas that aren't actually valid JSON. When the DefaultValue is a string, it doesn't wrap it in quotes, so you end up with something like { "default": This is certainly not a date }. It turns out that Microsoft.OpenApi tolerates this, and reports it as though you'd written { "default": "This is certainly not a date" }.)