fix: ensure all Person instances have valid ids

addresses #812

Author: rokroskar

docs/models/datasets.rst

@@ -37,11 +37,3 @@ Manage files in the dataset.
 .. autoclass:: renku.core.models.datasets.DatasetFile
    :members:
    :inherited-members:
-
-
-Creator
--------
-
-.. autoclass:: renku.core.models.datasets.Creator
-   :members:
-   :inherited-members:

renku/cli/exception_handler.py

@@ -134,10 +134,10 @@ def _handle_sentry(self):
             with capture_internal_exceptions():
                 from git import Repo
                 from renku.core.commands import get_git_home
-                from renku.core.models.datasets import Creator
+                from renku.core.models.provenance.agents import Person
 
                 repo = Repo(get_git_home())
-                user = Creator.from_git(repo)
+                user = Person.from_git(repo)
 
                 scope.user = {'name': user.name, 'email': user.email}
 

renku/core/commands/dataset.py

@@ -42,7 +42,8 @@
     MigrationRequired, ParameterError
 from renku.core.management.datasets import DATASET_METADATA_PATHS
 from renku.core.management.git import COMMIT_DIFF_STRATEGY
-from renku.core.models.datasets import Creator, Dataset
+from renku.core.models.datasets import Dataset
+from renku.core.models.provenance.agents import Person
 from renku.core.models.refs import LinkReference
 from renku.core.models.tabulate import tabulate
 from renku.core.utils.doi import extract_doi
@@ -107,7 +108,7 @@ def create_dataset(client, name, handle_duplicate_fn=None):
     existing = client.load_dataset(name=name)
     if (not existing or handle_duplicate_fn and handle_duplicate_fn(existing)):
         with client.with_dataset(name=name) as dataset:
-            creator = Creator.from_git(client.repo)
+            creator = Person.from_git(client.repo)
             if creator not in dataset.creator:
                 dataset.creator.append(creator)
 

renku/core/management/datasets.py

@@ -35,10 +35,10 @@
 
 from renku.core import errors
 from renku.core.management.config import RENKU_HOME
-from renku.core.models.datasets import Creator, Dataset, DatasetFile, \
-    DatasetTag
+from renku.core.models.datasets import Dataset, DatasetFile, DatasetTag
 from renku.core.models.git import GitURL
 from renku.core.models.locals import with_reference
+from renku.core.models.provenance.agents import Person
 from renku.core.models.refs import LinkReference
 
 
@@ -391,7 +391,7 @@ def _add_from_git(self, dataset, dataset_path, url, sources, destination):
                 creators = []
                 # grab all the creators from the commit history
                 for commit in repo.iter_commits(paths=path):
-                    creator = Creator.from_commit(commit)
+                    creator = Person.from_commit(commit)
                     if creator not in creators:
                         creators.append(creator)
 

renku/core/management/repository.py

@@ -394,8 +394,8 @@ def init_repository(self, name=None, force=False):
         self.repo.description = name or path.name
 
         # Check that an creator can be determined from Git.
-        from renku.core.models.datasets import Creator
-        Creator.from_git(self.repo)
+        from renku.core.models.provenance.agents import Person
+        Person.from_git(self.repo)
 
         # TODO read existing gitignore and create a unique set of rules
         import pkg_resources

renku/core/models/datasets.py

@@ -17,7 +17,6 @@
 # limitations under the License.
 """Model objects representing datasets."""
 
-import configparser
 import datetime
 import os
 import pathlib
@@ -30,7 +29,6 @@
 import attr
 from attr.validators import instance_of
 
-from renku.core import errors
 from renku.core.models.provenance.agents import Person
 from renku.core.models.provenance.entities import Entity
 from renku.core.utils.datetime8601 import parse_date
@@ -54,69 +52,24 @@ def _extract_doi(value):
     return value
 
 
-class Creator(Person):
-    """Represent the creator of a resource."""
-
-    affiliation = jsonld.ib(
-        default=None, kw_only=True, context='schema:affiliation'
-    )
-
-    alternate_name = jsonld.ib(
-        default=None, kw_only=True, context='schema:alternateName'
-    )
-
-    @property
-    def short_name(self):
-        """Gives full name in short form."""
-        names = self.name.split()
-        if len(names) == 1:
-            return self.name
-
-        last_name = names[-1]
-        initials = [name[0] for name in names]
-        initials.pop()
-
-        return '{0}.{1}'.format('.'.join(initials), last_name)
-
-    @classmethod
-    def from_git(cls, git):
-        """Create an instance from a Git repo."""
-        git_config = git.config_reader()
-        try:
-            name = git_config.get_value('user', 'name', None)
-            email = git_config.get_value('user', 'email', None)
-        except (
-            configparser.NoOptionError, configparser.NoSectionError
-        ):  # pragma: no cover
-            raise errors.ConfigurationError(
-                'The user name and email are not configured. '
-                'Please use the "git config" command to configure them.\n\n'
-                '\tgit config --global --add user.name "John Doe"\n'
-                '\tgit config --global --add user.email '
-                '"john.doe@example.com"\n'
-            )
-
-        # Check the git configuration.
-        if not name:  # pragma: no cover
-            raise errors.MissingUsername()
-        if not email:  # pragma: no cover
-            raise errors.MissingEmail()
-
-        return cls(name=name, email=email)
+def _convert_creators(value):
+    """Convert creators."""
+    if isinstance(value, dict):  # compatibility with previous versions
+        return [Person.from_jsonld(value)]
 
-    def __attrs_post_init__(self):
-        """Finish object initialization."""
-        # handle the case where ids were improperly set
-        if self._id == 'mailto:None':
-            self._id = self.default_id()
+    if isinstance(value, list):
+        return [Person.from_jsonld(v) for v in value]
 
 
 @attr.s
-class CreatorsMixin:
+class PersonMixin:
     """Mixin for handling creators container."""
 
     creator = jsonld.container.list(
-        Creator, kw_only=True, context='schema:creator'
+        Person,
+        kw_only=True,
+        context='schema:creator',
+        converter=_convert_creators
     )
 
     @property
@@ -190,34 +143,16 @@ class Language:
     name = jsonld.ib(default=None, kw_only=True, context='schema:name')
 
 
-def _convert_dataset_files_creators(value):
-    """Convert dataset files creators."""
-    coll = value
-
-    if isinstance(coll, dict):
-        return [Creator.from_jsonld(coll)]
-
-    if isinstance(coll, list):
-        return [Creator.from_jsonld(c) for c in coll]
-
-
 @jsonld.s(
     type='schema:DigitalDocument',
     slots=True,
     context={
         'schema': 'http://schema.org/',
     }
 )
-class DatasetFile(Entity, CreatorsMixin):
+class DatasetFile(Entity, PersonMixin):
     """Represent a file in a dataset."""
 
-    creator = jsonld.container.list(
-        Creator,
-        converter=_convert_dataset_files_creators,
-        kw_only=True,
-        context='schema:creator'
-    )
-
     added = jsonld.ib(
         converter=parse_date, context='schema:dateCreated', kw_only=True
     )
@@ -290,15 +225,6 @@ def _convert_dataset_tags(value):
     return [DatasetTag.from_jsonld(v) for v in value]
 
 
-def _convert_dataset_creator(value):
-    """Convert dataset creators."""
-    if isinstance(value, dict):  # compatibility with previous versions
-        return [Creator.from_jsonld(value)]
-
-    if isinstance(value, list):
-        return [Creator.from_jsonld(v) for v in value]
-
-
 def _convert_language(obj):
     """Convert language object."""
     if isinstance(obj, dict):
@@ -324,7 +250,7 @@ def _convert_keyword(keywords):
         'schema': 'http://schema.org/',
     },
 )
-class Dataset(Entity, CreatorsMixin):
+class Dataset(Entity, PersonMixin):
     """Repesent a dataset."""
 
     SUPPORTED_SCHEMES = ('', 'file', 'http', 'https', 'git+https', 'git+ssh')
@@ -337,13 +263,6 @@ class Dataset(Entity, CreatorsMixin):
     _id = jsonld.ib(default=None, context='@id', kw_only=True)
     _label = jsonld.ib(default=None, context='rdfs:label', kw_only=True)
 
-    creator = jsonld.container.list(
-        Creator,
-        converter=_convert_dataset_creator,
-        context='schema:creator',
-        kw_only=True
-    )
-
     date_published = jsonld.ib(
         default=None, context='schema:datePublished', kw_only=True
     )

renku/core/models/projects.py

@@ -23,8 +23,8 @@
 import attr
 
 from renku.core.models import jsonld
-from renku.core.models.datasets import Creator
 from renku.core.models.datastructures import Collection
+from renku.core.models.provenance.agents import Person
 from renku.core.utils.datetime8601 import parse_date
 
 PROJECT_URL_PATH = 'projects'
@@ -89,14 +89,14 @@ def __attrs_post_init__(self):
         """Initialize computed attributes."""
         if not self.creator and self.client:
             if self.client.renku_metadata_path.exists():
-                self.creator = Creator.from_commit(
+                self.creator = Person.from_commit(
                     self.client.find_previous_commit(
                         self.client.renku_metadata_path, return_first=True
                     ),
                 )
             else:
                 # this assumes the project is being newly created
-                self.creator = Creator.from_git(self.client.repo)
+                self.creator = Person.from_git(self.client.repo)
 
         self._id = self.project_id
 

renku/core/models/provenance/agents.py

@@ -17,9 +17,11 @@
 # limitations under the License.
 """Represent provenance agents."""
 
+import configparser
 import re
 import uuid
 
+from renku.core import errors
 from renku.core.models import jsonld as jsonld
 from renku.version import __version__, version_url
 
@@ -33,7 +35,6 @@
         'schema': 'http://schema.org/',
         'prov': 'http://www.w3.org/ns/prov#',
     },
-    frozen=True,
     slots=True,
 )
 class Person:
@@ -42,7 +43,12 @@ class Person:
     name = jsonld.ib(context='schema:name')
     email = jsonld.ib(context='schema:email', default=None, kw_only=True)
     label = jsonld.ib(context='rdfs:label', kw_only=True)
-
+    affiliation = jsonld.ib(
+        default=None, kw_only=True, context='schema:affiliation'
+    )
+    alternate_name = jsonld.ib(
+        default=None, kw_only=True, context='schema:alternateName'
+    )
     _id = jsonld.ib(context='@id', kw_only=True)
 
     @_id.default
@@ -73,6 +79,51 @@ def from_commit(cls, commit):
             email=commit.author.email,
         )
 
+    @property
+    def short_name(self):
+        """Gives full name in short form."""
+        names = self.name.split()
+        if len(names) == 1:
+            return self.name
+
+        last_name = names[-1]
+        initials = [name[0] for name in names]
+        initials.pop()
+
+        return '{0}.{1}'.format('.'.join(initials), last_name)
+
+    @classmethod
+    def from_git(cls, git):
+        """Create an instance from a Git repo."""
+        git_config = git.config_reader()
+        try:
+            name = git_config.get_value('user', 'name', None)
+            email = git_config.get_value('user', 'email', None)
+        except (
+            configparser.NoOptionError, configparser.NoSectionError
+        ):  # pragma: no cover
+            raise errors.ConfigurationError(
+                'The user name and email are not configured. '
+                'Please use the "git config" command to configure them.\n\n'
+                '\tgit config --global --add user.name "John Doe"\n'
+                '\tgit config --global --add user.email '
+                '"john.doe@example.com"\n'
+            )
+
+        # Check the git configuration.
+        if not name:  # pragma: no cover
+            raise errors.MissingUsername()
+        if not email:  # pragma: no cover
+            raise errors.MissingEmail()
+
+        return cls(name=name, email=email)
+
+    def __attrs_post_init__(self):
+        """Finish object initialization."""
+        # handle the case where ids were improperly set
+        if self._id == 'mailto:None':
+            self._id = self.default_id()
+
 
 @jsonld.s(
     type=[

tests/core/commands/test_dataset.py

@@ -25,7 +25,8 @@
 import git
 import pytest
 
-from renku.core.models.datasets import Creator, Dataset, DatasetFile
+from renku.core.models.datasets import Dataset, DatasetFile
+from renku.core.models.provenance.agents import Person
 
 
 def _key(client, dataset, filename):
@@ -140,7 +141,7 @@ def test_git_repo_import(client, dataset, tmpdir, data_repository):
 
 @pytest.mark.parametrize(
     'creators', [
-        [Creator(name='me', email='me@example.com')],
+        [Person(name='me', email='me@example.com')],
         [{
             'name': 'me',
             'email': 'me@example.com',
@@ -150,14 +151,14 @@ def test_git_repo_import(client, dataset, tmpdir, data_repository):
 def test_creator_parse(creators, data_file):
     """Test that different options for specifying creators work."""
     f = DatasetFile(path='file', creator=creators)
-    creator = Creator(name='me', email='me@example.com')
+    creator = Person(name='me', email='me@example.com')
     assert creator in f.creator
 
     # email check
     with pytest.raises(ValueError):
-        Creator(name='me', email='meexample.com')
+        Person(name='me', email='meexample.com')
 
-    # creators must be a set or list of dicts or Creator
+    # creators must be a set or list of dicts or Person
     with pytest.raises(ValueError):
         f = DatasetFile(path='file', creator=['name'])