solidusjs / solidus

A simple server that generates pages from JSON and Templates
MIT License
28 stars 7 forks source link

Optional querystring param? #41

Closed rxaviers closed 11 years ago

rxaviers commented 11 years ago

How shall I declare an optional querystring parameter? For example, consider the following resource:

{{!
{
    "resources": {
        "facebook": "http://423-proxy.sites.storytellerhq.com/.json?resource=facebook&q=keithurban&limit={limit}&until={until}"
    }
}
}}

On cursoring pagination, until is absent on 1st page. But, it gets set to some value on further pages.

The problem with the above is because I can't make until={until} optional. It always gets set, wrongly, even with until undefined.

pushred commented 11 years ago

What is sent when until is undefined? Is it the template tag {until} being sent?

rxaviers commented 11 years ago

Yeap, it's sending until={until}

rxaviers commented 11 years ago

Hi @Fauntleroy thanks for working on this. Although, I've noticed it still does produce an undesired url.

Take as example, the same one of the description. If you request that page, passing limit=15 ONLY (in the querystring):

Before 2ee6d78, you get http://423-proxy.sites.storytellerhq.com/.json?resource=facebook&q=keithurban&limit=15&until={until}

After 2ee6d78, you get http://423-proxy.sites.storytellerhq.com/.json?resource=facebook&q=keithurban&limit=15&until=

The until= leftover is still undesired. We need a solution to allow optional querystring params.

rxaviers commented 11 years ago

I would like you to consider reopening this Issue.

rxaviers commented 11 years ago

I would like you to also consider adding unit tests for fixed Issues. FWIW, in jQuery, we tend to always close an Issue only if it's provided a fix, and a unit test that checks such fix. This is extremely important for code quality. It's a viable way to prevent the same Issue to happen again.

Fauntleroy commented 11 years ago

This is the test for the issue. https://github.com/SparkartGroupInc/solidus/commit/d036abbb7e4aab67377218a5e30aee8af895bd1b

Fauntleroy commented 11 years ago

@rxaviers I'm also not sure why the parameter itself needs to be removed. If a parameter is passed with no data, it should come up as undefined on the API's end, essentially doing nothing.

rxaviers commented 11 years ago

Nice job by adding the test case. But, note the testing resources you added don't use querystring as part of it, eg. ?foo={foo}.

rxaviers commented 11 years ago

@rxaviers I'm also not sure why the parameter itself needs to be removed. If a parameter is passed with no data, it should come up as undefined on the API's end, essentially doing nothing.

That's not what I am saying. I am saying I need a way to reference an optional querystring. In the example I gave, I want to include until in the resource's querystring only if it is defined. But, don't include it if it's not defined.

buggy:

http://423-proxy.sites.storytellerhq.com/.json?resource=facebook&q=keithurban&limit=15&until=

vs

http://423-proxy.sites.storytellerhq.com/.json?resource=facebook&q=keithurban&limit=15

See what I am saying?

Fauntleroy commented 11 years ago

If http://423-proxy.sites.storytellerhq.com/.json?resource=facebook&q=keithurban&limit=15&until= and http://423-proxy.sites.storytellerhq.com/.json?resource=facebook&q=keithurban&limit=15 return the same data, does it matter if we completely remove the parameter?

It would be "nice" to have the URL parameter not appear at all, but this seems to work fine. Since removing the parameter entirely would be much more complex than just leaving the parameter blank, I see no reason to do anything further at this time.

rxaviers commented 11 years ago

Twitter asks to omit parameters on its cursoring pagination doc "To use max_id correctly, an application's first request to a timeline endpoint should only specify a count".

Google also requires to omit parameters on its pagination section "If you've held a pageToken for a long period of time and want to continue paging, it may be better to restart pagination by repeating the original request (omitting pageToken)."

I'm afraid it will fail for other services as well.

Isn't Solidus' main goal to proxy user-application requests? If so, I think it is important that Solidus does not modify (or restrict) the actual request. Anyway, this shouldn't need to be complex. If it is, I suggest changing current architecture (for example, as suggested in #43).

rxaviers commented 11 years ago

This needs to be reopened.

Fauntleroy commented 11 years ago

@rxaviers I've tested this with both the Twitter and Google APIs you mentioned and I've had no problem passing null values for these parameters. Since sending empty parameters does not actually break anything, I do not consider this to be an issue, therefore there's no reason to reopen it. If we do run into a case where passing ?parameter= instead of nothing does cause an issue I'll reopen this in an instant, but for now that's not the case.

We already have an issue for discussing more precise ways to pass parameters along to resources, as well: https://github.com/SparkartGroupInc/solidus/issues/43