fix: use project_id as part of project filesystem path (#1754)

conftest.py

@@ -960,23 +960,28 @@ def svc_client_cache(mock_redis, identity_headers):
     ctx.pop()
 
 
-def integration_repo_path(headers, url_components):
+def integration_repo_path(headers, project_id, url_components):
     """Constructs integration repo path."""
-    from renku.service.config import CACHE_PROJECTS_PATH
     from renku.service.serializers.headers import UserIdentityHeaders
+    from renku.service.utils import make_project_path
 
     user = UserIdentityHeaders().load(headers)
-    project_path = CACHE_PROJECTS_PATH / user["user_id"] / url_components.owner / url_components.name
+    project = {
+        "project_id": project_id,
+        "owner": url_components.owner,
+        "name": url_components.name,
+    }
 
+    project_path = make_project_path(user, project)
     return project_path
 
 
 @contextlib.contextmanager
-def integration_repo(headers, url_components):
+def integration_repo(headers, project_id, url_components):
     """With integration repo helper."""
     from renku.core.utils.contexts import chdir
 
-    with chdir(integration_repo_path(headers, url_components)):
+    with chdir(integration_repo_path(headers, project_id, url_components)):
         repo = Repo(".")
         yield repo
 
@@ -1029,17 +1034,16 @@ def integration_lifecycle(svc_client, mock_redis, identity_headers):
     response = svc_client.post("/cache.project_clone", data=json.dumps(payload), headers=identity_headers,)
 
     assert response
-    assert "result" in response.json
-    assert "error" not in response.json
+    assert {"result"} == set(response.json.keys())
 
     project_id = response.json["result"]["project_id"]
     assert isinstance(uuid.UUID(project_id), uuid.UUID)
 
     yield svc_client, identity_headers, project_id, url_components
 
     # Teardown step: Delete all branches except master (if needed).
-    if integration_repo_path(identity_headers, url_components).exists():
-        with integration_repo(identity_headers, url_components) as repo:
+    if integration_repo_path(identity_headers, project_id, url_components).exists():
+        with integration_repo(identity_headers, project_id, url_components) as repo:
             try:
                 repo.remote().push(refspec=(":{0}".format(repo.active_branch.name)))
             except GitCommandError:
@@ -1051,7 +1055,7 @@ def svc_client_setup(integration_lifecycle):
     """Service client setup."""
     svc_client, headers, project_id, url_components = integration_lifecycle
 
-    with integration_repo(headers, url_components) as repo:
+    with integration_repo(headers, project_id, url_components) as repo:
         repo.git.checkout("master")
 
         new_branch = uuid.uuid4().hex

renku/service/cache/models/project.py

@@ -49,7 +49,7 @@ class Project(Model):
     @property
     def abs_path(self):
         """Full path of cached project."""
-        return CACHE_PROJECTS_PATH / self.user_id / self.owner / self.name
+        return CACHE_PROJECTS_PATH / self.user_id / self.project_id / self.owner / self.name
 
     @property
     def age(self):

renku/service/controllers/cache_list_projects.py

@@ -16,6 +16,8 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 """Renku service cache list cached projects controller."""
+import itertools
+
 from renku.service.controllers.api.abstract import ServiceCtrl
 from renku.service.controllers.api.mixins import ReadOperationMixin
 from renku.service.serializers.cache import ProjectListResponseRPC
@@ -40,7 +42,14 @@ def context(self):
     def list_projects(self):
         """List locally cache projects."""
         projects = [project for project in self.cache.get_projects(self.user) if project.abs_path.exists()]
-        return {"projects": projects}
+
+        result = {
+            "projects": [
+                max(g, key=lambda p: p.created_at) for _, g in itertools.groupby(projects, lambda p: p.git_url)
+            ]
+        }
+
+        return result
 
     def renku_op(self):
         """Renku operation for the controller."""

renku/service/controllers/templates_create_project.py

@@ -28,7 +28,7 @@
 from renku.service.controllers.api.mixins import ReadOperationMixin
 from renku.service.controllers.utils.project_clone import user_project_clone
 from renku.service.serializers.templates import ProjectTemplateRequest, ProjectTemplateResponseRPC
-from renku.service.utils import make_new_project_path, new_repo_push
+from renku.service.utils import new_repo_push
 from renku.service.views import result_response
 
 
@@ -80,13 +80,26 @@ def git_user(self):
 
     def setup_new_project(self):
         """Setup new project for initialization."""
-        new_project_path = make_new_project_path(self.user_data, self.ctx)
+        # TODO: Request attribute naming on create project and read manifest is not consistent.
+        new_project_data = {
+            "clone_depth": self.ctx["depth"],
+            "git_url": self.ctx["new_project_url"],
+            "name": self.ctx["project_name_stripped"],
+            "fullname": self.ctx["fullname"],
+            "email": self.ctx["email"],
+            "owner": self.ctx["owner"],
+            "token": self.ctx["token"],
+            "initialized": True,
+        }
+        project = self.cache.make_project(self.user, new_project_data)
 
+        new_project_path = project.abs_path
         if new_project_path.exists():
             shutil.rmtree(new_project_path)
+
         new_project_path.mkdir(parents=True, exist_ok=True)
 
-        return new_project_path
+        return project
 
     def setup_template(self):
         """Reads template manifest."""
@@ -115,7 +128,8 @@ def new_project_push(self, project_path):
     def new_project(self):
         """Create new project from template."""
         template_project, provided_parameters = self.setup_template()
-        new_project_path = self.setup_new_project()
+        new_project = self.setup_new_project()
+        new_project_path = new_project.abs_path
 
         source_path = template_project.abs_path / self.ctx["identifier"]
 
@@ -133,6 +147,7 @@ def new_project(self):
                 self.ctx["ref"],
                 "service",
             )
+
         self.new_project_push(new_project_path)
 
         return {

renku/service/controllers/utils/project_clone.py

@@ -16,46 +16,32 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 """Utilities for renku service controllers."""
-import shutil
-
 from renku.core.commands.clone import project_clone
-from renku.core.errors import RenkuException
 from renku.service.logger import service_log
-from renku.service.utils import make_project_path
 from renku.service.views.decorators import requires_cache
 
 
 @requires_cache
 def user_project_clone(cache, user_data, project_data):
     """Clones the project for a given user."""
-    local_path = make_project_path(user_data, project_data)
-    if local_path is None:
-        raise RenkuException("project not found")
+    if "project_id" in project_data:
+        project_data.pop("project_id")
 
     user = cache.ensure_user(user_data)
+    project = cache.make_project(user, project_data)
+    project.abs_path.mkdir(parents=True, exist_ok=True)
 
-    if local_path.exists():
-        shutil.rmtree(local_path)
-
-        for project in cache.get_projects(user):
-            if project.abs_path == local_path:
-                project.delete()
-
-        if "project_id" in project_data:
-            project_data.pop("project_id")
-
-    repo, initialized = project_clone(
+    repo, project.initialized = project_clone(
         project_data["url_with_auth"],
-        local_path,
+        project.abs_path,
         depth=project_data["depth"] if project_data["depth"] != 0 else None,
         raise_git_except=True,
         config={"user.name": project_data["fullname"], "user.email": project_data["email"],},
         checkout_rev=project_data["ref"],
     )
-    project_data["initialized"] = initialized
+    project.save()
 
     service_log.debug(f"project successfully cloned: {repo}")
-    service_log.debug(f"project folder exists: {local_path.exists()}")
+    service_log.debug(f"project folder exists: {project.exists()}")
 
-    project = cache.make_project(user, project_data)
     return project

renku/service/utils/__init__.py

@@ -25,20 +25,10 @@
 def make_project_path(user, project):
     """Construct full path for cached project."""
     valid_user = user and "user_id" in user
-    valid_project = project and "owner" in project and "name" in project
+    valid_project = project and "owner" in project and "name" in project and "project_id" in project
 
     if valid_user and valid_project:
-        return CACHE_PROJECTS_PATH / user["user_id"] / project["owner"] / project["name"]
-
-
-def make_new_project_path(user, project):
-    """Adjust parameters new project path."""
-    new_project = {
-        "owner": project["project_namespace"],
-        "name": project["project_name_stripped"],
-    }
-
-    return make_project_path(user, new_project)
+        return CACHE_PROJECTS_PATH / user["user_id"] / project["project_id"] / project["owner"] / project["name"]
 
 
 def make_file_path(user, cached_file):

renku/service/views/decorators.py

@@ -96,6 +96,7 @@ def decorated_function(*args, **kwargs):
         try:
             return f(*args, **kwargs)
         except (RedisError, OSError) as e:
+            # NOTE: Trap the process working directory when internal error occurs.
             try:
                 set_context("pwd", os.readlink(f"/proc/{os.getpid()}/cwd"))
             except (Exception, BaseException):

tests/service/controllers/test_templates_create_project.py

@@ -51,7 +51,6 @@ def test_template_create_project_ctrl(ctrl_init, svc_client_templates_creation,
         "project_repository",
         "url",
         "identifier",
-        "initialized",
         "parameters",
         "project_name",
         "name",
@@ -80,7 +79,6 @@ def test_template_create_project_ctrl(ctrl_init, svc_client_templates_creation,
         "__project_slug__",
     }
     assert expected_metadata == set(received_metadata.keys())
-
     assert payload["url"] == received_metadata["__template_source__"]
     assert payload["ref"] == received_metadata["__template_ref__"]
     assert payload["identifier"] == received_metadata["__template_id__"]

tests/service/controllers/utils/test_project_clone.py

@@ -49,30 +49,25 @@ def test_service_user_project_clone(svc_client_cache):
 
     project_data = ManifestTemplatesRequest().load({**user_data, **project_data}, unknown=EXCLUDE)
     project_one = user_project_clone(user_data, project_data)
-    time.sleep(1)
-
-    assert project_one.age == 1
+    assert project_one.age >= 0
     assert not project_one.ttl_expired()
     assert project_one.exists()
     old_path = project_one.abs_path
 
     os.environ["RENKU_SVC_CLEANUP_TTL_PROJECTS"] = "1"
     time.sleep(1)
-    assert project_one.age == 2
     assert project_one.ttl_expired()
 
     os.environ["RENKU_SVC_CLEANUP_TTL_PROJECTS"] = "3600"
-    project_two = user_project_clone(user_data, project_data)
-    time.sleep(1)
 
-    assert project_two.age == 1
+    project_two = user_project_clone(user_data, project_data)
+    assert project_two.age >= 0
     assert not project_two.ttl_expired()
     assert project_two.exists()
 
     new_path = project_two.abs_path
-    assert old_path == new_path
+    assert old_path != new_path
     user = cache.get_user(user_data["user_id"])
-
     projects = [project.project_id for project in cache.get_projects(user)]
-    assert project_one.project_id not in projects
+    assert project_one.project_id in projects
     assert project_two.project_id in projects

tests/service/jobs/test_datasets.py

@@ -59,7 +59,9 @@ def test_dataset_url_import_job(url, svc_client_with_repo):
     assert_rpc_response(response)
     assert {"job_id", "created_at"} == set(response.json["result"].keys())
 
-    dest = make_project_path(user_data, {"owner": url_components.owner, "name": url_components.name})
+    dest = make_project_path(
+        user_data, {"owner": url_components.owner, "name": url_components.name, "project_id": project_id}
+    )
 
     old_commit = Repo(dest).head.commit
     job_id = response.json["result"]["job_id"]
@@ -100,7 +102,9 @@ def test_dataset_import_job(doi, svc_client_with_repo):
     assert_rpc_response(response)
     assert {"job_id", "created_at"} == set(response.json["result"].keys())
 
-    dest = make_project_path(user, {"owner": url_components.owner, "name": url_components.name})
+    dest = make_project_path(
+        user, {"owner": url_components.owner, "name": url_components.name, "project_id": project_id}
+    )
 
     old_commit = Repo(dest).head.commit
     job_id = response.json["result"]["job_id"]
@@ -148,7 +152,9 @@ def test_dataset_import_junk_job(doi, expected_err, svc_client_with_repo):
     assert_rpc_response(response)
     assert {"job_id", "created_at"} == set(response.json["result"].keys())
 
-    dest = make_project_path(user, {"owner": url_components.owner, "name": url_components.name})
+    dest = make_project_path(
+        user, {"owner": url_components.owner, "name": url_components.name, "project_id": project_id}
+    )
 
     old_commit = Repo(dest).head.commit
     job_id = response.json["result"]["job_id"]
@@ -191,7 +197,9 @@ def test_dataset_import_twice_job(doi, svc_client_with_repo):
     assert_rpc_response(response)
     assert {"job_id", "created_at"} == set(response.json["result"].keys())
 
-    dest = make_project_path(user, {"owner": url_components.owner, "name": url_components.name})
+    dest = make_project_path(
+        user, {"owner": url_components.owner, "name": url_components.name, "project_id": project_id}
+    )
 
     old_commit = Repo(dest).head.commit
     job_id = response.json["result"]["job_id"]
@@ -240,7 +248,9 @@ def test_dataset_add_remote_file(url, svc_client_with_repo):
     assert_rpc_response(response)
     assert {"files", "name", "project_id", "remote_branch"} == set(response.json["result"].keys())
 
-    dest = make_project_path(user, {"owner": url_components.owner, "name": url_components.name})
+    dest = make_project_path(
+        user, {"owner": url_components.owner, "name": url_components.name, "project_id": project_id}
+    )
     old_commit = Repo(dest).head.commit
     job_id = response.json["result"]["files"][0]["job_id"]
     commit_message = "service: dataset add remote file"
@@ -272,7 +282,9 @@ def test_dataset_project_lock(doi, svc_client_with_repo):
     assert_rpc_response(response)
     assert {"job_id", "created_at"} == set(response.json["result"].keys())
 
-    dest = make_project_path(user, {"owner": url_components.owner, "name": url_components.name})
+    dest = make_project_path(
+        user, {"owner": url_components.owner, "name": url_components.name, "project_id": project_id}
+    )
 
     old_commit = Repo(dest).head.commit
 

tests/service/jobs/test_jobs.py

@@ -214,20 +214,19 @@ def test_project_cleanup_success(svc_client_cache):
     project_data = ManifestTemplatesRequest().load({**user_data, **project_data}, unknown=EXCLUDE)
     assert "user_id" not in project_data.keys()
     project_one = user_project_clone(user_data, project_data)
-    time.sleep(1)
 
-    assert project_one.age == 1
+    assert project_one.age >= 0
     assert not project_one.ttl_expired()
     assert project_one.exists()
 
     os.environ["RENKU_SVC_CLEANUP_TTL_PROJECTS"] = "1"
     time.sleep(1)
-    assert project_one.age == 2
+
+    assert project_one.age >= 1
     assert project_one.ttl_expired()
 
     cache_project_cleanup()
 
-    project_data.pop("project_id")
     project_data = ManifestTemplatesRequest().load({**user_data, **project_data}, unknown=EXCLUDE)
     assert "user_id" not in project_data.keys()
     user = cache.get_user(user_data["user_id"])
@@ -236,9 +235,8 @@ def test_project_cleanup_success(svc_client_cache):
 
     project_two = user_project_clone(user_data, project_data)
     os.environ["RENKU_SVC_CLEANUP_TTL_PROJECTS"] = "1800"
-    time.sleep(1)
 
-    assert project_two.age == 1
+    assert project_two.age >= 0
     assert not project_two.ttl_expired()
     assert project_two.exists()