get group from CACHE after update/create in SCIMController

[soltanireza65]

isn't it better to return the final group members from DB in Create and Update? instead of returning the body??? this way we can be sure that our tests are more precise... we can check the body len with response len in tests (if the members in DB are the same as the request body) Branch: https://github.com/SURFnet/rd-sram-integration/tree/fix/issues-197

[michielbdejong]

Is this a reply to #190?

[michielbdejong]

Or you mean to propose this in general as a strategy for writing functions that are easy to test?

[soltanireza65]

my main goal is to have functions that are easy to test because this way we are 100% sure that whether the group and its members have been inserted or not

[soltanireza65]

working on https://github.com/SURFnet/rd-sram-integration/tree/refactor-scim

[soltanireza65]

this branch has 2 points:

• having functions that are easy to test
• making sure updateGroup/createGroup responses is correct

[soltanireza65]

the second one is somehow fixed with https://github.com/SURFnet/rd-sram-integration/issues/195 and for the first one, I have this branch to refactor scimController https://github.com/SURFnet/rd-sram-integration/tree/refactor-scim

[michielbdejong]

we decided that it should be from cache, not from db

[soltanireza65]

at first, I want to add cachedGroups to the new Group backend but then I realized that the manager was actually handling the cache, so I think we can freely call getGroup cous it's internally calling cache to get the groups if the is one

this is the flow if anyone cares: groupManager->get(gid)

public function get($gid) {
        if (isset($this->cachedGroups[$gid])) {
            return $this->cachedGroups[$gid];
        }
        return $this->getGroupObject($gid);
}

protected function getGroupObject($gid, $displayName = null) {
        $backends = [];
        foreach ($this->backends as $backend) {
            if ($backend->groupExists($gid)) {
                if ($backend->implementsActions(\OC\Group\Backend::GROUP_DETAILS)) {
                    /* @phan-suppress-next-line PhanUndeclaredMethod */
                    $groupData = $backend->getGroupDetails($gid);
                    if (\is_array($groupData)) {
                        // take the display name from the first backend that has a non-null one
                        if ($displayName === null && isset($groupData['displayName'])) {
                            $displayName = $groupData['displayName'];
                        }
                    }
                }
                $backends[] = $backend;
            }
        }
        if (\count($backends) === 0) {
            return null;
        }
        $this->cachedGroups[$gid] = new Group($gid, $backends, $this->userManager, $this->eventDispatcher, $this, $displayName);
        return $this->cachedGroups[$gid];
    }

so it seems we can call groupManager->get(gid)

then I can continue Refactor ScimController further

[navid-shokri]

you should to check that the cache is filled on creation time. I think you should check the mechanism of inserting items into the cache

[soltanireza65]

@navid-shokri $cachedGroups is a local property in groupManager and we are creating the group in groupBackend

[soltanireza65]

we can also cashe the group members in groupBackend

[navid-shokri]

we should discuss the problem with @michielbdejong @yasharpm @thepeak99

but remind us that we were forced to add a group using the backend directly because we were using the default Group PHP object and there was no mechanism to determine the related group backend inside groupManager. groupManager always were using the default group backend (oc_groups) to persist it into the database.

[thepeak99]

well if we are writing and reading to/from the cache and you're sure that we're writing to the cache it's perfectly fine... (the cache has to be flushed to the DB of course but I assume that's also happening :rofl: ).

If that's the case it's all good... I'm just thinking of a way to actually make sure that is the case... I think the best way would be to start a little cluster with docker-compose and test that it doesnt do anything funny.

[yasharpm]

The problem is that after we create a group here, we query it again from the GroupManager here but the GroupManager won't find it and return null, because of the master/slave database situation.

But all we use the GroupManager for, is to get our hands on the Group object that we have just created. I think we can instantiate that Group object ourselves, instead of asking the GroupManager to do it for us. We already have all the parameters required to create the Group object. As @soltanireza65 already demonstrated in the getGroup function implemented here:

protected function getGroupObject($gid, $displayName = null) {
        $backends = [];
        foreach ($this->backends as $backend) {
            if ($backend->groupExists($gid)) {
                if ($backend->implementsActions(\OC\Group\Backend::GROUP_DETAILS)) {
                    /* @phan-suppress-next-line PhanUndeclaredMethod */
                    $groupData = $backend->getGroupDetails($gid);
                    if (\is_array($groupData)) {
                        // take the display name from the first backend that has a non-null one
                        if ($displayName === null && isset($groupData['displayName'])) {
                            $displayName = $groupData['displayName'];
                        }
                    }
                }
                $backends[] = $backend;
            }
        }
        if (\count($backends) === 0) {
            return null;
        }
        $this->cachedGroups[$gid] = new Group($gid, $backends, $this->userManager, $this->eventDispatcher, $this, $displayName);
        return $this->cachedGroups[$gid];
    }

Except we already know that there is only one group backend (which is our GroupBackend instance) and we don't need to query it. So it will be an easy one liner:

$group = new Group($gid, [$this->groupBackend], $this->userManager, $this->eventDispatcher, $this->groupManager, $displayName);

The rest of the code in the groupBackend function should be fine since we want to work with the cached data already and for a newly created group, the member list is correctly empty already.

[yasharpm]

In general, our strategy towards fixing the master/slave DB situation should be that we do not try to read anything we have written into the DB ourselves. Instead we assume that the changes we wrote is already there using the cache strategy or any other work around.

[soltanireza65]

answer for I think we can instantiate that Group object ourselves, instead of asking the GroupManager to do it for us. @yasharpm in order to instantiate a group Object we need to have access

The problem is that after we create a group here, we query it again from the GroupManager here but the GroupManager won't find it and return null, because of the master/slave database situation.

But all we use the GroupManager for, is to get our hands on the Group object that we have just created. I think we can instantiate that Group object ourselves, instead of asking the GroupManager to do it for us. We already have all the parameters required to create the Group object. As @soltanireza65 already demonstrated in the getGroup function implemented here:

protected function getGroupObject($gid, $displayName = null) {
      $backends = [];
      foreach ($this->backends as $backend) {
          if ($backend->groupExists($gid)) {
              if ($backend->implementsActions(\OC\Group\Backend::GROUP_DETAILS)) {
                  /* @phan-suppress-next-line PhanUndeclaredMethod */
                  $groupData = $backend->getGroupDetails($gid);
                  if (\is_array($groupData)) {
                      // take the display name from the first backend that has a non-null one
                      if ($displayName === null && isset($groupData['displayName'])) {
                          $displayName = $groupData['displayName'];
                      }
                  }
              }
              $backends[] = $backend;
          }
      }
      if (\count($backends) === 0) {
          return null;
      }
      $this->cachedGroups[$gid] = new Group($gid, $backends, $this->userManager, $this->eventDispatcher, $this, $displayName);
      return $this->cachedGroups[$gid];
  }

Except we already know that there is only one group backend (which is our GroupBackend instance) and we don't need to query it. So it will be an easy one liner:

$group = new Group($gid, [$this->groupBackend], $this->userManager, $this->eventDispatcher, $this->groupManager, $displayName);

The rest of the code in the groupBackend function should be fine since we want to work with the cached data already and for a newly created group, the member list is correctly empty already.

this is the same thing that the cache is trying to give us nice

[soltanireza65]

so for now the given solution is to instantiate a Group instead of getting it directly from groupManager in ScimController::createGroup

[yasharpm]

in order to instantiate a group Object we need to have access

The constructor for the Group class is public according to it's source code so we should be fine here.

so for now the given solution is to instantiate a Group instead of getting it directly from groupManager in ScimController::createGroup

I think so yes! :-)

[soltanireza65]

final touches add in fc3d296 (#215)