Prefix support

[andrew-aladev]

I think we should try to strip prefix from path if defined. For example: '/db/dbname/_local/thali_id' -> '/dbname/_local/thali_id'.

---

This change is

[coveralls]

Coverage remained the same at 100.0% when pulling 0bd49c4 on vNext_aaladev_4 into 8a5721b on master.

[andrew-aladev]

Passing 'req' to thaliId callback is very usefull.

[coveralls]

Coverage decreased (-2.5%) to 97.541% when pulling 11af91e on vNext_aaladev_4 into 8a5721b on master.

[coveralls]

Coverage remained the same at 100.0% when pulling 6adee32 on vNext_aaladev_4 into 8a5721b on master.

[yaronyg]

Reviewed 1 of 2 files at r1, 2 of 3 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks broke.

---

test/test-prefix.js, line 3 [r3] (raw file):

'use strict';

var request = require('supertest'),

NB: We typically just have each var on its own line. It makes it much faster to edit and move things around.

---

test/test-prefix.js, line 5 [r3] (raw file):

var request = require('supertest'),
  express = require('express'),
  fspath = require('path'),

NB: We typically say 'path' not 'fspath'. It's just traditional.

---

test/test-prefix.js, line 9 [r3] (raw file):

  assert = require('assert');

var lib = require(fspath.join(__dirname, '../lib/index'));

NB: You shouldn't need to use path.join here. You can just up '../lib/index' on its own. It should always work even in Android/iOS.

---

test/test-prefix.js, line 12 [r3] (raw file):

function genericHandlers(router) {
  var handlers = require('./handlers2');

NB: Why isn't this just a generic requires at the top?

---

test/test-prefix.js, line 55 [r3] (raw file):

        .expect(200, done);
    });
    it('/fit/ should be 200', function(done) {

The whole point of prefix was to deal with the situation where the root of the CouchDB server (in this case Express-PouchDB) isn't at '/'. But we don't know where the root will be until runtime. So we use prefix as a way to specify where that location will be. In the case of Thali the prefix is defined here but that can be changed.

So once we set the prefix it is in stone. Any path that is 'before' the prefix isn't part of the ACL and MUST NOT be honored. So in the case of this code once a prefix is set then a request for / MUST be rejected because we only honor requests with the ACL that start with the prefix (in this case, fit). So / is rejected but /fit is fine. /fit should translate to / in terms of the ACL. This also means that /fit/foo is fine (that is treated as /foo) but /foo MUST be rejected because it doesn't start with the prefix and therefore isn't part of the ACL.

---

Comments from Reviewable

[coveralls]

Coverage remained the same at 100.0% when pulling fcc539d on vNext_aaladev_4 into 8a5721b on master.

[coveralls]

Coverage remained the same at 100.0% when pulling 8ededff on vNext_aaladev_4 into 8a5721b on master.

[coveralls]

Coverage decreased (-2.4%) to 97.6% when pulling 2e12d49 on vNext_aaladev_4 into 8a5721b on master.

[coveralls]

Coverage remained the same at 100.0% when pulling 2e12d49 on vNext_aaladev_4 into 8a5721b on master.

[coveralls]

Coverage remained the same at 100.0% when pulling edbd36a on vNext_aaladev_4 into 8a5721b on master.

[andrew-aladev]

I've made many cosmetics in tests in order to be valid with jscs and jshint styles.

[yaronyg]

Reviewed 14 of 16 files at r5, 2 of 2 files at r7.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

lib/index.js, line 102 [r7] (raw file):

    var path = req.path;
    if (req.connection.prefix) {

But who would set prefix on the connection object? If we are going to support prefix don't we need to set it on the ACL object and read that in as part of the ACL definition? Or better yet, maybe we just get rid of prefix all together and instead create a way to take a generic ACL and change it's paths to have the prefix or maybe even better than that maybe we forget about prefix all together and just edit Thali's replication ACL to include the prefix directly and call it a day?

---

test/test-prefix.js, line 42 [r7] (raw file):

      router.all('*', function (req, res, next) {
        req.connection.pskRole = 'user';
        req.connection.prefix  = '/prefix';

Didn't you explain on today's call why this won't work? It would require all paths, including the beacon paths, to be prefixed. But we only want to prefix the PouchDB paths. See my previous comment for alternative approaches that could work.

---

Comments from Reviewable

[yaronyg]

Please make it a point to raise this PR on the stack for tomorrow's call. I want us to discuss this and reach a concrete agreement on what to do. Thanks!

---

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

Comments from Reviewable

[andrew-aladev]

Yes, this is a problem. I've just commited this workaround to salti and used it in cordova plugin (this branch as a dependency) in order to finish thali manager.

The problem is that some pathes can be used with dbPrefix. For example:

1. /NotificationBeacons for user beacon
2. /{:dbPrefix}/{:dbName}/_changes for user 'replication'

Salti was done by using path.split('/') and parts.length === 3 && pathParts[1] === 'something'.
The easiest way to implement prefix is just to strip prefix before processing. But in this case both /{:dbPrefix}/{:dbName}/_changes and /{:dbName}/_changes will be valid. This is not a solution.

So if dbPrefix was defined - it should be required.

The only reason to pass dbName to salti is to define thali prefix: '/{:dbName}/_local/thali_' and provide a custom callback to validate thali id. So dbPrefix should be passed to salti for the same purpose. It will define thali prefix as '/{:dbPrefix}/{:dbName}/_local/thali_'.

So {:db} can be resolved as {:dbPrefix}/{:dbName} or {:dbName} if dbPrefix is undefined.

[coveralls]

Coverage remained the same at 100.0% when pulling 54d8844 on vNext_aaladev_4 into 8a5721b on master.

[coveralls]

Coverage remained the same at 100.0% when pulling 536edc5 on vNext_aaladev_4 into 8a5721b on master.