Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better collection type for dictionary-like things where duplicate keys are possible #541

Closed
tmenier opened this issue Aug 7, 2020 · 5 comments

Comments

@tmenier
Copy link
Owner

tmenier commented Aug 7, 2020

A common problem has arisen with things like query parameters, request/response headers, and cookies: IDictionary<string, T> feels like the perfect interface, except that duplicate keys, while uncommon, are technically allowed in all cases. That's a bummer, because it could mean replacing something simple like:

headers["x"]

with something more messy like:

headers.FirstOrDefault(h => h.Name == "x")?.Value

Flurl is all about reducing keystrokes so I want to do something better than this, and standardize on it throughout the library where it seems appropriate. After scanning the collections landscape, nothing fit quite right out of the box, so I've arrived at something custom based on IList<ValueTuple> with a few additional methods optimized for the most common case.

IReadOnlyNameValuleList<TValue>

Inherits from IReadOnlyList<(string Name, TValue Value)> and adds the following methods:

TValue FirstOrDefault(string name);
bool TryGetFirst(string name, out TValue value);
IEnumerable<TValue> GetAll(string name);
bool Contains(string name);
bool Contains(string name, TValue value);

INameValuleList<TValue>

Inherits from IList<(string Name, TValue Value)>, includes all methods above, and adds the following methods:

void Add(string name, TValue value);
void AddOrReplace(string name, TValue value); // if multiple exist, replaces the first and removes the rest
bool Remove(string name); // if multiple exist, removes them all

In the test suite and actual library code, these interfaces seemed to pass the "feel test" of being about as easy to use as dictionaries for the common cases while not hiding the fact that you're dealing with the "first" item of a given name, not necessarily the only one.

I'm experimenting with these types for headers and cookies first; query params may come later. Once things are a little more set in stone, I'll update this issue with the specific breaking changes, which should be expected in the final 3.0 release.

@ghost
Copy link

ghost commented Aug 8, 2020

After the official version 3.0 is released, will you update the new document on https://flurl.dev?

@tmenier
Copy link
Owner Author

tmenier commented Aug 8, 2020

@mincasoft yes, docs will be updated with the full release.

@tmenier
Copy link
Owner Author

tmenier commented Aug 8, 2020

As of 3.0-pre4, this is now used for a couple things:

  • IFlurlRequest.Headers and IFlurlClient.Headers are now INameValueList<string>.
  • IFlurlResponse.Headers is now IReadOnlyNameValueList<string>.

I considered using it for IFlurlRequest.Cookies, but I went with IEnumerable<(string Name, string Value)> because (at least for now) it is generated by parsing the Cookie header on the fly, and IEnumerable implies that there could be some overhead in re-enumerating, which is true in this case. I could change my mind on this based on feedback in #506, but for now I went with this as a very simple way to keep the Cookies collection in sync with the Cookie header.

@tmenier
Copy link
Owner Author

tmenier commented Oct 1, 2020

As noted in #553, headers names are case-insensitive (https://stackoverflow.com/a/5259004/62600) but names in this collection aren't. Since the new collection type is also used for query strings (#555) and internally for cookies, both of which should be case-sensitive, let's add a constructor arg to make the type work either way.

@tmenier
Copy link
Owner Author

tmenier commented Oct 24, 2020

Collection type updated with case-sensitivity switch, applied as follows:

  • Header names: case-insensitive
  • Cookie names: case-sensitive
  • Query param names: case-sensitive

This is in 3.0 prerelease 6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant