Model changes

[jvillafanez]

Refactor plans

Introduce OCA\DataExporter\Model\IModel

The IModel will be the base interface of all models. All valid models MUST implement this interface. The IModel interface won't have any method defined

Using this interface will allow us to create base interfaces for the extractor and importer classes, and enforce the same signature, such as:

interface Importer {
  public function import(IModel);
}
......
interface Extractor {
  public function extract(): IModel;
}

It's expected that each importer and extractor subclasses use specific models, for example:

class FileImporter extends Importer {
  public function import(File)
}
......
class FileExtractor extends Extractor {
  public function extract(): File
}

Note that the File needs to extends the IModel interface.

~Introduce OCA\DataExporter\Model\BaseCollectionModel~ (discarded)

The class BaseCollectionModel will extend the IModel interface, and it will serve as base class of collections.

Expected implementation would be something like:

abstract class BaseCollectionModel implements IModel {
    private $collection = [];
    protected abstract function getCollectionType(): string;

    public function setItems(array $items): bool {
        foreach ($items as $item) {
            if (\gettype($item) !== $this->getCollectionType()) {
                return false;
            }
        }
        $this->collection = $items;
        return true;
    }

    public function getItems(): array {
        return $this->collection;
    }
}

This means that, for example, we'll have a File and a FileCollection models. The File class will implement the IModel interface while the FileCollection will extend the BaseCollectionModel

Expected implementation for the FileCollection class should be

class FileCollection extends BaseCollectionModel {
    protected abstract function getCollectionType(): string {
        return File::class;
    }
}

The idea behind this change is to allow a strict type checking.

• The current User model might be modified to use these new "collection" classes. For example:

  public function getFiles(): FilesCollection { ..... }
  public function setFiles(FilesCollection $files) { ..... }

  There are other models that can benefit from this change

• Importers and extractors can also use these "collection". Instead of extracting an array of File model (which you assume that contains only File), you can return a FileCollection. The importer can rely on using a FileCollection and ensure that only File models are there.

Folder structure changes

Model/
Model/IModel.php
Model/UserMetadata.php    (renamed from the current Metadata.php)
Model/UserMetadata/User.php
Model/UserMetadata/User/File.php
Model/UserMetadata/User/File/Version.php
Model/UserMetadata/User/Preference.php
Model/UserMetadata/User/Share.php

The "big" change would be to rename the current Metadata.php to UserMetadata, and move almost all the current classes inside the folder. This should reflect the dependecies among the models

[IljaN]

Collections: While this change might be the right thing to do from the object oriented design standpoint and might slightly improve maintainability I don't see any practical benefit this gives us.

• Currently we don't have that much code which is prone to type errors, we also don't have code which does crazy collection handling where this might benefit us a lot.
• We can soft type-hint collections today via stan/phan using File[] or File<int, File> in doc-blocks to catch errors at "compile-time"
• Let's try to solve the problem without inheritance (trough composition)
• This adds some level of complexity

I am not completely opposed to this, but I think we should get more functionality going before thinking of further refactorings. Also at the moment I think we don't know if our architecture is 100% valid as more and more requirements trickle in and we find new edge-case. So as long as code is changed a lot we should not in-grain types too deep too early.

Other changes make sense.

[jvillafanez]

Yes. There are additional problems to solve with the symfony's serializer if we decide to use collections. The changes will be bigger than the ones sketched.

So, in exchange of dismissing the collections:

• Importers will be limited to import only one model. In case of arrays, the outer importer will traverse the list of items.
• The importer could have other methods oriented to optimize the import mechanism. Those methods can still be used due to the specific dependencies between importers. ("UserImporter" depends on "FileImporter", specifically on "FileImporter" and not on any other generic importer)

For the extractors, we'll likely need them to return some kind of model iterator. The idea will be something similar to the collections but outside of the models. This means that, even if the extractor is expected to return only one object (the UserExtractor for example), it will be required to return an iterator over that element. I'm still thinking how to do this.

[jvillafanez]

For the extractors, we'll likely need them to return some kind of model iterator.

This is quite flexible. The extractor interface will return an iterator; other extractors can return a Generator, an ArrayIterator (for the common case), or even a custom iterator (mainly for proper type hinting, but this shouldn't be needed)

[jvillafanez]

For the Version model, it will only contain the path where the version is located. The specific path shouldn't matter.

The File model will have a list of Versions. This list might be empty. Note that the list of versions shouldn't contain the actual file (or latest version that should be shown to the user).

{
  "type": "file",
  "path": "my/file.txt",
  "eTag": "1234567890",
  "permissions": 31,
  "versions" : [
    {"path": "my/file.txt.v01"},
    {"path": "my/file.txt.v02"},
    {"path": "my/file.txt.v03"},
  ]
}

Note that the list of versions will be processed as they're defined. For the example above, we'll write first the v01, then the v02, then the v03 and finally the actual file.

To further clarify, this is also valid:

{
  "type": "file",
  "path": "my/file.txt",
  "eTag": "1234567890",
  "permissions": 31,
  "versions" : [
    {"path": "versions/my/v01.txt"},
    {"path": "versions/my/v40.txt"},
    {"path": "my/file.txt.v03"},
  ]
}

[IljaN]

This is something where I already stumbled while thinking about comments which also belong to a file: With the current architecture we have specialized extractors. Now if we nest it like this the files-extractor/importer will need to know about versions and potentially comments.

[jvillafanez]

This is something where I already stumbled while thinking about comments which also belong to a file: With the current architecture we have specialized extractors. Now if we nest it like this the files-extractor/importer will need to know about versions and potentially comments.

Not really. The file extractor can depend on a version extractor and a comment extractor to take care of fetching the versions and comments. Something similar can be done for the importer.

[jvillafanez]

parent-child model relationship

As said above, the plan is to have a import(IModel $model) method. This implies:

• We'll only import one model at a time. If we need to import multiple models (several files for one user, for example), that method will need to be called for each model
• There is no additional information outside of the model.

The VersionImporter would look like:

public function import(Version $version) { .... }

According to the model, we need some file model information (the file path) to know where the versions should be placed, or what is the file associated to the versions.

In order to deal with this we'll need some additional changes:

• Change the planned IModel interface to a AbstractModel abstract class. It will contain:

  abstract class AbstractModel {
    /** @var AbstractModel */
    private $parent;
    public final function setParent(AbstractModel $model) {
      $this->parent = $model;
    }
    public final function getParent(): AbstractModel {
      return $this->parent;
    }
  }

  This is intended to include a parent-child relationship between models, so "child" models can get information about its parent. For the Version case, we could fetch the information about the associated file and upload the versions there.

  Note that these methods won't be overwritten to ensure a known behaviour

• Adjust "set" methods to use this setParent call. For example:

  public function setFiles(array $files) {
      foreach ($files as $file) {
          $file->setParent($this);
      }
      ....
  }

• Adjust our symfony serializer to ignore the "parent" variable in our models. The ObjectNormalizer needs to ignore that attribute. This should allow to serialize the models properly with these changes.