OData / RESTier

A turn-key library for building RESTful services
http://odata.github.io/RESTier
Other
471 stars 137 forks source link

Replace Non Standard IfMatch Header with Standard If-Match Header #747

Open yulivee opened 9 months ago

yulivee commented 9 months ago

When working with the OData E-Tag, Restier expects the Header to contain IfMatch.

This Header actually contains a hyphen, according (see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/If-Match). Per RFC 7232 the Notation is also If-Match (see https://www.rfc-editor.org/rfc/rfc7232#section-3.1)

When working with E-Tags, we currently require a fix in our request pipeline to rewrite the header:

            // Fix IfMatch --> If-Match
            app.Use(async (context, next) =>
            {
                if (context.Request.Headers.IfMatch.Count > 0)
                {
                    context.Request.Headers.Add("IfMatch", context.Request.Headers.IfMatch.First());
                }

                await next();
            });

For Batch Requests we need an additional Batch-Interceptor to rewrite the header (which I am currently still working on)

Expected result

I expect ResTier to work with Standard HTTP Headers and not need a workaround for this

Actual result

Restier expects the Header in the Notation IfMatch

robertmclaws commented 9 months ago

Can you please tell me which version of Restier you are using?

Looking at the code, Restier just gets the information from OData, and the latest versions of both Microsoft.AspNet.OData and Microsoft.AspNetCore.OData (v7.2.2) use the same If-Match header specified in ODataQueryOptions.IfMatch.

vincedan commented 8 months ago

“If-Match” header is defined in OData protocol 3.0/4.0/4.01. and restier 1.0.0 beta supports it perfectly. I was wondering something was wrong in later version upgrade. mybe the first initials for supporting netcore. src/Microsoft.Restier.AspNetCore /RestierController.cs line 627, "IfMatch" as literal string for check etag and the logic is different in 1.0.0 beta version. @jspuij.

@robertmclaws related testcases is also important..

jspuij commented 8 months ago

Pretty sure that I got that method from OData source at that time to minimize reflection calls. It might be beneficial to either get OData to friend us (InternalsVisibleTo), or to use reflection anyway to avoid having to fix these things in both projects.

@robertmclaws what do you think? Be smart and try to fix it the proper way or just do a simple bugfix...

Scratch that, I think it's a copy of the controller at that time.

robertmclaws commented 8 months ago

@jspuij I mean, we need to catch up on some stuff, so happy to grab some time to dig in. I seem to remember taking a look and seeing that we were doing it the OData way, so if there's a problem it may be in WebAPI or ODL, at which point we may need to contribute a fix higher up the stack.