Auth._loadRoles should not query the same role twice.

spec/ParseRole.spec.js

@@ -2,6 +2,7 @@
 
 // Roles are not accessible without the master key, so they are not intended
 // for use by clients.  We can manually test them using the master key.
+var RestQuery = require("../src/RestQuery");
 var Auth = require("../src/Auth").Auth;
 var Config = require("../src/Config");
 
@@ -60,21 +61,87 @@ describe('Parse Role testing', () => {
 
   });
 
-  it("should recursively load roles", (done) => {
+  var createRole = function(name, sibling, user) {
-
+    var role = new Parse.Role(name, new Parse.ACL());
-    var rolesNames = ["FooRole", "BarRole", "BazRole"];
+    if (user) {
-
+      var users = role.relation('users');
-    var createRole = function(name, sibling, user) {
+      users.add(user);
-      var role = new Parse.Role(name, new Parse.ACL());
-      if (user) {
-        var users = role.relation('users');
-        users.add(user);
-      }
-      if (sibling) {
-        role.relation('roles').add(sibling);
-      }
-      return role.save({}, { useMasterKey: true });
     }
+    if (sibling) {
+      role.relation('roles').add(sibling);
+    }
+    return role.save({}, { useMasterKey: true });
+  };
+
+  it("should not recursively load the same role multiple times", (done) => {
+    var rootRole = "RootRole";
+    var roleNames = ["FooRole", "BarRole", "BazRole"];
+    var allRoles = [rootRole].concat(roleNames);
+
+    var roleObjs = {};
+    var createAllRoles = function(user) {
+      var promises = allRoles.map(function(roleName) {
+        return createRole(roleName, null, user)
+          .then(function(roleObj) {
+            roleObjs[roleName] = roleObj;
+            return roleObj;
+          });
+      });
+      return Promise.all(promises);
+    };
+
+    var restExecute = spyOn(RestQuery.prototype, "execute").and.callThrough();
+
+    var user,
+      auth,
+      getAllRolesSpy;
+    createTestUser().then( (newUser) => {
+      user = newUser;
+      return createAllRoles(user);
+    }).then ( (roles) => {
+      var rootRoleObj = roleObjs[rootRole];
+      roles.forEach(function(role, i) {
+        // Add all roles to the RootRole
+        if (role.id !== rootRoleObj.id) {
+          role.relation("roles").add(rootRoleObj);
+        }
+        // Add all "roleNames" roles to the previous role
+        if (i > 0) {
+          role.relation("roles").add(roles[i - 1]);
+        }
+      });
+
+      return Parse.Object.saveAll(roles, { useMasterKey: true });
+    }).then( () => {
+      auth = new Auth({config: new Config("test"), isMaster: true, user: user});
+      getAllRolesSpy = spyOn(auth, "_getAllRoleNamesForId").and.callThrough();
+
+      return auth._loadRoles();
+    }).then ( (roles) => {
+      expect(roles.length).toEqual(4);
+
+      allRoles.forEach(function(name) {
+        expect(roles.indexOf("role:"+name)).not.toBe(-1);
+      });
+
+      // 1 Query for the initial setup
+      // 4 Queries for all the specific roles
+      // 1 Query for the final $in
+      expect(restExecute.calls.count()).toEqual(6);
+
+      // 4 One query for each of the roles
+      // 3 Queries for the "RootRole"
+      expect(getAllRolesSpy.calls.count()).toEqual(7);
+      done()
+    }).catch( () =>  {
+      fail("should succeed");
+      done();
+    });
+
+  });
+
+  it("should recursively load roles", (done) => {
+    var rolesNames = ["FooRole", "BarRole", "BazRole"];
     var roleIds = {};
      createTestUser().then( (user) => {
        // Put the user on the 1st role
@@ -97,7 +164,7 @@ describe('Parse Role testing', () => {
        expect(roles.length).toEqual(3);
        rolesNames.forEach( (name) => {
         expect(roles.indexOf('role:'+name)).not.toBe(-1);
-       })
+       });
        done();
      }, function(err){
        fail("should succeed")

src/Auth.js

@@ -117,8 +117,9 @@ Auth.prototype._loadRoles = function() {
 
     var roleIDs = results.map(r => r.objectId);
     var promises = [Promise.resolve(roleIDs)];
+    var queriedRoles = {};
     for (var role of roleIDs) {
-      promises.push(this._getAllRoleNamesForId(role));
+      promises.push(this._getAllRoleNamesForId(role, queriedRoles));
     }
     return Promise.all(promises).then((results) => {
       var allIDs = [];
@@ -146,8 +147,12 @@ Auth.prototype._loadRoles = function() {
 };
 
 // Given a role object id, get any other roles it is part of
-Auth.prototype._getAllRoleNamesForId = function(roleID) {
+Auth.prototype._getAllRoleNamesForId = function(roleID, queriedRoles = {}) {
-  
+  // Don't need to requery this role as it is already being queried for.
+  if (queriedRoles[roleID] != null) {
+      return Promise.resolve([]);
+  }
+  queriedRoles[roleID] = true;
   // As per documentation, a Role inherits AnotherRole
   // if this Role is in the roles pointer of this AnotherRole
   // Let's find all the roles where this role is in a roles relation
@@ -173,7 +178,7 @@ Auth.prototype._getAllRoleNamesForId = function(roleID) {
     // Get the roles where those found roles are also
     // referenced the same way
     var parentRolesPromises = roleIDs.map( (roleId) => {
-      return this._getAllRoleNamesForId(roleId);
+      return this._getAllRoleNamesForId(roleId, queriedRoles);
     });
     parentRolesPromises.push(Promise.resolve(roleIDs));
     return Promise.all(parentRolesPromises);