Implement Service & ServiceObject classes #904

stephenplusplus · 2015-10-01T19:21:29Z

This PR is going to be split into pieces

Core Service & ServiceObject: #928
BigQuery: #931
Compute: #932
Datastore: #897
DNS: #942
Resource: #943
Pub/Sub: #944
Search: #945
Storage: #946

Fixes #847
Fixes #862
Fixes #914

This will be a big PR, and is still a work in progress. @callmehiphop -- please review at your earliest convenience!

To Dos

Goal

Add create, get (with autoCreate behavior), and exists methods to all service objects.

Implementation

Distinctly define the types of classes we have:

Service classes (Datastore, Storage, Compute, ...)
Service Object classes (Dataset, Bucket, VM, ...)

Define common methods for each:

Service classes:
1. request => make an authenticated request
Service Object classes:
1. create => create this object
2. exists => does this object exist, returns true or false
3. get => get the metadata for this object -- pass {autoCreate:true} to create the object if it doesn't exist
4. delete => delete this object
5. getMetadata => get the metadata for this object
6. setMetadata => set the metadata for this object
7. request => make an authenticated request

We've been re-writing these methods all over our library, which has led to some unwanted side effects (inconsistency / repeated code / repeated tests). This separation will allow us to write once, run everywhere.

The Problems

Not every service object can utilize all of these methods. For example, a Compute Engine Zone can't be created. You can't set the metadata on a Compute Engine Snapshot. You can't delete a Compute Engine Region. This requires configuration so that the unwanted methods are blanked out.
Documentation. What we want is to inherit a bunch of methods and be done with it, but we still need to document them matching the context of the service object. We don't want the user to see some generic "To delete this object, call object.delete(). And we have to uniquely document create, since each method will take different parameters.

lib/bigquery/dataset.js

-  this.makeReq_('DELETE', '', query, null, callback);
+  this.request({
+    method: 'DELETE',
+    uri: '',


stephenplusplus · 2015-10-22T17:39:39Z

@callmehiphop can you take another quick glance over the changes I've made (latest commit)? I think I may have come up with an easier way to handle the inherits/excludes/borrows mess. Now, we pass an object listing the methods we want to inherit explicitly: methods. It's just a map of methodName: true, but the best part is that we can document these properties instead of overwriting the methds we want to extend, only to call them by ServiceObject.prototype.method... again.

We don't override methods just to call them again only for the docs to be built properly. I cheated around that by just documenting aobject that is used when we extend from ServiceObject.

Also, the docs are much simpler to avoid all of the replication. The create, get, and getOrCreate docs are basically shells that don't show anything more than basic usage that will always be true, even if the upstream API arguments change (like zone#createVM's config).

callmehiphop · 2015-10-22T18:48:25Z

I like this approach much more than the previous.

However it still kinda feels odd to me, mainly because I don't think we should be adding code for the sake of documentation, even if it's just a little. I think I've suggested this before, but have we considered moving away from dox? JSDoc3 actually has pretty nice support for spitting out plain old JSON (minus custom tags). It also allows you to document "virtual" members, which is just code that exists elsewhere. (e.g. getOrCreate).

I realize this would be a large undertaking, so this is obviously not the time or place for a change like that, but it's still something I think we should consider. Aside from that, I think it looks good!

stephenplusplus · 2015-10-22T18:54:40Z

mainly because I don't think we should be adding code for the sake of documentation, even if it's just a little

Which code does that?

stephenplusplus · 2015-10-22T18:59:21Z

I think I've suggested this before, but have we considered moving away from dox? JSDoc3 actually has pretty nice support for spitting out plain old JSON (minus custom tags).

I'd be in support of this, but given the scope of the work involved, it's a pretty intimidating task to take on. Maybe switching over should happen when we get to the gcloud-* common docs task.

callmehiphop · 2015-10-22T18:59:28Z

Maybe I misunderstood, but I thought the main reason to switch to an object containing method names flagged as true was for documentation purposes?

callmehiphop · 2015-10-22T18:59:51Z

I'd be in support of this, but given the scope of the work involved, it's a pretty intimidating task to take on. Maybe switching over should happen when we get to the gcloud-* common docs task.

Yeah totally!

stephenplusplus · 2015-10-22T19:00:06Z

It does work for that purpose, but it's also to communicate to ServiceObject what methods we're going to inherit.

callmehiphop · 2015-10-22T19:10:50Z

It does work for that purpose, but it's also to communicate to ServiceObject what methods we're going to inherit.

Gotcha! Do you think it would be better to make a custom inherit function to do that? It might be slightly more efficient to tie ServiceObject methods to an api's prototype chain

inherit(BigQuery, ServiceObject, {
  getOrCreate: null
});

Either way, I approve!

stephenplusplus · 2015-10-22T19:17:37Z

RE: the getOrCreate: null, I prefer the whitelist over the blacklist because:

each class has to explicitly ask for what methods they want
having all of the methods listed that you want to inherit is a logical place to put the documentation

There definitely might be a better way to do this though, so I'll think about it as I go on. Something worth noting is in some cases, we don't know if a class needs a method or not until it's instantiated.

callmehiphop · 2015-10-22T19:21:00Z

Something worth noting is in some cases, we don't know if a class needs a method or not until it's instantiated.

Why is that?

lib/compute/snapshot.js

-  this.name = name;
-  this.metadata = {};
+function Snapshot(scope, name) {
+  var isDisk = scope.constructor.name === 'Disk';


stephenplusplus · 2015-10-23T18:42:50Z

@callmehiphop feel free to take a look at the new implementation of exists and get with autoCreate: true. getOrCreate has been removed. I think the pattern I'll be following re: docs, inheritance, ServiceObject is pretty much on lock now. I'll just be carrying over the code to the libraries that haven't been implemented yet.

Then... testing. Dun, dun, dun.

lib/common/service-object.js

+
+  // The `request` method may have been overridden to hold any special behavior.
+  // Ensure we call the original `request` method.
+  ServiceObject.prototype.request.call(this, reqOpts, function(err, resp) {


callmehiphop · 2015-10-24T11:34:56Z

getOrCreate has been removed

yay! looks good

callmehiphop · 2015-11-06T05:20:10Z

This is getting pretty huge, GitHub can't even display the entire diff because of the size. Any chance we could break any of this up?

stephenplusplus · 2015-11-06T21:22:13Z

Yes, very good idea. I'll start with Service & ServiceObject.

stephenplusplus · 2015-11-09T17:05:06Z

@callmehiphop: #928

stephenplusplus added core don't merge labels Oct 1, 2015

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 1, 2015

stephenplusplus changed the title ~~Spp get or create~~ Implement Service & ServiceObject classes Oct 1, 2015

callmehiphop reviewed Oct 5, 2015
View reviewed changes

lib/bigquery/dataset.js

this.makeReq_('DELETE', '', query, null, callback);

this.request({

method: 'DELETE',

uri: '',

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

stephenplusplus mentioned this pull request Oct 17, 2015

exists method for storage/bucket#file ? #914

Closed

stephenplusplus force-pushed the spp--getOrCreate branch from fb15704 to 9a752c2 Compare October 22, 2015 17:35

stephenplusplus reviewed Oct 22, 2015
View reviewed changes

lib/compute/snapshot.js

this.name = name;

this.metadata = {};

function Snapshot(scope, name) {

var isDisk = scope.constructor.name === 'Disk';

This comment was marked as spam.

Sign in to view

callmehiphop reviewed Oct 24, 2015
View reviewed changes

stephenplusplus force-pushed the spp--getOrCreate branch 3 times, most recently from f15c67e to 59570b9 Compare October 30, 2015 16:18

stephenplusplus added 10 commits November 2, 2015 10:25

pubsub.index

4f285ff

resource.index

f05bfc4

resource.project

856248b

search.document

72d6c39

search.index-class

f821717

search.index

92c7c8f

storage.acl

f96590a

storage.bucket

3d8bdb6

storage.file

1b87f65

storage.index

f12965f

callmehiphop mentioned this pull request Nov 3, 2015

Why don't we use promises in our API? #551

Closed

stephenplusplus added 5 commits November 3, 2015 17:06

common.serviceobject

85331d2

common.service

4f0d3b1

fix examples

70151fb

docs + change.create

fe2a9a5

doc clean up

01ce610

stephenplusplus mentioned this pull request Nov 9, 2015

Implement Service & ServiceObject classes #928

Merged

stephenplusplus closed this Nov 17, 2015

This was referenced Nov 17, 2015

Clear up makeRequest_ naming conventions #847

Closed

Switch to verbObject() methods to .object().verb()? #862

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Service & ServiceObject classes #904

Implement Service & ServiceObject classes #904

stephenplusplus commented Oct 1, 2015

This comment was marked as spam.

This comment was marked as spam.

stephenplusplus commented Oct 22, 2015

callmehiphop commented Oct 22, 2015

stephenplusplus commented Oct 22, 2015

stephenplusplus commented Oct 22, 2015

callmehiphop commented Oct 22, 2015

callmehiphop commented Oct 22, 2015

stephenplusplus commented Oct 22, 2015

callmehiphop commented Oct 22, 2015

stephenplusplus commented Oct 22, 2015

callmehiphop commented Oct 22, 2015

This comment was marked as spam.

stephenplusplus commented Oct 23, 2015

This comment was marked as spam.

This comment was marked as spam.

callmehiphop commented Oct 24, 2015

callmehiphop commented Nov 6, 2015

stephenplusplus commented Nov 6, 2015

stephenplusplus commented Nov 9, 2015

Implement Service & ServiceObject classes #904

Implement Service & ServiceObject classes #904

Conversation

stephenplusplus commented Oct 1, 2015

This PR is going to be split into pieces

To Dos

Goal

Implementation

The Problems

This comment was marked as spam.

This comment was marked as spam.

stephenplusplus commented Oct 22, 2015

callmehiphop commented Oct 22, 2015

stephenplusplus commented Oct 22, 2015

stephenplusplus commented Oct 22, 2015

callmehiphop commented Oct 22, 2015

callmehiphop commented Oct 22, 2015

stephenplusplus commented Oct 22, 2015

callmehiphop commented Oct 22, 2015

stephenplusplus commented Oct 22, 2015

callmehiphop commented Oct 22, 2015

This comment was marked as spam.

stephenplusplus commented Oct 23, 2015

This comment was marked as spam.

This comment was marked as spam.

callmehiphop commented Oct 24, 2015

callmehiphop commented Nov 6, 2015

stephenplusplus commented Nov 6, 2015

stephenplusplus commented Nov 9, 2015