Websocket handshake refactoring.

[ShreyasJejurkar]

Addresses #31373

I have moved CheckSupportedWebSocketRequest method from HandShakeHelpers to WebSocketMiddleware.
I am not getting what needs to be done for NeededHeaders do we want to remove those directly?

cc @Tratcher

[BrennanConroy]

I think the main thing is that the temporary interestingHeaders allocation isn't needed if we merged the two methods.

[umitkavala]

Yeah. Instead of allocation you'd do query on request headers directly.
You may do something like this I guess.

public bool IsWebSocketRequest
           {
               get
               {
                   if (_isWebSocketRequest == null)
                   {
                       if (!_upgradeFeature.IsUpgradableRequest)
                       {
                           _isWebSocketRequest = false;
                       }
                       else
                       {
                           _isWebSocketRequest = CheckSupportedWebSocketRequest(_context.Request.Method, _context.Request.Headers);
                       }
                   }
                   return _isWebSocketRequest.Value;
               }
           }

           public static bool CheckSupportedWebSocketRequest(string method, IHeaderDictionary requestHeaders)
           {
               bool validUpgrade = false, validConnection = false, validKey = false, validVersion = false;

               if (!string.Equals("GET", method, StringComparison.OrdinalIgnoreCase))
               {
                   return false;
               }

               foreach (var pair in requestHeaders)
               {
                   if (string.Equals(HeaderNames.Connection, pair.Key, StringComparison.OrdinalIgnoreCase))
                   {
                       check requestHeaders.GetCommaSeparatedValues(HeaderNames.Connection) instead below and other ifs
                      // if (string.Equals(HeaderNames.Upgrade, pair.Value, StringComparison.OrdinalIgnoreCase))
                       {
                           validConnection = true;
                       }
                   }
.......

[davidfowl]

Assigned to @Tratcher since he suggested the change 😄

[ShreyasJejurkar]

Hi @BrennanConroy, I have removed an extra parameter to that method, and moved interestingHeaders to another method. Yeah, I haven't yet removed intrestingHeaders.
But I am not understanding before iterating request headers directly, we want to iterate only specific headers correct? So before foreach we need to anyhow filter them!

Sorry, if I am missing any important point!

[ShreyasJejurkar]

Hi @umitkavala , Thanks for the reply. But GetCommaSeparatedValues don't return boolean to check condition, it returns array of string!

[BrennanConroy]

Oh good, tests failed, means we have good coverage 😃

[BrennanConroy]

Updated

[BrennanConroy]

I think this test has been broken for 5 years...

[BrennanConroy]

Thanks @MCCshreyas ! Hope your computer issues are resolved soon 😃

[ShreyasJejurkar]

Not yet @BrennanConroy, I ordered a SSD, not sure when it will get delivered now! 😅😅

But thanks for taking care of this PR. 😊
Much appreciated!

[ghost]

Hi @MCCshreyas. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.