Skip to content

Conversation

Deantwo
Copy link
Contributor

@Deantwo Deantwo commented Jan 16, 2020

Description

Throw exception on invalid resource query
Fixes #1404

Purpose

This pull request is a: Bugfix (non-breaking change which fixes an issue)

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@Deantwo
Copy link
Contributor Author

Deantwo commented Jan 16, 2020

Old behavior mentioned in #1404 allowed a query with ?& because it removed empty entries. I don't think this is allowed in a Uri, is it?
If not then we can simply change the Split to ignore empty entries.

query.Split(new char[] { '&' }, StringSplitOptions.RemoveEmptyEntries)

But I would think that throwing an error to make the user aware that the Uri is wrong is a good idea.
Thoughts?

@Deantwo Deantwo requested a review from alexeyzimarev January 16, 2020 14:59
@Deantwo
Copy link
Contributor Author

Deantwo commented Jan 16, 2020

Found this nice little article about query strings, and it sadly doesn't answer the question of rather it is legal or not to have empty key and null value, but it does suggest that it at least makes no sense in JSON.

I looked at the RFC, but it didn't seem to mention any finer rules on this that I could see.
I mean need to know if these are valid and should be ignored, or if an ArgumentException should be thrown.
Examples:

  • ?&
  • &&
  • &(end of string)

Simply ignoring the empty split entries seem like the safer bet, and is backwards compatible with old behavior. But I feel almost afraid to make a new pull request though, since you also need to handle empty key and same key multiple times.

@Deantwo
Copy link
Contributor Author

Deantwo commented Jan 17, 2020

test?key1=value1&=value2

Result in: "=value" = ""
image
This pull request doesn't affect this.

So it feels like the entire static IEnumerable<NameValuePair> ParseQuery(string query) method might need to be changed. Something I don't know if I can do, or if it is even a good idea to do it in the first place.
Might be better to find some built-in features or another library to do this correctly, rather than try to re-invent the wheel again.

@alexeyzimarev
Copy link
Member

The thing is that the Uri class constructor does some checks to ensure the given uri is valid. Apparently, it doesn't see an issue with ?&. I think ignoring empty entries is the best way to handle this. I don't think throwing an exception is a good idea since it will possibly introduce a breaking change.

@Deantwo Deantwo closed this Jan 23, 2020
@Deantwo Deantwo deleted the patch-1 branch January 24, 2020 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Value cannot be null error on RestRequest

2 participants