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

Provide typed Access to Header-Fields #616

Closed
drothmaler opened this issue Feb 22, 2024 · 16 comments
Closed

Provide typed Access to Header-Fields #616

drothmaler opened this issue Feb 22, 2024 · 16 comments
Labels
enhancement New feature or request implemented requested feature has been implemented

Comments

@drothmaler
Copy link

In .NET there is a way to access Request Headers in a typed way, using Request.GetTypedHeaders() and the RequestHeaders wrapper class.

It would be really nice to have something similar in FastEndpoints. Especially for those properties, that are actually weighted lists, instead of simple values (like the Accept header - to name one prominent example).

I know that I could still access the typed headers using Endpoint.HttpContext.Request.GetTypedHeaders(), but that feels a bit like working around the framework.

I'm not sure how to implement that in a idiomatic way. The quick & dirty solution would probably be to provide a Json converter for IList<MediaTypeHeaderValue>, IList<StringWithQualityHeaderValue>, ContentRangeHeaderValue and alle the other helper classes, as they are used in the RequestHeaders class. That way, it would be possible to use similar declarations in custom Request classes.

The problem with this, is that it still wouldn't be actually type-safe. Another variant could be to provide some kind of Request base class, that declared all the headers as protected properties. To make it visible in the API-Spec I would than have to override it and annotate it with [FromHeader]. But I didn't yet, think that through to be honest. So maybe someone can come up with a better idea.

@dj-nitehawk
Copy link
Member

dj-nitehawk commented Feb 22, 2024

this seems to work. tested with a sample content-disposition value attachment; filename="filename.jpg"

var bld = WebApplication.CreateBuilder(args);
bld.Services
   .AddFastEndpoints()
   .SwaggerDocument();

var app = bld.Build();
app.UseFastEndpoints(
       c => c.Binding.ValueParserFor<ContentDispositionHeaderValue>(
           input =>
           {
               if (ContentDispositionHeaderValue.TryParse(new((StringValues)input), out var res))
                   return new(true, res);

               return new(false, null);
           }))
   .UseSwaggerGen();
app.Run();

sealed class MyRequest
{
    public int Id { get; set; }

    [FromHeader("Content-Disposition")]
    public ContentDispositionHeaderValue Disposition { get; set; }
}

sealed class MyEndpoint : Endpoint<MyRequest>
{
    public override void Configure()
    {
        Post("test");
        AllowAnonymous();
    }

    public override async Task HandleAsync(MyRequest r, CancellationToken c)
    {
        await SendAsync(r.Disposition.FileName.Value);
    }
}

someone's gonna have to write "value parsers" for all of the typed request header types. that ain't gonna be me though 😜
if someone can write a list of value parsers, i can integrate it into the library.

so the requirement is to write a static class with a static property of type IList<Func<object?, ParseResult>>.
which i just need to loop over and add the value parser funcs to the cache.

also gonna need unit tests for each of the typed header types 😁

@dj-nitehawk dj-nitehawk added the discussion this is a general discussion or opinion based argument label Feb 22, 2024
@drothmaler
Copy link
Author

I think I'll try to implement those converters during the next days... Let's see how it turns out :-)

@dj-nitehawk
Copy link
Member

dj-nitehawk commented Feb 23, 2024

i figured out how to bake in the support for parsing typed headers. so no need to manually write parsers for every header type. as long as header types have a consistent TryParse method with a signature like below, it will automatically parse and bind values.

public static bool TryParse(StringSegment input, out XXXXXXXXX? parsedValue)

just working out the final kinks. will publish a beta for you to test out soon.

@dj-nitehawk dj-nitehawk added enhancement New feature or request implemented requested feature has been implemented and removed discussion this is a general discussion or opinion based argument labels Feb 23, 2024
@dj-nitehawk
Copy link
Member

check out v5.22.0.13-beta. the following now works out of the box:

using System.Net.Http.Headers; // THIS IS IMPORTANT. DON'T USE Microsoft.Net.Http.Headers

var bld = WebApplication.CreateBuilder(args);
bld.Services
   .AddFastEndpoints()
   .SwaggerDocument();

var app = bld.Build();
app.UseFastEndpoints()
   .UseSwaggerGen();
app.Run();

sealed class MyRequest
{
    [FromHeader("Content-Disposition")]
    public ContentDispositionHeaderValue Disposition { get; set; }
}

sealed class MyEndpoint : Endpoint<MyRequest>
{
    public override void Configure()
    {
        Post("test");
        AllowAnonymous();
    }

    public override async Task HandleAsync(MyRequest r, CancellationToken c)
    {
        await SendAsync(r.Disposition.FileName);
    }
}

@drothmaler
Copy link
Author

Wow, that was fast, thanks… I’ll try it as soon as possible…
From a first look at the Commit, I didn‘t see any code specific to the List-Style headers (like Accept*, Cookie and If*Match). Or did I miss something?
Also: may I ask, why you chose to use the types from System.Net.Http instead of Microsoft.Net.Http? I was under the Impression, that the System.Net Namespace is kind of a legacy one - and the Microsoft Namespace is the preferred way to go since .NET Core…
Anyway, I‘ll try it out till Monday - and maybe I‘ll add a PR with some more tests too. Thanks again :-)

@dj-nitehawk
Copy link
Member

initially i made it work with Microsoft.Net.Http.Headers. then i compared it with System.Net.Http.Headers and chose to only support System.Net.Http.Headers because:

  • it has more typed header classes than Microsoft.Net.Http.Headers
  • binding via TryParse(string?, out xxx) method signature was already supported.
  • Microsoft.Net.Http.Headers TryParse has a different signature with StringSegment which would need additional allocations.
  • System.Net.Http.Headers is the RFC compatible namespace.

and please, additional test cases would be much appreciated 🙏🙏🙏

@drothmaler
Copy link
Author

Hey I just did some tests today... Unfortunately I was right regarding the list style headers... If I try to do a call using multiple Accept header values, I get: "The JSON value could not be converted to System.Net.Http.Headers.MediaTypeWithQualityHeaderValue. Path: $[0] | LineNumber: 0 | BytePositionInLine: 12."
I'll try to have a closer look on it tomorrow...

Regarding S.N.H vs. M.N.H:

  • Can't say much regarding the pure class count, but the System namespace seems to have quite some unnecessary abstractions und the Microsoft namespace seems to also contain all the needed classes - but I didn't do a deep analysis.
  • StringSegment seems to be a really thin wrapper around String - so I don't think it would make much of difference. Alle the String building done in DeserializeJsonArrayString seem to be a bigger issue to me - but without doing any benchmarks, all this is just wild guessing. And of cause the implementation for the StringSegment base parsing methods might be a bit more complex - but again it doesn't seem to be a big difference.
  • Regarding the RFC compatibility: M.N.H also mentions RFC 2612 compatibility at several places (and the more modern RFC 6265).

Long story short, I'd still prefer to use Microsoft.Net.Http.Headers or to support both - but I'll take what ever I can get :-)

@dj-nitehawk
Copy link
Member

v5.22.0.16-beta now supports any typed header which ends with the name HeaderValue from any namespace as long as the type has a TryParse method with either a string or StringSegment input type.

i have no idea how to support lists of typed headers though. other than having to register a json converter for each list type.

@drothmaler
Copy link
Author

Well it should be rel. straightforward for the Microsoft.Net.Http.Headers types... There are TryParseList methods, on the relevant types...

System.Net.Http.Headers on the other hand is a whole different story... They use HttpHeaderValueCollection for handling the list-headers there. And it is coupled pretty tightly to HeaderDescriptor and HttpHeaders. Also it has not public constructor. So I don't think there is any (easy) way to implement list header handling there (without rewriting most of the logic) - so I'd just drop it

@dj-nitehawk
Copy link
Member

i think i got it with v5.22.0.17-beta, with a more static approach by registering a value parser func per all known typed headers from m.n.h namespace:

//header parsers
o.ValueParserFor<CacheControlHeaderValue>(input => new(CacheControlHeaderValue.TryParse(new((StringValues)input!), out var res), res));
o.ValueParserFor<ContentDispositionHeaderValue>(input => new(ContentDispositionHeaderValue.TryParse(new((StringValues)input!), out var res), res));
o.ValueParserFor<ContentRangeHeaderValue>(input => new(ContentRangeHeaderValue.TryParse(new((StringValues)input!), out var res), res));
o.ValueParserFor<MediaTypeHeaderValue>(input => new(MediaTypeHeaderValue.TryParse(new((StringValues)input!), out var res), res));
o.ValueParserFor<RangeConditionHeaderValue>(input => new(RangeConditionHeaderValue.TryParse(new((StringValues)input!), out var res), res));
o.ValueParserFor<RangeHeaderValue>(input => new(RangeHeaderValue.TryParse(new((StringValues)input!), out var res), res));
o.ValueParserFor<EntityTagHeaderValue>(input => new(EntityTagHeaderValue.TryParse(new((StringValues)input!), out var res), res));
//list header parsers
o.ValueParserFor<IList<MediaTypeHeaderValue>>(input => new(MediaTypeHeaderValue.TryParseList((StringValues)input!, out var res), res));
o.ValueParserFor<IList<EntityTagHeaderValue>>(input => new(EntityTagHeaderValue.TryParseList((StringValues)input!, out var res), res));
o.ValueParserFor<IList<SetCookieHeaderValue>>(input => new(SetCookieHeaderValue.TryParseList((StringValues)input!, out var res), res));

it works with IList<xxxx> now but.
only works in .net 8+ unfortunately.

@drothmaler
Copy link
Author

Wo, that was fast, thanks! I was just about to finish a generic implementation... But your looks much more straightforward :-) I'll test that one.

@dj-nitehawk
Copy link
Member

if they had used a common interface for the tryparse methods, the above could have been much simpler :-(
did you have to use reflection in your generic implementation?

@drothmaler
Copy link
Author

You can check out my work-in-progress implementation here: drothmaler@bac2a2b

@dj-nitehawk
Copy link
Member

that looks alright at first glance. does it fully work with all possible header types?

didn't wanna go down that route myself because of complicating the expression tree building logic.
that code is already ugly af for my taste 😁

the downside to my method would be that we need to update it when MS adds new header types.

@drothmaler
Copy link
Author

Doing some tests right now. Looks good so far, but doesn't make the Binder any prettier, as you said...
Once I added some more tests, I might be more confident to try some cleanup/refactoring of the code.

@dj-nitehawk
Copy link
Member

great! let me know how it goes...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request implemented requested feature has been implemented
Development

No branches or pull requests

2 participants