Query Parameters shouldn't be mandatory?

[Andersmholmgren]

It seems unintuitive to fail a match when query parameters are missing.

e.g. a route like

search{?name}

curl -X GET '/search' would fail to match but that is not what I would consider typical behaviour. You normally want the request to match and the name parameter to be null.

[Andersmholmgren]

Any thoughts on this one?
I'm thinking of adding a named bool argument that if true (default is false) causes it to not fail matching based on query params but still populate the values

[dristic]

👍 for this issue. It would be great if the query params could be optional, maybe using the same [] syntax as optional params in dart:

search{?name,[price],[id],[sort]}

Meaning name is required but will still match if price, id, or sort do not exist.

[justinfagnani]

I'll take a look and see if there is a way to fit this in the syntax. We can't invent new syntax and stay compatible with URI Templates.

[justinfagnani]

I don't immediately see a nice way to express this in the syntax so that you can individually define optional query parameters. But I do see a few options, not all excelusive:

1. Add an named parameter to match() like @Andersmholmgren suggests.
2. Change the return value of parse() to be a Result object rather than a map, and have it include a query parameter map. Not sure how useful that is since you presumably have access to the original Uri object and its queryParameters map. This idea here would be that you just don't add the optional parameters.
3. Use the explode operator (*) to capture all unmatched query parameters into another map stored in the result of parse(). This would work like so:

var template = new UriTemplate("/resource{?a,b,params*});
var result = new UriParser(template).parse('foo.com/resource?a=1&b=2&c=3');
print(result['params']['c']); // 3

The problem here is that we might want to support the explode operator for grouping multiple values for the same key:

var template = new UriTemplate("/resource{?a,b,params*});
var result = new UriParser(template).parse('foo.com/resource?a=1&b=2&params=3&params=4');
print(result['params']); // ['3', '4']

The one question I have is whether optional parameters need to be in the template. Can they just be retrieved from the original Uri?

[dristic]

Personally I like being more declarative with my URI templates, but retrieving additional parameters from the URI even if they are not defined would also work for me.

[Andersmholmgren]

Thanks @justinfagnani

Unfortunately, the spec only seems to deal with expansion and not matching. For expansion it seems to treat variables as optional

3.2.1.  Variable Expansion

   A variable that is undefined (Section 2.3) has no value and is
   ignored by the expansion process.  If all of the variables in an
   expression are undefined, then the expression's expansion is the
   empty string.

so one could make a tenuous case that the same approach should be applied to matching but then that would also mean matching missing path segments which is not what I want ;-)

You're right in that optional parameters don't "need" to be in the template but I personally really like them there. They help document the API. For example

/computers{?name,make,colour}

expresses the API more clearly than

/computers

just like the dart function

searchComputers(String name, String make, String colour)

expresses it's API much better than

searchComputers(Map maybeThereAreSomeParamsHereMaybeThereArent)

Of course shelf_route could still include the parameters in the uri template and avoid passing them to uri and instead match them itself but I would prefer to avoid that duplication of logic

[justinfagnani]

Yeah, I know the spec doesn't deal with matching, this whole thing is me trying to shoehorn matching in in a reasonable way so that you don't need two sets of URI templates.

I'm not sure what you mean by "shelf_route could still include the parameters in the uri template and avoid passing them to uri and instead match them itself". Do you mean pre-processing the UriTemplate to extract the parameter expressions? I agree that this is logic that shouldn't have to be done by a client of uri.

If an extra argument to match would work, I think we can do that. The problem I see there is that it turns off query parameter matching as a whole - there's no control per parameter.

[Andersmholmgren]

Well I'm glad you did as the matching is at least as valuable as the expansion.

Yes I mean that shelf_route would need to remove the parameters from the UriTemplate it passes to uri which would be sucky

BTW I don't want to turn off matching on parameters but rather have it not fail if one or more of the parameters are not in the uri being matched. I still want the values that do match to be populated in UriMatch.parameters as they currently do.

I realise that it would not be per parameter as the spec defines no way to declare a parameter as optional. That's not a big deal for me as other frameworks downstream from shelf_route like shelf_bind and shelf_rest support constraint annotations (via constrain) such as @ NotNull .e.g

searchComputers(@NotNull() String name, String name, ...)

[Andersmholmgren]

BTW your drone build badge is showing as failing

[justinfagnani]

Picking this up again, I think the best option is to add a parameter to UriPattern.match(), and possibly remove UriParser.parse(). When query parameters are optional, UriMatch can have a field which says whether all of the parameters had matches. Between that and the original URI being available as UriMatch.input users should be able to customize matching with specific optional and required parameters by wrapping.

This approach should be forward compatible if we find a way to make parameters optional/required on an individual basis in the template.

[Andersmholmgren]

That sounds good to me.

Another option to a bool saying if all query params matched you could expose an Iterable of unmatched query params, or of those that did match (depending on your preference).

There is already a Map called parameters that seems to relate to this. Not sure how that would fit

[justinfagnani]

UriPattern could expose the set of parameters it's matching on, then finding unmatched parameters is just a set difference.

[justinfagnani]

One detail is to work out the exact semantics of this new parameter. Does it make all parameters optional, or just query parameters? Path parameters probably shouldn't be optional, but I have seen some schemes where key/value pairs are encoded in paths and fragments. Right now UriPattern and UriMatch do not specify anything regarding different types of parameters.

Thinking this through could lead to another solution: a constructor parameter on UriParser, leaving the strictness of match() up to the implementation and even instance in our case.

[Andersmholmgren]

Sounds gtm

[justinfagnani]

@Andersmholmgren any chance you can do the PR for this?

[Andersmholmgren]

I think if there is just the one bool parameter then it should only apply to query params. If we want to support optional path params (which I don't think there is a major case for) then it needs to be kept separate. Either another bool param or the param needs to be more like an enum

[Andersmholmgren]

haha yeah I had already resigned to doing it myself although I must admit you "picking this up again" had me all excited ;-)

[justinfagnani]

This goes to why a constructor param on UriParser might be better - match() wouldn't say anything about strictness, each UriPattern instance would in it's own way, and their APIs can evolve on their own. I'd like to add a Rails-style pattern for instance, and I'm not even sure they support less-strict matching.

[justinfagnani]

Yeah, sorry about that. I've moved to Polymer full-time, and while I'd like to have time to maintain all my Dart packages, I just don't have enough to do it right. I can certainly help here and there and do reviews though!

[Andersmholmgren]

Actually yes I see what you mean by a ctr to UriParser. I like that. And the result UriMatch object simply provides details of what did match for those that care.

I think that is the cleaner option and will approach it that way. thanks

Ah time. Something we could all do with more of ;-)

[justinfagnani]

Awesome! Let me know if you need any help, I know the guts of matching are a little uglier than I would like. I can always look at a branch.

[Andersmholmgren]

fyi about to start dev on this now

[sethladd]

How's the patch coming along?

[Andersmholmgren]

#23

[JKRhb]

With the merge of #23, I think this issue was resolved, right?