Skip to content

Conversation

ostorc
Copy link
Contributor

@ostorc ostorc commented May 23, 2020

This PR adds support for parsing extension-uv and storing data retrieved from it.

Fixes #22077

Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a good start. The tests will be the most telling for a change like this. Don't forget to update the reference assemblies.

@@ -554,12 +580,13 @@ public override bool Equals(object obj)
&& StringSegment.Equals(Path, other.Path, StringComparison.OrdinalIgnoreCase)
&& Secure == other.Secure
&& SameSite == other.SameSite
&& HttpOnly == other.HttpOnly;
&& HttpOnly == other.HttpOnly
&& HeaderUtilities.AreEqualCollections(Extensions, other.Extensions);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this consider order of extensions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed that to consider order of extensions

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I meant the opposite, order shouldn't matter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I was thinking what you meant by that and chose the wrong option. I have reverted it back and changed the test for it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I just meant to ask about the behavior of HeaderUtilities.AreEqualCollections.

Copy link
Contributor Author

@ostorc ostorc May 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm things. The HeaderUtilities.AreEqualCollections is indeed checking if two unordered list have same content. It does it in O(n*m) time, but since the number of extensions is not likely to reach any high number, so it doesn't really matter.

ostorc added 3 commits May 24, 2020 12:48
Because extension-av doesn't have any fixed format we have to look after
commas when parsing to determine end of Set-Cookie string
@ostorc ostorc marked this pull request as ready for review May 24, 2020 10:54
@ostorc ostorc requested a review from jkotalik as a code owner May 24, 2020 10:54
@ostorc ostorc changed the title [WIP] Parsing extension-av on Set Cookie header Parsing extension-av on Set Cookie header May 24, 2020
@Tratcher Tratcher self-assigned this May 25, 2020
@ostorc ostorc requested a review from Tratcher May 25, 2020 18:56
Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few style issues to address.

The commas are my biggest behavioral question though.

@BrennanConroy BrennanConroy added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label May 29, 2020
@BrennanConroy
Copy link
Member

@ostorc Can you take a look at the feedback?

@shirhatti
Copy link
Contributor

@ostorc Are you still working on this?

@ostorc
Copy link
Contributor Author

ostorc commented Jun 6, 2020

Hi, I am sorry for the dealy. I had really busy week. I will finish changes today and update the PR.

@ostorc ostorc requested a review from Tratcher June 6, 2020 20:37
Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, one merge conflict to resolve and I think you're done.

@ostorc
Copy link
Contributor Author

ostorc commented Jun 9, 2020

Conflict resolved

@Tratcher
Copy link
Member

Tratcher commented Jun 9, 2020

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parsing a cookie fails when there is an invalid key in the cookie value
5 participants