ChromiumDotNet / Chromium.AspNetCore.Bridge

Chromium.AspNetCore.Bridge
Apache License 2.0
26 stars 10 forks source link

Query string starts with question mark #7

Closed mattkol closed 3 years ago

mattkol commented 3 years ago

@amaitland

The query string from the interceptor retains question mark query marker.

https://cefsharp.test/Movies?MovieGenre=&SearchString=ghost decomposed to:

image

This is from: https://github.com/amaitland/Chromium.AspNetCore.Bridge/blob/50e60b41c9855d7eefedef538f1d76823610f7d9/Chromium.AspNetCore.Bridge/RequestInterceptor.cs#L63

This works:

 {"owin.RequestQueryString", uri.Query?.Trim().TrimStart('?')},
amaitland commented 3 years ago

owin.RequestQueryString | A string containing the query string component of the HTTP request URI, without the leading "?" The value may be an empty string.

http://owin.org/html/spec/owin-1.0.html

Confirming the leading '?' should be trimmed as per the OWIN spec.

The odd part is that IHttpRequestFeature.QueryString says there should be a leading ?.

https://github.com/amaitland/Chromium.AspNetCore.Bridge/blob/master/Chromium.AspNetCore.Bridge/OwinFeatureImpl.cs#L89

Code was originally based on https://github.com/dotnet/aspnetcore/blob/c925f99cddac0df90ed0bc4a07ecda6b054a0b02/src/Http/Owin/src/OwinFeatureCollection.cs#L111

{"owin.RequestQueryString", uri.Query?.Trim().TrimStart('?')},

Do we need the overhead of Trim() and TrimStart()? Do we need more than a null check, maybe a length check and a SubString(1)?

amaitland commented 3 years ago

I'm wondering if we change AddQuestionMark to 'AddQuestionMarkIfRequiredand append one if required, I suspect currently it's??MovieGenre`.

Conforming strictly to the OWIN spec doesn't make much sense here at least from a performance point of view, remove the question mark, only to add one pretty much straight aware, happy to have a contrary opinion.

amaitland commented 3 years ago

Conforming strictly to the OWIN spec doesn't make much sense here at least from a performance point of view

Quick look and it may actually be slightly more performant, there are potentially a many calls to IHttpRequestFeature.QueryString the string.IsNullorEmpty check in combination with the queryString[0] == '?' check would probably be fractionally more expensive.