Expose Query Parameters

[j82w]

Here is the proposed API changes to make it where user can access the query parameters. This is a follow up to PR #802 (comment)

Option 1:
public IEnumerable<(string key, object value)> GetQueryParamters();

Option2:
public IReadOnlyList<(string key, object value)> GetQueryParamters();

Option 3: Does it give a snapshot or a mapping to the underlying data structure?
public IReadOnlyDictionary<string, object> GetQueryParamters();

@thomaslevesque, @FabianMeiswinkel, and @kirankumarkolli what are you opinions?

[FabianMeiswinkel]

My intuition tells me most developers would find it easiest to think about parameters as a Dictionary so I would prefer Option 3. But I would also sign-off on Option 2 or 1 because ultimately they allow accessing individual parameter values as well.

Snapshot vs. mapping: IMO a mapping is sufficient. A snapshot would provide better performance if the GetQueryParameters would be called frequently - but because it isn't used internally in the query pipeline I don't see that happening. If you use a snapshot it is extra sugar - just please make sure that the snapshot gets reset on modifications to avoid the issue I raised in one of the comments on the original PR.

[thomaslevesque]

Option 1 and 2 are fine, and are the easiest to do. Option 2 is probably slightly better, since it provides the number of parameters.

Option 3 is OK, as long as the documentation makes it clear that the method returns a snapshot at the time of the call. However, it allocates a new dictionary every time...

Directly exposing the parameters as a dictionary would be ideal, but since the underlying storage is a list, there's no efficient way to do that.

Snapshot vs. mapping: IMO a mapping is sufficient. A snapshot would provide better performance if the GetQueryParameters would be called frequently - but because it isn't used internally in the query pipeline I don't see that happening. If you use a snapshot it is extra sugar - just please make sure that the snapshot gets reset on modifications to avoid the issue I raised in one of the comments on the original PR.

@FabianMeiswinkel I think we have a different definition of "snapshot". The way I understand it, it's a copy of the parameters to an independant collection. That collection will not reflect changes made to the query definition after the call to GetQueryParameters.

[ImrePyhvel]

Option 1 would make the most sense since it is the simplest. It allows to stop enumerating once the interesting piece is found while still being open to consuming the full set.

Not anyone wants to know the number of parameters or consume them as dictionary, and if they do, it is one trivial LINQ command away. IMHO API should not try to be clever in advance and do extra work before it is being asked for.

I dislike unnamed tuples in API, but I could live with it

[thomaslevesque]

@ImrePyhvel Option 2 isn't really more expensive, so we might as well do it. It's trivial to expose a list that's just a wrapper for the actual parameters.

[thomaslevesque]

I dislike unnamed tuples in API, but I could live with it

The tuple members would be named. But yes, it's probably better to return a specific QueryParameter type; it's already been discussed in the PR.

[j82w]

@thomaslevesque we are good with any of the implementations. We would prefer a tuple to avoid exposing a new type just for this scenario. It can be a named tuple. If you send another PR with your preferred API we will get it merged.

[thomaslevesque]

@j82w That works for me. I'll send a new PR, the original one is too messed up now...

[thomaslevesque]

Here it is: #2210