Skip to content

Commit

Permalink
feat: Several improvements for caching of diagram cache
Browse files Browse the repository at this point in the history
This commit requests changes on the PR #1689. In particular, the following has
changed:

- Replace cache key from path and revision with git_model_id (database id). The
  unique database ID of the Git model in the Collaboration Manager has to be
  part of the cache key. For safety reasons, I'd not share a cache between
  projects even though they link to the same repository. This doesn't really
  matter currently since we make new requests against the Git API every time,
  but we can't assure that for the future.
  - An attack could look like:
  - Project 1 links Git repository "https://example.com" to a model.
  - Project 2 links the same Git repository to a model with a different token.
  - The token of project 1 expires. Project 2 keeps updating the cache with a
    valid token.
  - Users of project 1 can still access the cache for project 2 without a valid
    token.
- Since a git_model only has one revision, it's safe to drop the revision for
  artifacts. The revision is only relevant for files from the repository. In
  those cases, the value was passed as parameter every time. Therefore, I've
  removed it from the Cache object.
- For license reasons, I switched from Redis to Valkey.
- Use a Kubernetes secret for the valkey password.
- For REST compatibility, I've changed the resource name of "empty_cache" to
  "cache".
- I've reenabled the model badge error handling.
- Added a new function loadDiagramCacheMetadata in
  model-diagram-dialog.component.ts to reduce duplicated code.
- Added the job id to the diagram cache code snippet.
- Changed the styling of the "Clear cache" button.
  • Loading branch information
MoritzWeber0 authored and dominik003 committed Sep 16, 2024
1 parent b60dfb2 commit 4b71fa9
Show file tree
Hide file tree
Showing 27 changed files with 162 additions and 146 deletions.
1 change: 0 additions & 1 deletion .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ repos:
- capellambse
- typer
- types-lxml
- types-redis
- repo: local
hooks:
- id: pylint
Expand Down
16 changes: 7 additions & 9 deletions backend/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,7 @@ VENV = .venv

HOST ?= 127.0.0.1

REDIS_PORT = 6379
REDIS_INSIGHT_PORT = 8001
VALKEY_PORT = 6379

DATABASE_LOAD_FILE ?= ../local/load.sql
DATABASE_SAVE_DIR ?= ../local
Expand All @@ -34,13 +33,12 @@ database:
-e POSTGRES_DB=$(DB_NAME) \
postgres

redis:
docker start redis || \
valkey:
docker start valkey || \
docker run -d \
--name redis \
-p $(REDIS_PORT):6379 \
-p $(REDIS_INSIGHT_PORT):8001 \
redis/redis-stack:latest
--name valkey \
-p $(VALKEY_PORT):6379 \
valkey/valkey:latest

app:
if [ -d "$(VENV)/bin" ]; then
Expand Down Expand Up @@ -68,7 +66,7 @@ install:
openapi:
CAPELLACOLLAB_SKIP_OPENAPI_ERROR_RESPONSES=1 $(VENV)/bin/python -m capellacollab.cli openapi generate /tmp/openapi.json

dev: database app
dev: database valkey app

cleanup:
docker stop capella-collab-postgres
Expand Down
6 changes: 3 additions & 3 deletions backend/capellacollab/config/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -288,8 +288,8 @@ class DatabaseConfig(BaseConfig):
)


class RedisConfig(BaseConfig):
url: str = pydantic.Field(default="redis://localhost:6379/0")
class ValkeyConfig(BaseConfig):
url: str = pydantic.Field(default="valkey://localhost:6379/0")


class InitialConfig(BaseConfig):
Expand Down Expand Up @@ -342,7 +342,7 @@ class AppConfig(BaseConfig):
authentication: AuthenticationConfig = AuthenticationConfig()
prometheus: PrometheusConfig = PrometheusConfig()
database: DatabaseConfig = DatabaseConfig()
redis: RedisConfig = RedisConfig()
valkey: ValkeyConfig = ValkeyConfig()
initial: InitialConfig = InitialConfig()
logging: LoggingConfig = LoggingConfig()
requests: RequestsConfig = RequestsConfig()
Expand Down
6 changes: 3 additions & 3 deletions backend/capellacollab/core/database/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@
import typing as t

import pydantic
import redis
import sqlalchemy as sa
import valkey
from sqlalchemy import orm
from sqlalchemy.dialects import postgresql

Expand Down Expand Up @@ -38,8 +38,8 @@ def get_db() -> t.Iterator[orm.Session]:


@functools.lru_cache
def get_redis() -> redis.Redis:
return redis.Redis.from_url(config.redis.url, decode_responses=True)
def get_valkey() -> valkey.Valkey:
return valkey.Valkey.from_url(config.valkey.url, decode_responses=True)


def patch_database_with_pydantic_object(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,9 @@ async def get_model_complexity_badge(
logger: logging.LoggerAdapter = fastapi.Depends(log.get_request_logger),
):
try:
file = await git_handler.get_file("model-complexity-badge.svg")
file = await git_handler.get_file(
"model-complexity-badge.svg", git_handler.revision
)
return responses.SVGResponse(content=file[1])
except Exception:
logger.debug(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,16 @@
from capellacollab.core import database


class GitRedisCache:
def __init__(self, path: str, revision: str) -> None:
self._redis = database.get_redis()
self.path = path.replace(":", "-")
self.revision = revision
class GitValkeyCache:
def __init__(self, git_model_id: int) -> None:
self._valkey = database.get_valkey()
self.git_model_id = git_model_id
super().__init__()

def get_file_data(
self, file_path: str, revision: str | None = None
self, file_path: str, revision: str
) -> tuple[datetime.datetime, bytes] | None:
revision = revision or self.revision

file_data = self._redis.hmget(
file_data = self._valkey.hmget(
name=self._get_file_key(file_path, revision),
keys=["last_updated", "content"],
)
Expand All @@ -31,7 +28,7 @@ def get_file_data(
def get_artifact_data(
self, job_id: str | int, file_path: str
) -> tuple[datetime.datetime, bytes] | None:
artifact_data = self._redis.hmget(
artifact_data = self._valkey.hmget(
name=self._get_artifact_key(job_id, file_path),
keys=["started_at", "content"],
)
Expand All @@ -46,19 +43,17 @@ def put_file_data(
file_path: str,
last_updated: datetime.datetime,
content: bytes,
revision: str | None = None,
revision: str,
ttl: int = 3600,
) -> None:
revision = revision or self.revision

self._redis.hset(
self._valkey.hset(
name=self._get_file_key(file_path, revision),
mapping={
"last_updated": last_updated.isoformat(),
"content": content,
},
)
self._redis.expire(
self._valkey.expire(
name=self._get_file_key(file_path, revision), time=ttl
)

Expand All @@ -70,26 +65,25 @@ def put_artifact_data(
content: bytes,
ttl: int = 3600,
) -> None:
self._redis.hset(
self._valkey.hset(
name=self._get_artifact_key(job_id, file_path),
mapping={"started_at": started_at.isoformat(), "content": content},
)
self._redis.expire(
self._valkey.expire(
name=self._get_artifact_key(job_id, file_path), time=ttl
)

def clear(self, ignore_revision: bool = False) -> None:
pattern = f"{self.path}:{self.revision}:*"
if ignore_revision:
pattern = f"{self.path}:*"

for key in self._redis.scan_iter(match=pattern):
self._redis.delete(key)
def clear(self) -> None:
for key in self._valkey.scan_iter(match=f"{self.git_model_id}:*"):
self._valkey.delete(key)

def _get_file_key(self, file_path: str, revision: str) -> str:
file_path = file_path.replace(":", "-")
return f"{self.path}:{revision}:f:{file_path}"
return f"{self.git_model_id}:f:{self._escape_string(revision)}:{self._escape_string(file_path)}"

def _get_artifact_key(self, job_id: str | int, file_path: str) -> str:
file_path = file_path.replace(":", "-")
return f"{self.path}:{self.revision}:a:{job_id}:{file_path}"
return (
f"{self.git_model_id}:a:{job_id}:{self._escape_string(file_path)}"
)

def _escape_string(self, string: str) -> str:
return string.replace(":", "-")
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ def _create_specific_git_handler(
match git_instance.type:
case settings_git_models.GitType.GITLAB:
return gitlab_handler.GitlabHandler(
git_model.id,
git_model.path,
git_model.revision,
git_model.password,
Expand All @@ -113,6 +114,7 @@ def _create_specific_git_handler(
)
case settings_git_models.GitType.GITHUB:
return github_handler.GithubHandler(
git_model.id,
git_model.path,
git_model.revision,
git_model.password,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
class GitHandler:
def __init__(
self,
git_model_id: int,
path: str,
revision: str,
password: str,
Expand All @@ -23,7 +24,7 @@ def __init__(
self.password = password
self.api_url = api_url
self.repository_id = repository_id
self.cache = cache.GitRedisCache(path, revision)
self.cache = cache.GitValkeyCache(git_model_id)

@classmethod
@abc.abstractmethod
Expand Down Expand Up @@ -114,7 +115,7 @@ def get_started_at_for_job(self, job_id: str | int) -> datetime.datetime:
"""

async def get_file(
self, trusted_file_path: str, revision: str | None = None
self, trusted_file_path: str, revision: str
) -> tuple[datetime.datetime, bytes]:
last_updated = self.get_last_updated_for_file(
trusted_file_path, revision
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,13 +147,13 @@ def update_git_model_by_id(
return crud.update_git_model(db, db_git_model, put_git_model)


@router.delete("/empty_cache")
@router.delete("/cache")
def empty_cache(
git_model: models.DatabaseGitModel = fastapi.Depends(
injectables.get_existing_primary_git_model
),
):
cache.GitRedisCache(git_model.path, git_model.revision).clear()
cache.GitValkeyCache(git_model_id=git_model.id).clear()


@router.delete(
Expand Down
5 changes: 2 additions & 3 deletions backend/pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,7 @@ dependencies = [
"argon2-cffi",
"typer",
"lxml",
"redis",
"redis[hiredis]",
"valkey[libvalkey]",
]

[project.urls]
Expand All @@ -67,7 +66,6 @@ dev = [
"pytest-cov",
"aioresponses",
"types-lxml",
"types-redis",
]

[tool.black]
Expand Down Expand Up @@ -128,6 +126,7 @@ module = [
"argon2.*",
"websocket.*",
"testcontainers.*",
"valkey.*",
]
ignore_missing_imports = true

Expand Down
10 changes: 5 additions & 5 deletions backend/tests/projects/toolmodels/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@
from capellacollab.core import credentials


@pytest.fixture(name="mock_git_redis_cache")
def fixture_mock_git_redis_cache(monkeypatch: pytest.MonkeyPatch):
class MockGitRedisCache:
@pytest.fixture(name="mock_git_valkey_cache")
def fixture_mock_git_valkey_cache(monkeypatch: pytest.MonkeyPatch):
class MockGitValkeyCache:
def __init__(self, *args, **kwargs) -> None:
super().__init__()

Expand All @@ -42,8 +42,8 @@ def clear(self) -> None:
pass

monkeypatch.setattr(
"capellacollab.projects.toolmodels.modelsources.git.handler.cache.GitRedisCache",
MockGitRedisCache,
"capellacollab.projects.toolmodels.modelsources.git.handler.cache.GitValkeyCache",
MockGitValkeyCache,
)


Expand Down
10 changes: 5 additions & 5 deletions backend/tests/projects/toolmodels/test_diagrams.py
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ def fixture_mock_gitlab_diagram_cache_svg(git_type: git_models.GitType):
"mock_git_rest_api_for_artifacts",
"mock_git_diagram_cache_index_api",
"mock_git_get_commit_information_api",
"mock_git_redis_cache",
"mock_git_valkey_cache",
)
def test_get_diagram_metadata_from_repository(
project: project_models.DatabaseProject,
Expand Down Expand Up @@ -216,7 +216,7 @@ def test_get_diagram_metadata_from_repository(
"mock_git_rest_api_for_artifacts",
"mock_git_diagram_cache_index_api",
"mock_git_get_commit_information_api",
"mock_git_redis_cache",
"mock_git_valkey_cache",
)
def test_get_diagram_metadata_from_artifacts(
project: project_models.DatabaseProject,
Expand Down Expand Up @@ -423,7 +423,7 @@ def test_get_diagrams_failed_diagram_cache_job_found(
"mock_git_diagram_cache_index_api",
"mock_git_diagram_cache_svg",
"mock_git_get_commit_information_api",
"mock_git_redis_cache",
"mock_git_valkey_cache",
)
@pytest.mark.usefixtures("project_user", "git_instance", "git_model")
def test_get_single_diagram_from_artifacts(
Expand Down Expand Up @@ -482,7 +482,7 @@ def test_get_single_diagram_from_artifacts(
"mock_git_diagram_cache_index_api",
"mock_git_diagram_cache_svg",
"mock_git_get_commit_information_api",
"mock_git_redis_cache",
"mock_git_valkey_cache",
)
@pytest.mark.usefixtures("project_user", "git_instance", "git_model")
def test_get_single_diagram_from_artifacts_with_file_ending(
Expand Down Expand Up @@ -596,7 +596,7 @@ def test_get_single_diagram_from_artifacts_with_wrong_file_ending(
"mock_git_diagram_cache_index_api",
"mock_git_diagram_cache_svg",
"mock_git_get_commit_information_api",
"mock_git_redis_cache",
"mock_git_valkey_cache",
)
@pytest.mark.usefixtures("project_user", "git_instance", "git_model")
def test_get_single_diagram_from_repository(
Expand Down
4 changes: 2 additions & 2 deletions backend/tests/projects/toolmodels/test_model_badge.py
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ def test_get_model_badge_fails_without_api_endpoint(
"mock_git_rest_api",
"mock_git_model_badge_file_api",
"mock_git_get_commit_information_api",
"mock_git_redis_cache",
"mock_git_valkey_cache",
)
def test_get_model_badge(
project: project_models.DatabaseProject,
Expand Down Expand Up @@ -233,7 +233,7 @@ def test_get_model_badge(
"mock_git_model_badge_file_api_not_found",
"mock_get_model_badge_from_artifacts_api",
"mock_git_get_commit_information_api",
"mock_git_redis_cache",
"mock_git_valkey_cache",
)
def test_get_model_badge_from_artifacts(
project: project_models.DatabaseProject,
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading

0 comments on commit 4b71fa9

Please sign in to comment.