Refactor Transaction and Dataset into DatastoreRequest

[ryanseys]

Might be missing documentation in a few areas because I wanted to get the structural changes out first before we refine it.

Separates the request logic out from Dataset and Transaction into a DatastoreRequester which probably isn't the best name but can be refactored as well. Feel free to send suggestions.

[ryanseys]

regression test should run in a transaction is failing right now 👎 I also have to head out for the weekend, but I might get some time to look into it. In the meantime if someone can take a look at this, that'd be great 😄 Just do not merge!!

[stephenplusplus]

Just linking these together for us: #204

Thoughts so far:

• Transaction will either have to overwrite createRequest, or DatastoreRequester.createRequest will have to allow more customization. Thinking ahead for GAE dev support, we will have to set a port & host, and perhaps handle other things within that function just a little differently.
• Should Dataset.prototype.allocateIds be on the DatastoreRequester object?

Overall, I like the separation. We should make sure this structure is 👍 so that we can mimic it throughout the codebase as we go on supporting the other services.

To support that goal, is there a way we could make an even more abstract ApiRequest class that could handle some common things, like ApiRequest.prototype.getRemoteStream, which would take a request and callback that gets executed on end:

varremoteStream=https.request(request,function(resp){varbuffer=newBuffer('');resp.on('data',function(chunk){buffer=Buffer.concat([buffer,chunk]);});resp.on('end',function(){util.handleResp(null,resp,buffer.toString(),function(err){if(err){cb(err);return;}cb(null,respType.decode(buffer));});});});remoteStream.on('error',cb);remoteStream.write(req.toBuffer());remoteStream.end();

Something like:

| ApiRequest.prototype.createRequest
| ApiRequest.prototype.getRemoteStream
|
|__ DatastoreRequest.prototype.allocateIds
|__ DatastoreRequest.prototype.delete
|__ DatastoreRequest.prototype.get
|__ DatastoreRequest.prototype.runQuery
|__ DatastoreRequest.prototype.save
|
|__ Transaction.prototype.begin
|__ Transaction.prototype.commit
|__ Transaction.prototype.finalize
|__ Transaction.prototype.rollback
|
|__ Bucket.prototype.copy
|__ Bucket.prototype.list
|__ Bucket.prototype.remove
|__ Bucket.prototype.stat

Any of these sub-classes can implement their own createRequest if necessary.

[ryanseys]

Yes that would be ideal, to separate into a common module that can be used across all APIs. DatastoreRequester is a terrible name. Maybe all of DatastoreRequester can go into Datastore once we move the request logic out into ApiRequest

[ryanseys]

Having any class have to create a new createRequest is basically necessary here because we are doing different stuff with the results for every different API. That's why I'm hesistant to extract it further than Datastore because I don't think much can be gained from it.

[stephenplusplus]

There is still a benefit to abstracting some commonalities, such as creating streams, handling errors/responses, creating authenticated requests, etc. but it's ok to put this off for now. If we think we need to abstract later, we can revisit that then.

Are there any changes you want to make to this pr or is it ready for review?

[ryanseys]

Yes. I agree. It's important to pull out the streaming stuff and some common entities but it's beyond the scope of this PR.

[ryanseys]

I changed DatastoreRequester to DatastoreRequest and fixed small transaction bug so I think this is ready for more formal review. 😄 thanks!

[stephenplusplus]

Should Dataset.prototype.allocateIds be on the DatastoreRequest object?

[stephenplusplus]

I like it, but I think we're going to have a tough time working this into the docs. Atm, the only thing I can think of is having a new docs page for the DatastoreRequest module that is linked to from Dataset.

[stephenplusplus]

How about request.js instead of datastore_request.js? We have the datastore directory name that can keep it hierarchically clear. (we don't have datastore_transaction.js, datastore_dataset.js for example)

[ryanseys]

sure just as long as we don't confuse it with request library

[stephenplusplus]

We can still name it in a way that's helpful locally, if you think something like this is better:

varDatastoreRequest=require('./request.js');