Postgre #80

Merged
merged 9 commits into from Jul 9, 2014

Conversation

Projects
None yet
3 participants
Contributor

dlaxar commented Jun 16, 2013

Adding Postgre support for CardDAV

Owner

mikedeboer commented Jun 17, 2013

WOW! That looks awesome, even includes tests... <3

I'll check it out as soon as possible, when I have time to feature test it. One nit I noticed in the diff is that the files don't contain a newline at the end... you might want to fix that 😄

Contributor

dlaxar commented Jun 18, 2013

Glad you like it. I fixed the newlines :)

pygy commented Aug 5, 2013

FWIW, the name is either PostgreSQL [[post-GRESS-Q-L]] or Postgres [[post-GRESS]]. The former being the the official name.

PostgreSQL is the next installment of Ingres.

You're not the only one committing the error... Even some of the PostgreSQL authors regret the name choice, because of the hard pronounciation and the non-obvious abbreviation.

@mikedeboer mikedeboer commented on an outdated diff Aug 16, 2013

lib/DAVACL/backends/postgre.js
+
+var jsDAVACL_iBackend = require("./../interfaces/iBackend");
+
+var Db = require("./../../shared/backends/postgre");
+var Exc = require("./../../shared/exceptions");
+var Util = require("./../../shared/util");
+
+var Async = require("asyncjs");
+
+/**
+ * Postgre principal backend
+ *
+ * This backend assumes all principals are in a single table. The default table
+ * is the 'users' table, but this can be overriden.
+ */
+var jsDAVACL_Backend_Mongo = module.exports = jsDAVACL_iBackend.extend({
@mikedeboer

mikedeboer Aug 16, 2013

Owner

whoops you forgot rename this one!

@mikedeboer mikedeboer commented on an outdated diff Aug 16, 2013

lib/DAVACL/backends/postgre.js
+ * The reason we don't straight-up use that property, is because
+ * me-card is defined as a property on the users' addressbook
+ * collection.
+ */
+ "{http://ajax.org/2005/aml}vcard-url": "vcardurl",
+
+ /**
+ * This is the users' primary email-address.
+ */
+ "{http://ajax.org/2005/aml}email-address": "email",
+ },
+
+ /**
+ * Sets up the backend.
+ *
+ * @param Mongo mongo
@mikedeboer

mikedeboer Aug 16, 2013

Owner

another renaming issue

@mikedeboer mikedeboer commented on an outdated diff Aug 16, 2013

lib/DAVACL/backends/postgre.js
+ /**
+ * Returns the list of groups a principal is a member of
+ *
+ * @param {String} principal
+ * @return array
+ */
+ getGroupMemberShip: function(principal, callback) {
+ var self = this;
+ this.getPrincipalByPath(principal, function(err, principal) {
+ if (err)
+ return callback(err);
+ if (!principal)
+ return callback(new Exc.jsDAV_Exception("Principal not found"));
+
+ self.pg.query('SELECT \"group\" FROM ' + self.groupMembersTableName + ' WHERE \"member\"=$1', [principal.uri], function(err, result) {
+ if(err) {
@mikedeboer

mikedeboer Aug 16, 2013

Owner

the general style we tend to use for short-circuiting operations is

if (err)
    return callback(err);

notice the space between if and (? Could you correct that throughout your files?

@mikedeboer mikedeboer commented on an outdated diff Aug 16, 2013

lib/DAVACL/backends/postgre.js
+ * @return array
+ */
+ getGroupMemberShip: function(principal, callback) {
+ var self = this;
+ this.getPrincipalByPath(principal, function(err, principal) {
+ if (err)
+ return callback(err);
+ if (!principal)
+ return callback(new Exc.jsDAV_Exception("Principal not found"));
+
+ self.pg.query('SELECT \"group\" FROM ' + self.groupMembersTableName + ' WHERE \"member\"=$1', [principal.uri], function(err, result) {
+ if(err) {
+ return callback(err);
+ }
+ var out = [];
+ for(var i = 0; i < result.rows.length; i++) {
@mikedeboer

mikedeboer Aug 16, 2013

Owner

nit: missing a space between for and (

@mikedeboer mikedeboer commented on an outdated diff Aug 16, 2013

lib/DAVACL/backends/postgre.js
+ *
+ * @param {String} principal
+ * @param {Array} members
+ * @return void
+ */
+ setGroupMemberSet: function(principal, members, callback) {
+ var self = this;
+ this.pg.query('DELETE FROM ' + this.groupMembersTableName + ' WHERE \"group\"=$1', [principal], function(err, result) {
+ if(err) {
+ return callback(err);
+ }
+ Async.list(members)
+ .each(function(mem, next) {
+ self.pg.query('INSERT INTO '
+ + self.groupMembersTableName
+ + ' (\"group\", member) VALUES ($1, $2)',
@mikedeboer

mikedeboer Aug 16, 2013

Owner

do you need to escape " here, even if you're using ' for this string literal?

@mikedeboer mikedeboer commented on an outdated diff Aug 16, 2013

lib/shared/backends/postgre.js
@@ -0,0 +1,32 @@
+var getConnectionString = exports.getConnectionString = function(options) {
+ var auth;
@mikedeboer

mikedeboer Aug 16, 2013

Owner

general nits for this file:

  • it seems like you started using 2-spaces for indentation here instead of 4
  • can you put a license header on top of each file?
  • can you use double-quotes instead of single quotes for string literals? (" vs. ')
  • it is best practice to use "use strict"; for each JS file.

@mikedeboer mikedeboer commented on an outdated diff Aug 16, 2013

lib/shared/backends/postgre.js
+}
+
+exports.getConnection = function(options, callback) {
+ options = options || {};
+ var pg = require("pg");
+
+ var con = getConnectionString(options);
+ var client = new pg.Client(con);
+
+ client.connect(function(err) {
+ if(err) {
+ callback(err);
+ }
+ else {
+ callback(null, client);
+ }
@mikedeboer

mikedeboer Aug 16, 2013

Owner

I'd recommend writing this as

if (err)
    return callback(err);
callback(err, client);

@mikedeboer mikedeboer commented on an outdated diff Aug 16, 2013

@@ -16,6 +17,8 @@
"dependencies": {
"asyncjs": "~0.0.8",
"formidable": "~1.0.11",
+ "jsftp": "~0.5.4",
+ "node-sftp": "0.1.1",
@mikedeboer

mikedeboer Aug 16, 2013

Owner

why did you add these? Did you feel they were missing? Did you run into errors when these weren't defined?

@mikedeboer mikedeboer commented on an outdated diff Aug 16, 2013

test/postgre/auth.js
@@ -0,0 +1,60 @@
+var postgre = require('../../lib/shared/backends/postgre.js');
+var auth = require('../../lib/DAV/plugins/auth/postgre.js');
+var db = require('./db.js');
+var authInstance;
+var expect = require('chai').expect;
+var client;
+
+describe('auth', function () {
+ before(function(done) {
+ db.init(function (err) {
+ if(err) {
+ done(err);
+ }
@mikedeboer

mikedeboer Aug 16, 2013

Owner

you can collapse this to

if (err)
    return done(err);

postgre.getConnection(...

@mikedeboer mikedeboer commented on an outdated diff Aug 16, 2013

test/postgre/auth.js
+ authInstance = auth.new(client);
+
+ done();
+ }
+ });
+ }
+
+ });
+
+ });
+
+ it('should get the correct hash for an existing user', function (done) {
+ authInstance.getDigestHash('', 'daniel', function(err, hash) {
+ if(err) {
+ done(err);
+ }

@mikedeboer mikedeboer commented on an outdated diff Aug 16, 2013

test/postgre/auth.js
+ authInstance.getDigestHash('', 'daniel', function(err, hash) {
+ if(err) {
+ done(err);
+ }
+ else {
+ expect(hash).to.be.eq('abc');
+ done();
+ }
+ });
+ });
+
+ it('should return undefined for a non existing user', function(done) {
+ authInstance.getDigestHash('', 'armin', function(err, hash) {
+ if(err) {
+ done(err);
+ }

@mikedeboer mikedeboer commented on an outdated diff Aug 16, 2013

test/postgre/carddav.js
+ }
+ else {
+ client = cl;
+ cI = cDav.new(client);
+ done();
+ }
+ });
+ }
+ });
+ });
+
+ it('getAddressBooksForUser#', function(done) {
+ cI.getAddressBooksForUser('principals/me.daniel', function(err, list) {
+ if(err) {
+ done(err);
+ }
@mikedeboer

mikedeboer Aug 16, 2013

Owner

here you forgot the else clause, so it will keep executing the test on error.

I'd recommend writing this as:

if (err)
    return done(err);

expect(...

@mikedeboer mikedeboer commented on an outdated diff Aug 16, 2013

test/postgre/carddav.js
+ else {
+ client = cl;
+ cI = cDav.new(client);
+ done();
+ }
+ });
+ }
+ });
+ });
+
+ it('getAddressBooksForUser#', function(done) {
+ cI.getAddressBooksForUser('principals/me.daniel', function(err, list) {
+ if(err) {
+ done(err);
+ }
+ expect(list).to.be.an('array')
@mikedeboer

mikedeboer Aug 16, 2013

Owner

this can stay on one line.

@mikedeboer mikedeboer commented on an outdated diff Aug 16, 2013

test/postgre/carddav.js
+ cI.getAddressBooksForUser('principals/me.daniel', function(err, list) {
+ if(err) {
+ done(err);
+ }
+ expect(list).to.be.an('array')
+ .and.to.have.length(1);
+
+ expect(list[0]).to.have.property('id', 1);
+ expect(list[0]).to.have.property('uri', 'addr');
+ expect(list[0]).to.have.property('principaluri', 'principals/me.daniel');
+ expect(list[0]).to.have.property('{DAV:}displayname', null);
+ expect(list[0]).to.have.property('{http://calendarserver.org/ns/}getctag', 1);
+ expect(list[0]).to.have.property('{urn:ietf:params:xml:ns:carddav}addressbook-description', null);
+ expect(list[0]).to.have.property('{urn:ietf:params:xml:ns:carddav}supported-address-data');
+
+
@mikedeboer

mikedeboer Aug 16, 2013

Owner

nit: no extra newline needed.

@mikedeboer mikedeboer commented on an outdated diff Aug 16, 2013

test/postgre/carddav.js
+ it('updateAddressBook# should fail when setting unallowed properties', function(done) {
+ cI.updateAddressBook('1', {'{DAV:}abc': 'addressbook (DISP)'}, function(err, success) {
+ if(err) {
+ done(err)
+ }
+ expect(success).to.be.false;
+ done();
+ });
+ });
+
+ it('updateAddressBook# should update the database', function(done) {
+ cI.updateAddressBook('1', {'{DAV:}displayname': 'addressbook (DISP)'}, function(err, success) {
+ if(err) {
+ done(err)
+ }
+ expect(success).to.be.true;
@mikedeboer

mikedeboer Aug 16, 2013

Owner

same problem as above

@mikedeboer mikedeboer commented on an outdated diff Aug 16, 2013

test/postgre/carddav.js
+ expect(list[0]).to.have.property('{DAV:}displayname', null);
+ expect(list[0]).to.have.property('{http://calendarserver.org/ns/}getctag', 1);
+ expect(list[0]).to.have.property('{urn:ietf:params:xml:ns:carddav}addressbook-description', null);
+ expect(list[0]).to.have.property('{urn:ietf:params:xml:ns:carddav}supported-address-data');
+
+
+ done();
+ })
+ });
+
+ it('updateAddressBook# should fail when setting unallowed properties', function(done) {
+ cI.updateAddressBook('1', {'{DAV:}abc': 'addressbook (DISP)'}, function(err, success) {
+ if(err) {
+ done(err)
+ }
+ expect(success).to.be.false;
@mikedeboer

mikedeboer Aug 16, 2013

Owner

same problem as above

@mikedeboer mikedeboer commented on an outdated diff Aug 16, 2013

test/postgre/carddav.js
+ }]);
+ var outerResult = result[0];
+
+ client.query('SELECT * FROM addressbooks WHERE uri= \'daniel\' AND principaluri= \'principals/daniel\' ', function(err, result) {
+ if(err) {
+ return done(err);
+ }
+ expect(result.rows).to.be.an('array')
+ .and.to.have.length(1);
+ expect(result.rows[0]).to.be.deep.eq(outerResult);
+ done();
+ });
+ });
+ });
+
+ // it('test', function(done) {
@mikedeboer

mikedeboer Aug 16, 2013

Owner

if you commented out this test because it fails, can you explain why?
If this test is not needed, I think it's best to remove it.

@mikedeboer mikedeboer commented on an outdated diff Aug 16, 2013

test/postgre/connection.js
@@ -0,0 +1,52 @@
+var postgre = require('../../lib/shared/backends/postgre.js');
+var expect = require('chai').expect;
+var client;
+var db = require('./db.js')
+
+describe('connection', function () {
+ before(function(done) {
+ db.init(done);
+
@mikedeboer

mikedeboer Aug 16, 2013

Owner

nit: no additional newline needed

@mikedeboer mikedeboer commented on an outdated diff Aug 16, 2013

test/postgre/connection.js
@@ -0,0 +1,52 @@
+var postgre = require('../../lib/shared/backends/postgre.js');
+var expect = require('chai').expect;
+var client;
+var db = require('./db.js')
+
+describe('connection', function () {
+ before(function(done) {
+ db.init(done);
+
+ })
@mikedeboer

mikedeboer Aug 16, 2013

Owner

missing trailing semicolon, an additional newline would be nice

@mikedeboer mikedeboer commented on an outdated diff Aug 16, 2013

test/postgre/connection.js
+ console.error(err);
+ done(err);
+ }
+ else {
+ expect(result.rows).to.have.length(1);
+ done();
+ }
+ })
+ }
+ });
+ });
+
+ after(function(done) {
+ client.end();
+ db.cleanup(done);
+ })
@mikedeboer

mikedeboer Aug 16, 2013

Owner

missing semicolon

@mikedeboer mikedeboer commented on an outdated diff Aug 16, 2013

test/postgre/connection.js
+ client = cl;
+ if(err) {
+ console.error(err);
+ done(err);
+ }
+ else {
+ client.query('SELECT NOW() as "time"', function(err, result) {
+ if(err) {
+ console.error(err);
+ done(err);
+ }
+ else {
+ expect(result.rows).to.have.length(1);
+ done();
+ }
+ })
@mikedeboer

mikedeboer Aug 16, 2013

Owner

missing semicolon

@mikedeboer mikedeboer commented on an outdated diff Aug 16, 2013

test/postgre/carddav.js
+ return done(err);
+ }
+
+ expect(result.rows).to.have.length(1);
+ expect(result.rows[0]).to.be.an('object');
+
+ expect(result.rows[0]).to.have.property('uri', 'card02.vcf');
+ expect(result.rows[0]).to.have.property('addressbookid', 1);
+ expect(result.rows[0]).to.have.property('carddata', 'newdata');
+ expect(result.rows[0]).to.have.property('lastmodified');
+ expect(result.rows[0].lastmodified).to.be.closeTo(new Date(), 200);
+
+ done();
+ });
+
+ })
@mikedeboer

mikedeboer Aug 16, 2013

Owner

missing semicolon

@mikedeboer mikedeboer commented on an outdated diff Aug 16, 2013

test/postgre/carddav.js
@@ -0,0 +1,248 @@
+var postgre = require('../../lib/shared/backends/postgre.js');
@mikedeboer

mikedeboer Aug 16, 2013

Owner

General nits for this file:

  • two space indentation instead of four
  • please review your use of semicolons
  • missing license header
  • missing "use strict";
  • use of ' vs. " for string literals
  • static references declared on top of the file should start capitalized, with a descriptive name, like jsDAV_DAVACL_Backends_PostGre = require("...");. It might seem long, java-ish, etc., but it certainly helps maintaining readability of the entire source base.

@mikedeboer mikedeboer commented on an outdated diff Aug 16, 2013

test/postgre/connection.js
@@ -0,0 +1,52 @@
+var postgre = require('../../lib/shared/backends/postgre.js');
@mikedeboer

mikedeboer Aug 16, 2013

Owner

General nits for this file:

  • two space indentation instead of four
  • please review your use of semicolons
  • missing license header
  • missing "use strict";
  • use of ' vs. " for string literals
  • static references declared on top of the file should start capitalized, with a descriptive name, like jsDAV_DAVACL_Backends_PostGre = require("...");. It might seem long, java-ish, etc., but it certainly helps maintaining readability of the entire source base.

@mikedeboer mikedeboer commented on an outdated diff Aug 16, 2013

test/postgre/davacl.js
@@ -0,0 +1,134 @@
+var postgre = require('../../lib/shared/backends/postgre.js');
@mikedeboer

mikedeboer Aug 16, 2013

Owner

General nits for this file:

  • two space indentation instead of four
  • please review your use of semicolons
  • missing license header
  • missing "use strict";
  • use of ' vs. " for string literals
  • static references declared on top of the file should start capitalized, with a descriptive name, like jsDAV_DAVACL_Backends_PostGre = require("...");. It might seem long, java-ish, etc., but it certainly helps maintaining readability of the entire source base.

@mikedeboer mikedeboer commented on an outdated diff Aug 16, 2013

test/postgre/auth.js
@@ -0,0 +1,60 @@
+var postgre = require('../../lib/shared/backends/postgre.js');
@mikedeboer

mikedeboer Aug 16, 2013

Owner

General nits for this file:

  • two space indentation instead of four
  • please review your use of semicolons
  • missing license header
  • missing "use strict";
  • use of ' vs. " for string literals
  • static references declared on top of the file should start capitalized, with a descriptive name, like jsDAV_DAVACL_Backends_PostGre = require("...");. It might seem long, java-ish, etc., but it certainly helps maintaining readability of the entire source base.

@mikedeboer mikedeboer commented on an outdated diff Aug 16, 2013

test/postgre/db.js
@@ -0,0 +1,111 @@
+var pg = require('pg');
@mikedeboer

mikedeboer Aug 16, 2013

Owner

General nits for this file:

  • two space indentation instead of four
  • please review your use of semicolons
  • missing license header
  • missing "use strict";
  • use of ' vs. " for string literals
  • static references declared on top of the file should start capitalized, with a descriptive name, like jsDAV_DAVACL_Backends_PostGre = require("...");. It might seem long, java-ish, etc., but it certainly helps maintaining readability of the entire source base.
Owner

mikedeboer commented Aug 16, 2013

I vote for the name 'postgres', after reading what @pygy said 😄

Owner

mikedeboer commented Oct 2, 2013

@dlaxar did you have time to look at the comments I made earlier?

Contributor

dlaxar commented Jul 8, 2014

Sorry it took so long - I should have fixed all the issues. Hope this helps

Owner

mikedeboer commented Jul 9, 2014

@dlaxar I do see many formatting nits, but I'll fix those up right after the merge. OK?

Thanks for picking this up again! Much appreciated :)

mikedeboer merged commit 2c753a8 into mikedeboer:master Jul 9, 2014

Contributor

dlaxar commented Jul 9, 2014

I didn't see any but maybe I'm just overseeing them. Thank you :)

Owner

mikedeboer commented Jul 9, 2014

Please see commit 376172e for all the fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment