Skip to content
This repository was archived by the owner on Nov 20, 2018. It is now read-only.

Harden the PathString converter #857

Closed
wants to merge 5 commits into from
Closed

Harden the PathString converter #857

wants to merge 5 commits into from

Conversation

HaoK
Copy link
Member

@HaoK HaoK commented Jun 6, 2017

@davidfowl
Copy link
Member

Isn't this in preview2?

@HaoK
Copy link
Member Author

HaoK commented Jun 6, 2017

Yeah this is just cleanup, but I can make this change in preview 2 if we want

return new PathString((string)value);
}
=> value is string
? new PathString((string)value)
Copy link
Member

Choose a reason for hiding this comment

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

This replicates the asymmitry bug from the implicit converters:
#690

string -> PathString should use FromUriComponent(string), not new PathString(string)

We should change them both at the same time. @davidfowl

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me see if I can unify the code a bit to make that change easier

@HaoK
Copy link
Member Author

HaoK commented Jun 6, 2017

So I added a new

internal static PathString ConvertFromString(string s) => string.IsNullOrEmpty(s) ? new PathString(s) : FromUriComponent(s);

That both the implicit op and ConvertFrom call, but it wasn't obvious to me exactly how to write a test verifying that the round tripping issue was fixed.

@Tratcher
Copy link
Member

Tratcher commented Jun 12, 2017

The most obvious things that are broken by the asymmetry are the equality operators. E.g. Equals, StartsWithSegments, GetHashCode, etc..
Here's a repro:

            PathString p1 = "/?";
            String s1 = p1; // Becomes "/%3F"
            PathString p2 = s1; // Stays "/%3F"
            Console.WriteLine(p1.Equals(p2)); // false

@HaoK
Copy link
Member Author

HaoK commented Jun 15, 2017

Added a test verifying the equality now work

@HaoK
Copy link
Member Author

HaoK commented Jun 15, 2017

Now also fixes: #690

{
PathString p1 = "/?";
string s1 = p1; // Becomes "/%3F"
PathString p2 = s1; // Stays "/%3F"
Copy link
Member

@Tratcher Tratcher Jun 15, 2017

Choose a reason for hiding this comment

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

Stays "/%3F" is wrong after your fix, it gets unescaped again

@Tratcher
Copy link
Member

ping

@HaoK
Copy link
Member Author

HaoK commented Jun 20, 2017

Yeah I'll merge this later today

@HaoK
Copy link
Member Author

HaoK commented Jun 20, 2017

12f89f6

@HaoK HaoK closed this Jun 20, 2017
@HaoK HaoK deleted the haok/string branch August 7, 2017 17:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants