Break buildURL into multiple requestType methods

[jamiebuilds]

This provides separate abstract methods that can be used for different request types with better signatures. It also passes the query in from findQuery.

I still need to write tests for this, but I'm putting it up before I go to sleep.

PS: @wycats I still want to be able to pass query to findAll but I wasn't able to find a good way of doing that.

[fivetanley]

@bmac can you add this to the friday team discussion?

[tomdale]

Is this related to the PR in question?

[amiel]

Thanks for passing query along to buildURL; I will find that useful!

I'm wondering, though, why is it passed as the id parameter and would it make more sense as the snapshot parameter? I know it's not really a snapshot, but it seems more like a snapshot than an id. Before this change, id is either null, a string, or an array of strings. With this change it can also be an object. snapshot on the other hand, is either null, an object, or an array of objects. Also, I think the query is used similarly as the snapshot (to fill arbitrary values in the url).

For reference, here is the signature of buildURL:

buildURL: function(type, id, snapshot, requestType) {

[jamiebuilds]

That would be a breaking change for anyone currently overriding buildURL that is expecting it to be an actual snapshot. And I would say that the breaking change should be adding a formal query argument so that the signature would be:

buildURL: function(type, id, query, snapshot, requestType) {

[amiel]

ok
I just thought I would check. I don't understand how putting the using query instead of null would break anything that using query instead of id would, but it sounds like you've thought about this more in depth than I have.

Anyway, I'm just happy to have the query available in buildURL at all. Thanks!

[wycats]

@thejameskyle does this satisfy your requirements or do you still need more stuff here?

[funtusov]

I understand the benefits for when you want to override particular queries, but what is the reasoning for making the actual url construction (_buildURL) a private api?

Here's a usecase I had:
I was implementing nested models for the following api structure:
GET|POST|PUT /posts/:post_id/comments/...
by adding one line to buildUrl:

...
if (parentKey) { url.push(this.pathForParent(record, parentKey)); }``
...

To avoid using the private _buildURL, I only see adding the same custom code to 10 different methods?

[igorT]

Could you override buildUrl and call super?

[funtusov]

My override inserts a segment somewhere in the middle of the generated url, so while theoretically it could be possible to insert the needed part within this._super() with some strings work, I imagine that code would be convoluted and brittle.

The new buildUrl branches to 10 different methods based on request type (urlForFind, urlForFindAll, urlForFindQuery), and each of those methods by default is a proxy to the (now private) _buildUrl – making it public (constructUrl?) would solve that.

[jamiebuilds]

I don't see how that would improve anything. Calling super here should behave almost exactly the way it did previously for your case.

[funtusov]

@thejameskyle are you referring to calling super in the new buildUrl method (that only dispatches the url construction to _buildUrl) or in the private _buildUrl (that does url construction)?