viewDocs #133

jannyHou · 2017-05-12T19:47:13Z

Description

connect to #67

Add a datasource level function called viewDocs which does view query
Add test cases

Please note:

View cannot be created in a loopbackModel's design doc, it would be some ddoc that already exists in user's database.

jannyHou · 2017-05-26T19:05:34Z

CI passed(1st commit), now it's running due to a jsdoc change.

b-admike · 2017-05-26T19:14:31Z

lib/view.js

+   * @param {String} ddocName The design doc name
+   * @param {String} viewName The view name
+   * @param {Object} options The cloudant view filter
+   * @param {Function} [cb] The callback function


Do we need to follow the @callback format here for JSDocs?

b-admike · 2017-05-26T19:15:00Z

lib/view.js

+   * @param {Function} [cb] The callback function
+   */
+  Cloudant.prototype.viewDocs = function(ddocName, viewName, options, cb) {
+    // ds.viewDocs(ddocName, viewName, cb);


intentional comment?

yep :) the following two lines are written to make inputs compatible with the scenario in that comment.

ssh24 · 2017-05-26T19:50:09Z

lib/view.js

+  var parsedUrl = URL.parse(url);
+  if (parsedUrl.path && parsedUrl.path !== '/')
+    return parsedUrl.path.split('/')[1];
+  else return '';


Don't need an else statement here. Just return.

rmg · 2017-05-26T20:26:44Z

lib/view.js

@@ -0,0 +1,56 @@
+// Copyright IBM Corp. 2015,2016. All Rights Reserved.


Should just be 2017, not 2015,2016.

Does the copyright tool make those changes for us, now? We can always run a separate PR to update it.

It does, yes. The tool could also just be run on this branch and fixed as part of this PR ;-)

rmg · 2017-05-26T20:29:43Z

lib/view.js

+  /**
+   * Get data returned by querying a view
+   *
+   * @param {String} ddocName The design doc name


What form is the ddocName expected to be in?

/{db}/_design/{name}

_design/{name} (db is implied)

{name} (db and _design/ is implied)

It is the third one, {name} (db and _design/ is implied), and this is also what the driver function mo.db.viewDocs() expects.

+1
The JSDoc should explain which format is expected.

Fixing the JSDoc comment to answer my question was what I intended.

/** * Gets data at `/{db}/_design/{ddocName}/views/{viewName}` * @param {string} ddocName The name of the design document. * @param {string} viewName The name of the view function attached to the design document. * //etc */

rmg · 2017-05-26T20:36:10Z

test/cloudant.view.test.js

+          "_id": "_design/getModel",
+          "views": {
+            "returnModelInstances": {
+              "map": "function(doc) { if(doc.loopback__model__name) { emit(doc.loopback__model__name, doc); }}"


You might find Function.prototype.toString() helpful here ;-)

I would actually prefer it, to be honest. We should make that a default expectation so that ESLint can protect us.

rmg · 2017-05-26T20:41:50Z

test/cloudant.view.test.js

+          done(err);
+
+          function isLBModelInstance(elem) {
+            elem.value.hasOwnProperty('loopback__model__name').


If the intended use-case for this feature is existing databases with existing design docs, I would advise against using property names like loopback__model__name for a couple reasons:

it might make code readers/reviewers think this is a field added by LoopBack

it might make current/future maintainers think this is an official property name

Adding a property to every document in an existing CouchDB database is a very expensive operation. So expensive it might even be considered impossible by some operators. The only reasonable approach would be to have the user provide the map function and/or property name already being used for this purpose. If that isn't considered upfront, it could lead to assumptions and design decisions that will need to be undone later and may require future breaking changes.

To be more specific, I would use something like test_model_name or similar for the property name so that it is more clear that it is not an official/magic string.

When I wrote the test I didn't think into that deep, I was just thinking of a way to simply insert data (LBModel.create()), it doesn't imply "Adding a property to every document in an existing CouchDB database".
The view could be used for non-loopback data(already exist in database), and also could be used to improve LB model's performance.

Your two concerns are reasonable to me, I will change the field to some name that not misleading.

kjdelisle · 2017-05-26T21:02:53Z

lib/view.js

+   * @param {Function} cb
+   */
+  Cloudant.prototype.viewDocs = function(ddocName, viewName, options, cb) {
+    // ds.viewDocs(ddocName, viewName, cb);


Remove commented code.

@kjdelisle ah, I rephrased the comments, it was confusing.

kjdelisle · 2017-05-26T21:12:54Z

lib/view.js

+  /**
+   * Get data returned by querying a view
+   *
+   * @param {String} ddocName The design doc name


/** * Gets data at `/{db}/_design/{ddocName}/views/{viewName}` * @param {string} ddocName The name of the design document. * @param {string} viewName The name of the view function attached to the design document. * //etc */

kjdelisle · 2017-05-26T21:15:07Z

test/cloudant.view.test.js

+          "_id": "_design/getModel",
+          "views": {
+            "returnModelInstances": {
+              "map": "function(doc) { if(doc.loopback__model__name) { emit(doc.loopback__model__name, doc); }}"


I would actually prefer it, to be honest. We should make that a default expectation so that ESLint can protect us.

kjdelisle · 2017-05-26T21:20:45Z

test/cloudant.view.test.js

+var _ = require('lodash');
+var url = require('url');
+var should = require('should');
+var async = require('async');


Sort library imports, please. :)

jannyHou · 2017-05-29T16:47:33Z

@kjdelisle @rmg Changes applied, PTAL again. Thanks.

rmg

Couple more small tweaks and questions.

rmg · 2017-05-29T16:54:08Z

lib/view.js

+   * ds.viewDocs(model, getModel, {key: 'purchase'}, cb);
+   * ```
+   *
+   * @param {String} ddocName The design doc name


I would add ".. without {db}/_design/ prefix" (assuming that is correct).

@rmg I am confused why a user would think of using part of the url as a design doc name? By reading couchdb/cloudant document, or those nodejs drivers' README.md, I don't see anywhere implies that the design document name would contain the those prefix....

As a long time CouchDB user, I am confused by whether I am expected to provide the whole name or not. Since the actual Couch/Cloudant document name is actually _design/<name> (with the / requiring URL encoding as %2F), it makes sense to me to pass in the full name - unless I have written my own wrapper which adds it.

It is a case where knowing more means things are harder rather than easier.

rmg · 2017-05-29T16:56:01Z

test/cloudant.view.test.js

+  var samples = [
+    {
+      model: 'purchase',
+      'customer_id': '1',


The customer_id key shouldn't need quoting. Does eslint think it does?

nope :p changed it to Number.

Actually I meant the key it self.

rmg · 2017-05-29T17:04:50Z

lib/view.js

+
+    var self = this;
+    var db = this.cloudant.use(self.getDbName(self));
+    assert(db && typeof db.view === 'function',


This isn't checking the design doc or the datasource. this.cloudant.use is just returning a scoped object so that methods on it use URLs based on the db name. If db.view is ever not a function then it would mean that someone has changed the implementation of this connector to no longer use cloudant/cloudant-nano/nano as the client.

Is that the purpose of this assertion?

I thought if the dynamically loaded database doesn't exist, then this.cloudant.use('dbName') would return undefined, but I just checked it throws error saying db doesn't exist.
Ok I will remove that assert.

That sort of dynamic check would require network I/O, which would require .use() to be async and take a callback ;-)

jannyHou · 2017-05-29T18:50:06Z

Thanks @rmg I applied the other two comments, and explained my confusion of the "design doc name" in #133 (comment), I am ok to add the prefix explanation in the description if you insist, am just curious why it's that important, even after having an example usage :)

rmg · 2017-05-29T21:28:51Z

Paraphrasing my reply from the line-comment since I think it is important enough to not hide:
The actual name of the document, which is the "foo" design document is _design/foo. This is a normal Couch/Cloudant/JSON document, but the name includes the prefix _design/ which CouchDB/Cloudant uses as a filter for documents which contain views, lists, etc. Since / is part of the actual document's name, it is sometimes necessary to URL encode it so that it doesn't get treated as a path separator.

Yes, this is a little weird, and yes, it is sort of like having a file on disk with / in the name.

jannyHou · 2017-05-30T14:59:11Z

@rmg Thank you for elaborating the reason, by

the actual Couch/Cloudant document name is actually _design/ (with the / requiring URL encoding as %2F

you are right 👍
And I double checked, GET /db/_design/<design-doc>/_view/<view-name> is the correct endpoint.
Changes applied. PTAL again. Thanks!

rmg · 2017-05-30T16:12:16Z

test/cloudant.view.test.js

+  var samples = [
+    {
+      model: 'purchase',
+      'customer_id': 1,


var samples = [ { model: 'purchase', customer_id: 1, // <------ customer_id, not 'customer_id' basket: ['food', 'drink'], }, };

rmg

One minor nit, but it's not super important. You do have to rebase, though, so you may as well fix it before doing that.

ssh24

LGTM

b-admike · 2017-05-30T17:58:16Z

README.md

+    // NOT `ds.viewDocs()`
+    // 2. This api matches the Cloudant endpoint:
+    // GET /db/_design/<design-doc>/_view/<view-name>
+    ds.connector.viewDocs('design_doc', 'view_name', cb);


maybe expand cb to be inline to show what we expect to see in the callback function like function (err, views)?

jannyHou · 2017-05-30T18:08:24Z

@b-admike Changes applied. PTAL again, thanks.

b-admike

LGTM. Nice way to write the tests!

* Inherit couchdb functionalities (#163) (Sakib Hasan) * Add stalebot configuration (Kevin Delisle) * Recover & reuse couchdb2 tests (jannyHou) * Fix readme (ssh24) * Create Issue and PR Templates (#171) (Sakib Hasan) * Add CODEOWNER file (Diana Lau) * Recover manipulation.test.js (#159) (Janny) * Recover juggler tests (#158) (Janny) * Require init on mocha args (ssh24) * Do not strip _rev value on create (ssh24) * Fix docs on bulk replace op hooks (ssh24) * Fix update/updateAll function (ssh24) * Add cloudant specific bulkReplace function (ssh24) * Check error and result (#149) (Janny) * Fix updateAttributes function (ssh24) * Fix doc (#148) (Janny) * viewDocs (#133) (Janny) * Return back result count in updateAll (ssh24) * Fix database name typo on README (ssh24) * Add regexp doc (#143) (Janny) * Add proxy config test (#142) (Janny) * Allow users to spawn docker and run tests (ssh24) * test: use Cloudant 2.x based image for testing (Ryan Graham) * test: replace setup.sh with test.js (Ryan Graham) * Refactor functions in cloudant (ssh24) * Allow handling of ._rev on models (#123) (Kevin Delisle) * Allow travis to run against the latest code base (#138) (Sakib Hasan) * Add docker setup (#132) (Sakib Hasan) * Fix updateOrCreate (#136) (Sakib Hasan) * Fix typo (#135) (Janny) * cloudant.test: cleanup after test runs (Kevin Delisle) * Setup Travis with Docker Compose (Kevin Delisle) * Refactor doc (#116) (Janny) * reinstate bulk update (biniam) * add array prop update tests (biniam) * update docs with current revision (biniam) * Allow id property to be a number (#115) (Sakib Hasan) * autoupdate and automigrate fix (#109) (Janny) * update readme to doc async connect (biniam) * check cloudant db in config (biniam) * call driver asynchronously (biniam) * Fix sort query builder (#107) (Janny) * Recover maxrows.test.js (#91) (Janny) * Fix regexp.test.js (#103) (Janny) * Use define function in loopback-connector (jannyHou) * add url config example (biniam) * Update connector to 4.0.0 (ssh24) * Add doc for fitler and order (jannyHou) * Add advisory note regarding update (ssh24) * Add $elemMatch for array (jannyHou) * Revert "Build selector with array type data" (jannyHou)

jannyHou added #review needs-priority and removed #review labels May 12, 2017

jannyHou changed the title ~~viewDoc~~ [DON'T MERGE, STILL IN PROGRESS]viewDoc May 12, 2017

jannyHou changed the title ~~[DON'T MERGE, STILL IN PROGRESS]viewDoc~~ [DON'T MERGE, STILL IN PROGRESS]viewDocs May 12, 2017

jannyHou force-pushed the feature/view branch 2 times, most recently from fe41fa6 to 0ad4a2d Compare May 26, 2017 18:52

jannyHou changed the title ~~[DON'T MERGE, STILL IN PROGRESS]viewDocs~~ viewDocs May 26, 2017

b-admike reviewed May 26, 2017

View reviewed changes

ssh24 reviewed May 26, 2017

View reviewed changes

jannyHou force-pushed the feature/view branch from 8377e2d to acd34db Compare May 26, 2017 20:20

jannyHou self-assigned this May 26, 2017

rmg suggested changes May 26, 2017

View reviewed changes

kjdelisle suggested changes May 26, 2017

View reviewed changes

jannyHou force-pushed the feature/view branch 2 times, most recently from 1bd80a1 to 1d1507f Compare May 29, 2017 16:44

rmg reviewed May 29, 2017

View reviewed changes

jannyHou force-pushed the feature/view branch from b6a9a8d to 7ba32a8 Compare May 30, 2017 14:51

rmg reviewed May 30, 2017

View reviewed changes

rmg approved these changes May 30, 2017

View reviewed changes

jannyHou force-pushed the feature/view branch from 4248afa to b6dc343 Compare May 30, 2017 17:08

ssh24 approved these changes May 30, 2017

View reviewed changes

b-admike reviewed May 30, 2017

View reviewed changes

b-admike approved these changes May 30, 2017

View reviewed changes

jannyHou force-pushed the feature/view branch 2 times, most recently from 8c1a5e2 to fd0ccc3 Compare May 30, 2017 22:12

jannyHou added 3 commits May 30, 2017 20:55

viewDoc

f6ad499

feedback

606b8dd

Add doc

152b7c1

jannyHou force-pushed the feature/view branch from fd0ccc3 to 152b7c1 Compare May 31, 2017 00:55

jannyHou merged commit 20e909e into master May 31, 2017

jannyHou removed the needs-priority label May 31, 2017

jannyHou added this to the Sprint 37 - Apex milestone May 31, 2017

jannyHou added the apex label May 31, 2017

jannyHou deleted the feature/view branch June 8, 2017 17:29

		@@ -0,0 +1,56 @@
		// Copyright IBM Corp. 2015,2016. All Rights Reserved.

viewDocs #133

viewDocs #133

Conversation

jannyHou commented May 12, 2017 • edited Loading

Description

jannyHou commented May 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou May 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou commented May 29, 2017

rmg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou commented May 29, 2017 • edited Loading

rmg commented May 29, 2017

jannyHou commented May 30, 2017

Choose a reason for hiding this comment

rmg left a comment

Choose a reason for hiding this comment

ssh24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou commented May 30, 2017

b-admike left a comment

Choose a reason for hiding this comment

jannyHou commented May 12, 2017 •

edited

Loading

jannyHou May 26, 2017 •

edited

Loading

jannyHou commented May 29, 2017 •

edited

Loading