fix: _get_kwargs bool headers are now converted to strings

[John98Zakaria]

fixes : #552

[John98Zakaria]

I didn't fully grasp how to integrate my openapi spec to the existing one.
It is missing a test to prevent future regression

[dbanty]

Thanks for providing the fix! I've been pondering it since you opened the PR and I'm not sure what the correct path forward is. Providing "True" and "False" as header values is very pythonic and is likely not what general APIs are expecting when they ask for a bool in a header. More likely they want the JSON-ish "true" and "false" or "1" and "0" (as I've seen in some implementations).

There is no guidance in the spec as far as I can see. I've seen conflicting suggestions in other web resources (like the Mozilla developer's guide) suggesting that "?1" and "?0" are the correct values for booleans in headers.. but I've never seen a server implementation that knows how to represent those.

My inclination is to just make it a generation error to have a boolean anywhere in a header so as to not cause obscure runtime errors when people try to use them. Technically nothing but strings should be supported, but OpenAPI does provide guidance on transforming objects, arrays, and numbers into strings based on the spec. I don't think this generator will transform all of that properly today though.

[John98Zakaria]

I looked at the json specification.
https://www.ecma-international.org/publications-and-standards/standards/ecma-404/
Indeed it expects "true" "false" which I can certainly add to my merge request.

One tags of the project is fastapi, which generated this openapi spec.
By making it a generation error we are potentially preventing users from using the library entirely.
Later I will check what .net generates as well as next.js.

We shouldn't make our quest for perfection prevent a partial solution.

[John98Zakaria]

Closed by mistake, my hand tapped on close by accident

[John98Zakaria]

I generated using .net 6 minimal API another OpenApiSpec that had a Boolean header.
Then generated a client to send "True" and "False" which it happily accepted.
If you want I can still build in the lowering.
FastAPI accepts it as well.

using Microsoft.AspNetCore.Mvc;

var builder = WebApplication.CreateBuilder(args);

builder.Services.AddEndpointsApiExplorer();
builder.Services.AddSwaggerGen();
var app = builder.Build();
app.UseSwagger();

app.MapGet("/", () => "Hello World!");
app.MapPost("/boolHead", ([FromHeader(Name = "bool-header")] bool header) => header);
app.UseSwaggerUI();

app.Run();

minimalDotNetOpenAPI.txt

[dbanty]

One tags of the project is fastapi, which generated this openapi spec.
By making it a generation error we are potentially preventing users from using the library entirely.
Later I will check what .net generates as well as next.js.

Boolean headers are allowed by OpenAPI (and indeed FastAPI), unfortunately, there's just no guidance in the OpenAPI spec about how to send them. This is far from the first time we've had to decide on how to narrow the spec for this project because the spec was not specific enough, just always feels bad to do it.

We shouldn't make our quest for perfection prevent a partial solution.

Indeed, I'm just always cognizant of the fact that breaking changes are major problems down the line. So once we commit to a solution here, it's going to be hard to change course. It would be nice to pick the most broadly accepted solution so we don't regret it later.

I looked at the json specification.
https://www.ecma-international.org/publications-and-standards/standards/ecma-404/
Indeed it expects "true" "false" which I can certainly add to my merge request.

I generated using .net 6 minimal API another OpenApiSpec that had a Boolean header.
Then generated a client to send "True" and "False" which it happily accepted.
If you want I can still build in the lowering.
FastAPI accepts it as well.

Since there is no universal guidance on passing types other than strings to headers, falling back to "how JSON does it" seems like a safe bet. Can you verify that lowercase "true" and "false" are also accepted by those frameworks? Header values are generally considered case-sensitive, so I'd rather stick to all lower case as it might help future compatibility.

[John98Zakaria]

I added the lowercasing to the parameters and tested it on fastapi and .net core 6
Both are happy with it

[codecov]

Codecov Report

Merging #553 (14c8051) into main (23aebd1) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #553   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           48        48           
  Lines         1661      1661           
=========================================
  Hits          1661      1661           

Impacted Files	Coverage Δ
openapi_python_client/__init__.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 23aebd1...14c8051. Read the comment docs.

[dbanty]

Thanks for all the work on this @John98Zakaria! I'm going to take it over in #556 because I feel like having each type express how it can be put in a header is more maintainable long-term than the one special case for bools. Gonna be a much larger change 😅 but better to do it now than come back later I think.