From 336c3415fec204c1ed3b977f6b36e1877ed27e80 Mon Sep 17 00:00:00 2001 From: AbuJulaybeeb Date: Sun, 18 Oct 2026 21:40:01 +0100 Subject: [PATCH] feat: ship logs to Loki, audit PR template, add test:changed script, and song version rollback - Ship structured logs to Grafana Loki via Promtail sidecar and direct HTTP log shipper, with strict redaction of sensitive credentials (Closes #380) - Audit and enhance PR template (.github/PULL_REQUEST_TEMPLATE.md) with prompts for test plan evidence, breaking changes, and linked issues (Closes #386) - Add 'npm run test:changed' script using Jest '--changedSince=main' and document usage in README and CONTRIBUTING (Closes #397) - Add song version rollback endpoint 'POST /api/song/:id/versions/:version/rollback' preserving revision history for owning artists (Closes #441) --- .github/PULL_REQUEST_TEMPLATE.md | 55 ++++-- CONTRIBUTING.md | 4 + README.md | 16 +- docker-compose.yml | 31 ++++ docs/logging-and-aggregation.md | 109 +++++++++++ docs/openapi.yaml | 35 ++++ monitoring/README.md | 12 +- monitoring/grafana/provisioning.yml | 8 + monitoring/promtail.yml | 34 ++++ package-lock.json | 28 ++- package.json | 1 + src/__tests__/SongRoutes.test.ts | 45 ++++- src/__tests__/SongRoutesOrder.test.ts | 18 +- src/config/__tests__/logger.test.ts | 86 +++++++++ src/config/logger.ts | 170 +++++++++++++++--- src/controllers/SongController.ts | 19 ++ src/routes/SongRoutes.ts | 49 ++++- src/services/Song/SongVersionService.ts | 87 +++++++++ src/services/SongService.ts | 7 +- .../__tests__/SongVersionService.test.ts | 71 ++++++++ 20 files changed, 821 insertions(+), 64 deletions(-) create mode 100644 docs/logging-and-aggregation.md create mode 100644 monitoring/promtail.yml create mode 100644 src/config/__tests__/logger.test.ts diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index acd3579..5fb6af4 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -1,17 +1,50 @@ ## Description -Please include a summary of the change and which issue is fixed. -Fixes # (issue) +Please include a summary of the change, the problem it solves, and why it is needed. -## Type of change -- [ ] Bug fix -- [ ] New feature -- [ ] Breaking change -- [ ] Documentation update +Fixes / Closes #(issue) -## Checklist: -- [ ] My code follows the coding conventions +## Type of Change + +Select all that apply and ensure the matching PR label is set per [CONTRIBUTING.md](../CONTRIBUTING.md#pr-labels): + +- [ ] `bug` / `fix`: Bug fix (non-breaking change which fixes an issue) +- [ ] `feature` / `enhancement`: New feature (non-breaking change which adds functionality) +- [ ] `breaking-change`: Breaking change (fix or feature that causes existing functionality to not work as expected) +- [ ] `documentation` / `docs`: Documentation update only +- [ ] `performance`: Performance improvement +- [ ] `security`: Security hardening or fix +- [ ] `chore` / `refactor`: Refactoring, dependencies, or maintenance + +## Breaking Changes Disclosure + +Does this PR introduce any breaking change to APIs, database schemas, environment variables, or workflows? + +- [ ] **No** breaking changes are introduced. +- [ ] **Yes**, this PR introduces breaking changes. + +_If yes, describe the breaking impact, reasoning, and migration steps for consumers/operators:_ + + +## Test Plan & Verification + +Explain how you tested these changes. Provide commands executed, automated test files run, and manual verification results. + +### Automated Tests +- Command(s) run: `npm test` / `npm run test:changed` / `npm run lint` +- Relevant test file(s): + +### Manual Verification +- Steps taken to verify in local or staging environment: +- Verification results / evidence: + +## Checklist + +- [ ] My code follows the repository's coding style and conventions (`npm run lint`, `npm run format:check`) - [ ] I have performed a self-review of my own code -- [ ] I have commented my code, particularly in hard-to-understand areas -- [ ] I have added tests that prove my fix is effective or that my feature works +- [ ] I have added or updated tests that prove my fix is effective or that my feature works - [ ] New and existing unit tests pass locally with my changes +- [ ] I have commented my code, particularly in hard-to-understand or security-sensitive areas +- [ ] I have updated corresponding documentation (`README.md`, `docs/`, `openapi.yaml`) where applicable +- [ ] I have confirmed no secrets, private keys, or credentials are committed +- [ ] I have applied the appropriate PR label per [CONTRIBUTING.md](../CONTRIBUTING.md#pr-labels) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 6e409b5..2166e00 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -46,7 +46,11 @@ npm run dev Run the test suite before pushing: ```bash +# Full test suite (CI default) npm test + +# Run only tests affected by changed files (fast local iteration) +npm run test:changed ``` ### Pre-commit Hooks diff --git a/README.md b/README.md index acada9a..412363b 100644 --- a/README.md +++ b/README.md @@ -658,6 +658,7 @@ Additional in-repo docs live under [`docs/`](docs/): - [Deployment & Scaling Guide](docs/deployment-and-scaling.md) — expected CPU/memory sizing for the API and the ffmpeg-backed worker, how to measure it, and how to scale each process (Issue #405) +- [Logging & Aggregation Guide](docs/logging-and-aggregation.md) — centralized log shipping to Grafana Loki via Promtail, LogQL queries, and sensitive data redaction (Issue #380) - [Architecture](docs/ARCHITECTURE.md) — high-level module layout - [AI Feature Set](docs/AI_FEATURES.md) — AI capabilities, what data is sent where, and the per-artist opt-in/opt-out story @@ -698,18 +699,18 @@ This brings up the **minimal stack** (API `localhost:4000` + Postgres `5432` + R | Command | What it starts | | --------------------------------------------------- | ----------------------------------------------------------------------- | -| `docker compose up --build` | `backend` + `db` + `redis` (minimal, no profile) | -| `docker compose --profile full up --build` | Everything: minimal + `rabbitmq` + `pgadmin` + `prometheus` + `grafana` | -| `docker compose --profile queue up --build` | Minimal + `rabbitmq` (song-processing queue) | -| `docker compose --profile tools up --build` | Minimal + `pgadmin` (`localhost:5050`) | -| `docker compose --profile monitoring up --build` | Minimal + `prometheus` (`9090`) + `grafana` (`3000`) | -| `docker compose --profile observability up --build` | Same as `monitoring` (alias) | +| `docker compose up --build` | `backend` + `db` + `redis` (minimal, no profile) | +| `docker compose --profile full up --build` | Everything: minimal + `rabbitmq` + `pgadmin` + `prometheus` + `grafana` + `loki` + `promtail` | +| `docker compose --profile queue up --build` | Minimal + `rabbitmq` (song-processing queue) | +| `docker compose --profile tools up --build` | Minimal + `pgadmin` (`localhost:5050`) | +| `docker compose --profile monitoring up --build` | Minimal + `prometheus` (`9090`) + `grafana` (`3000`) + `loki` (`3100`) + `promtail` | +| `docker compose --profile observability up --build` | Same as `monitoring` (alias) | `backend` no longer `depends_on: rabbitmq` — the API starts without a queue and connects to RabbitMQ lazily when it appears (see `src/index.ts`), so the minimal stack stays self-contained. Any combination of profiles can be stacked, e.g.: ```bash docker compose --profile queue --profile monitoring up --build -# → backend + db + redis + rabbitmq + prometheus + grafana +# → backend + db + redis + rabbitmq + prometheus + grafana + loki + promtail ``` The full observability stack (`--profile full`) is unchanged from the pre-profile `docker compose up` topology and is what CI and `docker-compose.prod.yml` extend. @@ -757,6 +758,7 @@ npm run dev | `npm run migration:run` | Applies pending migrations to the database | | `npm run migration:revert` | Reverts the last applied migration | | `npm test` | Runs the full Jest suite (`src/__tests__/**/*.test.ts`) | +| `npm run test:changed` | Runs only tests affected by files changed since `main` (for fast local iteration; Issue #397) | | `npm test -- src/__tests__/health.test.ts` | Runs a single test file (swap in any path under `src/__tests__`) | | `npm run test:watch` | Runs Jest in watch mode | | `npm run dev:bootstrap` | Bootstraps Docker dependencies, runs migrations, seeds genres and test accounts (Issue #369) | diff --git a/docker-compose.yml b/docker-compose.yml index 92929b9..4b5c800 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -157,6 +157,36 @@ services: - monitoring - observability + loki: + image: grafana/loki:latest + container_name: loki + restart: always + ports: + - '3100:3100' + command: -config.file=/etc/loki/local-config.yaml + volumes: + - loki_data:/loki + profiles: + - full + - monitoring + - observability + + promtail: + image: grafana/promtail:latest + container_name: promtail + restart: always + volumes: + - ./monitoring/promtail.yml:/etc/promtail/promtail.yml:ro + - /var/lib/docker/containers:/var/lib/docker/containers:ro + - /var/run/docker.sock:/var/run/docker.sock:ro + command: -config.file=/etc/promtail/promtail.yml + depends_on: + - loki + profiles: + - full + - monitoring + - observability + volumes: pgdata: redisdata: @@ -164,3 +194,4 @@ volumes: rabbitmq_data: prometheus_data: grafana_data: + loki_data: diff --git a/docs/logging-and-aggregation.md b/docs/logging-and-aggregation.md new file mode 100644 index 0000000..b6e3efa --- /dev/null +++ b/docs/logging-and-aggregation.md @@ -0,0 +1,109 @@ +# Structured Logging & Centralized Log Aggregation (Issue #380) + +## Overview + +AudioBlock Backend implements structured JSON logging using [Pino](https://getpino.io). All logs carry service metadata (`audioblock-backend`), runtime environment, ISO timestamps, request correlation IDs (`reqId`), and HTTP context. + +Centralized log aggregation is wired to **Grafana Loki** via a **Promtail sidecar** in Docker Compose and Kubernetes environments, and also supports **direct HTTP log shipping** via the `LOG_AGGREGATOR_URL` environment variable. + +--- + +## 1. Zero Sensitive Data Leakage (Redaction) + +To strictly enforce that **no credentials, tokens, or sensitive user data are shipped or persisted in logs**, Pino's native `redact` engine automatically sanitizes all matching fields before logs are emitted or shipped: + +- Passwords (`password`, `newPassword`, `oldPassword`, `confirmPassword`) +- Authentication tokens & headers (`token`, `accessToken`, `refreshToken`, `authorization`, `headers.authorization`) +- Secrets & keys (`apiKey`, `secret`, `jwtSecret`, `clientSecret`, `privateKey`, `twoFactorSecret`, `totpSecret`) +- Financial & PII attributes (`creditCard`, `cardNumber`, `cvv`, `ssn`) + +All matching values are replaced with `[REDACTED]`. + +--- + +## 2. Log Aggregation Topology + +### Local / Staging Docker Stack + +In `docker-compose.yml`, the observability stack provides: + +``` +┌─────────────────────────┐ +│ audioblocks-backend │ +│ (stdout structured JSON)│ +└────────────┬────────────┘ + │ Docker socket stream + ▼ +┌─────────────────────────┐ +│ Promtail │ +│ (parses JSON & labels) │ +└────────────┬────────────┘ + │ HTTP POST /loki/api/v1/push + ▼ +┌─────────────────────────┐ ┌─────────────────────────┐ +│ Grafana Loki │◄────────┤ Grafana │ +│ (centralized retention) │ │ (interactive log search)│ +└─────────────────────────┘ └─────────────────────────┘ +``` + +1. **Backend** emits single-line JSON logs to `stdout`. +2. **Promtail** sidecar automatically ingests container stdout via the Docker daemon socket (`unix:///var/run/docker.sock`), applies JSON parsing, and extracts labels (`service`, `level`, `statusCode`). +3. **Loki** indexes labels and stores compressed log chunks on the `loki_data` volume. +4. **Grafana** provisions Loki as a default log datasource (`http://loki:3100`), allowing developers to search, filter, and correlate logs against Prometheus metrics in Grafana Explore. + +--- + +## 3. How to Run Locally + +Start the minimal backend stack with the monitoring & logging profile: + +```bash +docker compose --profile monitoring up -d +# or full stack +docker compose --profile full up -d +``` + +Services started: +- Backend: `http://localhost:4000` +- Prometheus: `http://localhost:9090` +- Grafana Loki: `http://localhost:3100` +- Promtail: `http://localhost:9080` +- Grafana: `http://localhost:3000` (Login: `admin` / `admin`) + +--- + +## 4. Querying Logs in Grafana + +1. Open Grafana in your browser: `http://localhost:3000`. +2. Navigate to **Explore** (compass icon on the left menu). +3. Select datasource: **Loki**. +4. Run LogQL queries: + +```logql +# Stream all logs from AudioBlock backend +{container_name="audioblocks-backend"} + +# Filter by log level +{container_name="audioblocks-backend"} | json | level="error" + +# Search for specific request correlation ID +{container_name="audioblocks-backend"} |= "reqId" | json | reqId="" + +# Search for 5xx errors +{container_name="audioblocks-backend"} | json | statusCode >= 500 +``` + +--- + +## 5. Direct HTTP Log Shipping (Non-Docker / Serverless) + +If running outside Docker (e.g. bare metal or serverless) without a sidecar, direct log shipping can be enabled by setting: + +```bash +LOG_AGGREGATOR_URL=http://loki:3100/loki/api/v1/push +``` + +When `LOG_AGGREGATOR_URL` is set: +- Logs continue writing to `stdout` for local visibility. +- `logger` batches and asynchronously flushes log records in batches of up to 50 entries or every 2 seconds via `POST`. +- Aggregator outages fail open to prevent application degradation. diff --git a/docs/openapi.yaml b/docs/openapi.yaml index 479b347..d1a8f27 100644 --- a/docs/openapi.yaml +++ b/docs/openapi.yaml @@ -328,6 +328,41 @@ paths: description: The caller does not own the song. '404': description: Version not found. + /api/song/{id}/versions/{version}/rollback: + post: + summary: Roll back a song to a prior version + description: Owning artist only. Creates a new active version copying the specified historical revision's metadata and audio assets, preserving complete version history without mutation (Issue #441). + parameters: + - name: id + in: path + required: true + schema: + type: string + format: uuid + - name: version + in: path + required: true + description: 1-based historical version number to roll back to. + schema: + type: integer + minimum: 1 + requestBody: + required: false + content: + application/json: + schema: + type: object + properties: + changeNote: + type: string + description: Optional explanation of why the rollback was performed. + responses: + '201': + description: Created new version copying the target revision's fields. + '403': + description: The caller does not own the song. + '404': + description: Song or target version not found. /api/song/{id}/report: post: summary: Report a song diff --git a/monitoring/README.md b/monitoring/README.md index 749e73a..a88ce01 100644 --- a/monitoring/README.md +++ b/monitoring/README.md @@ -10,8 +10,9 @@ provisioned automatically on container startup. ``` monitoring/ ├── prometheus.yml # Prometheus scrape config (targets the backend /metrics) +├── promtail.yml # Promtail log shipping config (targets Loki) (Issue #380) ├── grafana/ -│ └── provisioning.yml # Grafana provisioning: Prometheus datasource + dashboards provider +│ └── provisioning.yml # Grafana provisioning: Prometheus & Loki datasources + dashboards provider └── dashboards/ ├── audioblock-app.json # Express app / HTTP / DB-pool / cache / royalties dashboard └── audioblock-node.json # Host system (CPU / memory / disk / network) overview @@ -19,7 +20,7 @@ monitoring/ ## How it works -`docker-compose.yml` starts a Prometheus instance and a Grafana instance. +`docker-compose.yml` starts Prometheus, Grafana, Grafana Loki, and Promtail. - Prometheus scrapes the backend's `GET /metrics` endpoint (exposed by `prom-client` in `src/services/MetricsService.ts`, served by @@ -28,10 +29,15 @@ monitoring/ - The standalone indexer worker serves its process-local metrics on port 9464; the `monitoring`, `observability`, and `full` Compose profiles start it and let Prometheus scrape `indexer:9464`. +- Log aggregation is handled by Grafana Loki and Promtail (Issue #380): + Promtail collects container stdout JSON logs, redacts any sensitive tokens, + and ships them to Loki at `http://loki:3100`. See + [`docs/logging-and-aggregation.md`](../docs/logging-and-aggregation.md) for + the complete runbook and LogQL query examples. - Grafana is provisioned entirely from disk: - `./monitoring/grafana/provisioning.yml` is mounted to `/etc/grafana/provisioning/provisioning.yml` — it defines the Prometheus - datasource and a file-based dashboard provider. + and Loki datasources and a file-based dashboard provider. - `./monitoring/dashboards/` is mounted to `/etc/grafana/provisioning/dashboards` — this is the provider's `path`, so every dashboard JSON in the directory is automatically loaded. diff --git a/monitoring/grafana/provisioning.yml b/monitoring/grafana/provisioning.yml index 7a3150e..568be80 100644 --- a/monitoring/grafana/provisioning.yml +++ b/monitoring/grafana/provisioning.yml @@ -14,6 +14,14 @@ datasources: url: http://prometheus:9090 isDefault: true + - name: Loki + type: loki + access: proxy + url: http://loki:3100 + isDefault: false + jsonData: + maxLines: 1000 + # Dashboard provider points at the version-controlled dashboards directory. providers: - name: "AudioBlock Backend" diff --git a/monitoring/promtail.yml b/monitoring/promtail.yml new file mode 100644 index 0000000..4b6c8a2 --- /dev/null +++ b/monitoring/promtail.yml @@ -0,0 +1,34 @@ +server: + http_listen_port: 9080 + grpc_listen_port: 0 + +positions: + filename: /tmp/positions.yaml + +clients: + - url: http://loki:3100/loki/api/v1/push + +scrape_configs: + - job_name: docker + docker_sd_configs: + - host: unix:///var/run/docker.sock + refresh_interval: 5s + relabel_configs: + - source_labels: ['__meta_docker_container_name'] + regex: '/(.*)' + target_label: 'container_name' + - source_labels: ['__meta_docker_container_label_service'] + target_label: 'service' + pipeline_stages: + - json: + expressions: + level: level + msg: msg + reqId: reqId + service: service + statusCode: statusCode + path: path + - labels: + level: + service: + statusCode: diff --git a/package-lock.json b/package-lock.json index 90ff08b..12682da 100644 --- a/package-lock.json +++ b/package-lock.json @@ -4819,6 +4819,17 @@ } } }, + "node_modules/@walletconnect/utils/node_modules/zod": { + "version": "3.25.76", + "resolved": "https://registry.npmjs.org/zod/-/zod-3.25.76.tgz", + "integrity": "sha512-gzUt/qt81nXsFGKIFcC3YnfEAx5NkunCfnDlvuBSSFS02bcXu4Lmea0AFIUwbLWxWPx3d9p8S5QoaujKcNQxcQ==", + "license": "MIT", + "optional": true, + "peer": true, + "funding": { + "url": "https://github.com/sponsors/colinhacks" + } + }, "node_modules/@walletconnect/window-getters": { "version": "1.0.1", "resolved": "https://registry.npmjs.org/@walletconnect/window-getters/-/window-getters-1.0.1.tgz", @@ -4963,6 +4974,7 @@ "version": "4.0.0", "resolved": "https://registry.npmjs.org/abbrev/-/abbrev-4.0.0.tgz", "integrity": "sha512-a1wflyaL0tHtJSmLSOVybYhy22vRih4eduhhrkcjgrWGnRfrZtovJ2FRjxuTtkkj47O/baf0R86QU5OuYpz8fA==", + "dev": true, "license": "ISC", "optional": true, "engines": { @@ -6730,6 +6742,7 @@ "version": "2.2.1", "resolved": "https://registry.npmjs.org/env-paths/-/env-paths-2.2.1.tgz", "integrity": "sha512-+h1lkLKhZMTYjog1VEpJNG7NZJWcuc2DDk/qsqSTRRCOXiLjeQ1d1/udrUGhqMxUgAlwKNZ0cf2uqan5GLuS2A==", + "dev": true, "license": "MIT", "optional": true, "engines": { @@ -7466,6 +7479,7 @@ "version": "3.1.3", "resolved": "https://registry.npmjs.org/exponential-backoff/-/exponential-backoff-3.1.3.tgz", "integrity": "sha512-ZgEeZXj30q+I0EN+CbSSpIyPaJ5HVQD18Z1m+u1FXbAeT94mr1zw50q4q6jiiC447Nl/YTcIYSAftiGqetwXCA==", + "dev": true, "license": "Apache-2.0", "optional": true }, @@ -7653,7 +7667,7 @@ "version": "6.5.0", "resolved": "https://registry.npmjs.org/fdir/-/fdir-6.5.0.tgz", "integrity": "sha512-tIbYtZbucOs0BRGqPJkshJUYdL+SDH7dVM8gjy+ERp3WAUjLEFJE+02kanyHtwjWOnwrKYBiwAmM0p4kLJAnXg==", - "devOptional": true, + "dev": true, "license": "MIT", "engines": { "node": ">=12.0.0" @@ -8186,7 +8200,7 @@ "version": "4.2.11", "resolved": "https://registry.npmjs.org/graceful-fs/-/graceful-fs-4.2.11.tgz", "integrity": "sha512-RbJ5/jmFcNNCcDV5o9eTnBLJ/HszWV0P73bc+Ff4nS/rJj+YaS6IGyiOL0VoBYX+l1Wrl3k63h/KrH+nhJ0XvQ==", - "devOptional": true, + "dev": true, "license": "ISC" }, "node_modules/h3": { @@ -10689,6 +10703,7 @@ "version": "12.4.0", "resolved": "https://registry.npmjs.org/node-gyp/-/node-gyp-12.4.0.tgz", "integrity": "sha512-OMcPNvqTCFUnNaBlmdgq+lfNqY7gTiSmNRDjY3uAXRyudeKZEZxu3CLtjMQrx4zZxCX2b/mpNqTtwuCJgXhHkw==", + "dev": true, "license": "MIT", "optional": true, "dependencies": { @@ -10725,6 +10740,7 @@ "version": "4.0.0", "resolved": "https://registry.npmjs.org/isexe/-/isexe-4.0.0.tgz", "integrity": "sha512-FFUtZMpoZ8RqHS3XeXEmHWLA4thH+ZxCv2lOiPIn1Xc7CxrqhWzNSDzD+/chS/zbYezmiwWLdQC09JdQKmthOw==", + "dev": true, "license": "BlueOak-1.0.0", "optional": true, "engines": { @@ -10735,6 +10751,7 @@ "version": "6.0.1", "resolved": "https://registry.npmjs.org/which/-/which-6.0.1.tgz", "integrity": "sha512-oGLe46MIrCRqX7ytPUf66EAYvdeMIZYn3WaocqqKZAxrBpkqHfL/qvTyJ/bTk5+AqHCjXmrv3CEWgy368zhRUg==", + "dev": true, "license": "ISC", "optional": true, "dependencies": { @@ -10774,6 +10791,7 @@ "version": "9.0.0", "resolved": "https://registry.npmjs.org/nopt/-/nopt-9.0.0.tgz", "integrity": "sha512-Zhq3a+yFKrYwSBluL4H9XP3m3y5uvQkB/09CwDruCiRmR/UJYnn9W4R48ry0uGC70aeTPKLynBtscP9efFFcPw==", + "dev": true, "license": "ISC", "optional": true, "dependencies": { @@ -11313,7 +11331,7 @@ "version": "4.0.7", "resolved": "https://registry.npmjs.org/picomatch/-/picomatch-4.0.7.tgz", "integrity": "sha512-qcJu88Q2IWqJsDD529JKMdwGm/dvInW4HvQnRwiH9JtihJvzGOscDtHE3x1pBKeUOTysQ8kVmLnJ2kJu7yhcGA==", - "devOptional": true, + "dev": true, "license": "MIT", "engines": { "node": ">=12" @@ -11677,6 +11695,7 @@ "version": "6.1.0", "resolved": "https://registry.npmjs.org/proc-log/-/proc-log-6.1.0.tgz", "integrity": "sha512-iG+GYldRf2BQ0UDUAd6JQ/RwzaQy6mXmsk/IzlYyal4A4SNFw54MeH4/tLkF4I5WoWG9SQwuqWzS99jaFQHBuQ==", + "dev": true, "license": "ISC", "optional": true, "engines": { @@ -13498,7 +13517,7 @@ "version": "0.2.17", "resolved": "https://registry.npmjs.org/tinyglobby/-/tinyglobby-0.2.17.tgz", "integrity": "sha512-wXR/dYpcqKmfWpEdZjiKJOwCNFndD0DMnrW/cYjVGttEkBfVgcLFHoNrlj47mjOVic9yyNu65alsgF4NQyTa2g==", - "devOptional": true, + "dev": true, "license": "MIT", "dependencies": { "fdir": "^6.5.0", @@ -14114,6 +14133,7 @@ "version": "6.28.0", "resolved": "https://registry.npmjs.org/undici/-/undici-6.28.0.tgz", "integrity": "sha512-LIY910g9TI13YS95lrMFrs8Rm/u/irgHeTWoKCoteeJ04CUJ92eEfj0rVn+7VKMPBpUPiUoBKfhNyLI23EE/KA==", + "dev": true, "license": "MIT", "optional": true, "engines": { diff --git a/package.json b/package.json index 5cd239b..652a008 100644 --- a/package.json +++ b/package.json @@ -12,6 +12,7 @@ "build": "tsc", "start": "node dist/index.js", "test": "jest", + "test:changed": "jest --changedSince=main", "test:coverage": "jest --coverage", "test:integration": "jest --runInBand tests/integration/**/*.test.ts", "test:watch": "jest --watch", diff --git a/src/__tests__/SongRoutes.test.ts b/src/__tests__/SongRoutes.test.ts index 841d368..2c483f2 100644 --- a/src/__tests__/SongRoutes.test.ts +++ b/src/__tests__/SongRoutes.test.ts @@ -72,9 +72,6 @@ jest.mock('fluent-ffmpeg', () => { jest.mock('../services/CacheService', () => ({ CacheService: { clearSong: jest.fn().mockResolvedValue(undefined) }, })); -jest.mock('../services/SearchIndexService', () => ({ - SearchIndexService: { scheduleRemoval: jest.fn(), scheduleIndexUpdate: jest.fn() }, -})); jest.mock('../workers/precomputeManifest', () => ({ precomputeSignedManifest: jest.fn() })); jest.mock('../controllers/UploadController', () => ({ UploadController: jest.fn(() => ({ @@ -360,6 +357,48 @@ describe('version history endpoints', () => { const res = await request(app).get('/api/song/s1/versions').set('Authorization', admin); expect(res.status).toBe(200); }); + + it('rejects a non-owner attempting to rollback a version with 403', async () => { + songRepo.findOneBy.mockResolvedValue(makeSong()); + const res = await request(app) + .post('/api/song/s1/versions/1/rollback') + .set('Authorization', other) + .send({ changeNote: 'unauthorized rollback' }); + expect(res.status).toBe(403); + expect(versionRepo.save).not.toHaveBeenCalled(); + }); + + it('lets the owning artist rollback to a prior version, creating a new active version', async () => { + const song = makeSong({ title: 'Current Title v2' }); + songRepo.findOneBy.mockResolvedValue(song); + versionRepo.findOne + // getVersion fetches target version + .mockResolvedValueOnce({ + versionNumber: 1, + title: 'Original Title v1', + description: 'Original Desc', + genre: 'afrobeats', + status: 'ready', + }) + // getLatestVersionNumber + .mockResolvedValueOnce({ versionNumber: 2 }); + + const res = await request(app) + .post('/api/song/s1/versions/1/rollback') + .set('Authorization', owner) + .send({ changeNote: 'Reverting to v1' }); + + expect(res.status).toBe(201); + expect(res.body.success).toBe(true); + expect(res.body.data.versionNumber).toBe(3); + expect(res.body.data.isActive).toBe(true); + expect(res.body.data.title).toBe('Original Title v1'); + expect(versionRepo.update).toHaveBeenCalledWith( + { songId: 's1', isActive: true }, + { isActive: false }, + ); + expect(songRepo.save).toHaveBeenCalled(); + }); }); describe('POST /api/song/:id/report', () => { diff --git a/src/__tests__/SongRoutesOrder.test.ts b/src/__tests__/SongRoutesOrder.test.ts index c604513..3d7654d 100644 --- a/src/__tests__/SongRoutesOrder.test.ts +++ b/src/__tests__/SongRoutesOrder.test.ts @@ -2,18 +2,19 @@ import 'reflect-metadata'; import express from 'express'; import request from 'supertest'; -jest.mock('../controllers/SongController', () => ({ - SongController: { +jest.mock('../controllers/SongController', () => { + const handlers: Record = { getSong: (_req: any, res: any) => res.status(200).json({ handler: 'getSong' }), listSongs: (_req: any, res: any) => res.status(200).json({ handler: 'listSongs' }), getPopularSongs: (_req: any, res: any) => res.status(200).json({ handler: 'popular' }), streamSong: (_req: any, res: any) => res.status(200).json({ handler: 'stream' }), - prepareMint: jest.fn(), - submitMint: jest.fn(), - disputeSplit: jest.fn(), - resolveDispute: jest.fn(), - }, -})); + }; + return { + SongController: new Proxy(handlers, { + get: (target, prop) => (prop in target ? target[prop as string] : jest.fn()), + }), + }; +}); jest.mock('../controllers/EmbedController', () => ({ EmbedController: { getSongEmbed: (_req: any, res: any) => res.status(200).json({ handler: 'embed' }), @@ -29,6 +30,7 @@ jest.mock('../controllers/UploadController', () => ({ })); jest.mock('../middlewares/authMiddleware', () => ({ authArtistMiddleware: (_req: any, _res: any, next: any) => next(), + requireAuth: (_req: any, _res: any, next: any) => next(), optionalAuth: (_req: any, _res: any, next: any) => next(), })); jest.mock('../middlewares/idempotency', () => ({ diff --git a/src/config/__tests__/logger.test.ts b/src/config/__tests__/logger.test.ts new file mode 100644 index 0000000..530c342 --- /dev/null +++ b/src/config/__tests__/logger.test.ts @@ -0,0 +1,86 @@ +import pino from 'pino'; +import { SENSITIVE_LOG_PATHS, createHttpLogShipper } from '../logger'; +import http from 'http'; + +describe('Logger Configuration (Issue #380)', () => { + it('redacts passwords, tokens, secrets, and auth headers from log records', () => { + let capturedLog: any = null; + + const testStream = { + write: (chunk: string) => { + try { + capturedLog = JSON.parse(chunk); + } catch { + // ignore + } + }, + }; + + const testLogger = pino( + { + level: 'info', + redact: { + paths: SENSITIVE_LOG_PATHS, + censor: '[REDACTED]', + }, + }, + testStream, + ); + + testLogger.info( + { + password: 'my-super-secret-password', + token: 'eyJh...token', + apiKey: 'sk-live-12345', + secret: 'very-secret', + authorization: 'Bearer secret-jwt', + user: { + password: 'nested-password', + refreshToken: 'refresh-123', + }, + safeField: 'visible-value', + }, + 'User login test', + ); + + expect(capturedLog).toBeDefined(); + expect(capturedLog.password).toBe('[REDACTED]'); + expect(capturedLog.token).toBe('[REDACTED]'); + expect(capturedLog.apiKey).toBe('[REDACTED]'); + expect(capturedLog.secret).toBe('[REDACTED]'); + expect(capturedLog.authorization).toBe('[REDACTED]'); + expect(capturedLog.user.password).toBe('[REDACTED]'); + expect(capturedLog.user.refreshToken).toBe('[REDACTED]'); + expect(capturedLog.safeField).toBe('visible-value'); + }); + + it('createHttpLogShipper buffers and ships logs to HTTP endpoint without throwing', (done) => { + const server = http.createServer((req, res) => { + let body = ''; + req.on('data', (chunk) => { + body += chunk; + }); + req.on('end', () => { + const parsed = JSON.parse(body); + expect(parsed.streams).toBeDefined(); + expect(parsed.streams[0].values.length).toBeGreaterThan(0); + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.end('{}'); + server.close(done); + }); + }); + + server.listen(0, '127.0.0.1', () => { + const port = (server.address() as any).port; + const shipper = createHttpLogShipper(`http://127.0.0.1:${port}/loki/api/v1/push`); + + // Write a log line to shipper + shipper.write('{"level":30,"msg":"test log shipping line"}\n'); + + // Fill buffer to trigger immediate flush + for (let i = 0; i < 55; i++) { + shipper.write(`{"level":30,"msg":"line ${i}"}\n`); + } + }); + }); +}); diff --git a/src/config/logger.ts b/src/config/logger.ts index e00f98b..3219d71 100644 --- a/src/config/logger.ts +++ b/src/config/logger.ts @@ -1,39 +1,169 @@ /** - * Structured logger configuration (Issue #33) + * Structured logger configuration (Issue #33, #380) * * Uses pino for structured, levelled logging with request correlation. * Log level is controlled via the LOG_LEVEL env var (default: "info"). * + * Redaction (#380): + * All sensitive credentials (passwords, tokens, API keys, secrets, authorization + * headers) are strictly redacted before output to prevent leaking credentials + * to stdout or any log aggregator. + * + * Shipping to Log Aggregator (#380): + * - Docker/Compose/K8s: Promtail/Fluentbit sidecar scrapes container logs and + * ships them to Grafana Loki, queryable via Grafana Explore. + * - Direct SDK/HTTP: If LOG_AGGREGATOR_URL is configured, logger streams + * batched log entries directly to the aggregator endpoint. + * * Usage: * import logger from './config/logger'; * logger.info({ reqId, route: req.path, status: 200 }, 'Request complete'); * logger.error({ reqId, err }, 'Something failed'); - * - * Log levels (set LOG_LEVEL=debug|info|warn|error): - * debug – verbose dev-time output - * info – normal operational events (default) - * warn – non-fatal anomalies - * error – failures that need attention */ import pino from 'pino'; +import http from 'http'; +import https from 'https'; +import { URL } from 'url'; -const logger = pino({ - level: process.env.LOG_LEVEL || 'info', - // Pretty-print in development, JSON in production/CI - ...(process.env.NODE_ENV !== 'production' && { - transport: { - target: 'pino-pretty', - options: { - colorize: true, - translateTime: 'SYS:standard', - ignore: 'pid,hostname', - }, +/** Sensitive field paths to redact before output/shipping (Issue #380). */ +export const SENSITIVE_LOG_PATHS = [ + 'password', + '*.password', + '*.*.password', + 'newPassword', + '*.newPassword', + 'oldPassword', + '*.oldPassword', + 'confirmPassword', + '*.confirmPassword', + 'token', + '*.token', + '*.*.token', + 'accessToken', + '*.accessToken', + 'refreshToken', + '*.refreshToken', + 'authorization', + '*.authorization', + 'headers.authorization', + '*.headers.authorization', + 'req.headers.authorization', + 'apiKey', + '*.apiKey', + 'secret', + '*.secret', + 'jwtSecret', + '*.jwtSecret', + 'clientSecret', + '*.clientSecret', + 'privateKey', + '*.privateKey', + 'twoFactorSecret', + 'totpSecret', + 'creditCard', + 'cardNumber', + 'cvv', + 'ssn', +]; + +/** Creates an HTTP log shipper destination for direct aggregator shipping (Issue #380). */ +export function createHttpLogShipper(aggregatorUrl: string): pino.DestinationStream { + const parsed = new URL(aggregatorUrl); + const client = parsed.protocol === 'https:' ? https : http; + + let buffer: string[] = []; + let flushTimeout: NodeJS.Timeout | null = null; + + const flush = () => { + if (buffer.length === 0) return; + const batch = buffer; + buffer = []; + flushTimeout = null; + + const payload = JSON.stringify({ + streams: [ + { + stream: { + service: 'audioblock-backend', + env: process.env.NODE_ENV || 'development', + }, + values: batch.map((line) => [String(Date.now() * 1000000), line]), + }, + ], + }); + + try { + const req = client.request(parsed, { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + 'Content-Length': Buffer.byteLength(payload), + }, + timeout: 5000, + }); + req.on('error', () => { + // Fail-open: aggregator network issue must never crash application + }); + req.write(payload); + req.end(); + } catch { + // Fail-open + } + }; + + return { + write(chunk: string) { + buffer.push(chunk.trim()); + if (buffer.length >= 50) { + if (flushTimeout) clearTimeout(flushTimeout); + flush(); + } else if (!flushTimeout) { + flushTimeout = setTimeout(flush, 2000); + } }, - }), + }; +} + +const baseOptions: pino.LoggerOptions = { + level: process.env.LOG_LEVEL || 'info', + redact: { + paths: SENSITIVE_LOG_PATHS, + censor: '[REDACTED]', + }, base: { service: 'audioblock-backend', env: process.env.NODE_ENV || 'development', }, -}); +}; + +function buildLogger(): pino.Logger { + const aggregatorUrl = process.env.LOG_AGGREGATOR_URL; + + if (aggregatorUrl) { + const streams: pino.StreamEntry[] = [ + { stream: process.stdout }, + { stream: createHttpLogShipper(aggregatorUrl) }, + ]; + return pino(baseOptions, pino.multistream(streams)); + } + + if (process.env.NODE_ENV !== 'production' && !process.env.CI) { + return pino({ + ...baseOptions, + transport: { + target: 'pino-pretty', + options: { + colorize: true, + translateTime: 'SYS:standard', + ignore: 'pid,hostname', + }, + }, + }); + } + + return pino(baseOptions); +} + +const logger = buildLogger(); export default logger; diff --git a/src/controllers/SongController.ts b/src/controllers/SongController.ts index 24bf93c..fe50994 100644 --- a/src/controllers/SongController.ts +++ b/src/controllers/SongController.ts @@ -448,6 +448,25 @@ export class SongController { } }; + /** POST /api/songs/:id/versions/:version/rollback — rollback to a prior version (Issue #441). */ + static rollbackVersion = async (req: Request, res: Response) => { + try { + const songId = req.params.id as string; + const versionNumber = Number(req.params.version); + const artistId = req.user?.id as string; + const { changeNote } = req.body || {}; + const newVersion = await songVersionService.rollbackVersion( + songId, + versionNumber, + artistId, + changeNote, + ); + return res.status(201).json({ success: true, data: newVersion }); + } catch (error) { + handleError(req, res, error); + } + }; + /** POST /api/songs/:id/report — submit a content report (Issue #88). */ static reportSong = async (req: Request, res: Response) => { try { diff --git a/src/routes/SongRoutes.ts b/src/routes/SongRoutes.ts index 4228bf2..271b15b 100644 --- a/src/routes/SongRoutes.ts +++ b/src/routes/SongRoutes.ts @@ -3,7 +3,8 @@ import { UploadController } from '../controllers/UploadController'; import { validateDTO } from '../middlewares/validate'; import { FinalizeUploadDTO } from '../dtos/FinalizeUploadDTO'; import { UploadChunkDTO } from '../dtos/UploadChunkDTO'; -import { authArtistMiddleware, requireAuth } from '../middlewares/authMiddleware'; +import { authArtistMiddleware, requireAuth, optionalAuth } from '../middlewares/authMiddleware'; +import { etagCache } from '../middlewares/etag'; import multer from 'multer'; import { CreateCoverDTO } from '../dtos/CreateCoverDTO'; import fs from 'fs'; @@ -14,6 +15,9 @@ import { idempotency } from '../middlewares/idempotency'; import { AddCollaboratorDTO } from '../dtos/AddCollaboratorDTO'; import { UpdateCollaboratorDTO } from '../dtos/UpdateCollaboratorDTO'; import { ApplyRoyaltyTemplateDTO } from '../dtos/ApplyRoyaltyTemplateDTO'; +import { reportRateLimiter } from '../middlewares/rateLimiter'; +import { ReportSongDTO } from '../dtos/ReportSongDTO'; +import { UpdateSongDTO } from '../dtos/UpdateSongDTO'; const uploadController = new UploadController(); const router = Router(); @@ -154,7 +158,12 @@ router.get('/:id', optionalAuth, etagCache({ visibility: 'no-cache' }), SongCont // Soroban on-chain song minting: the artist's wallet signs, the backend // only builds and relays the transaction. -router.post('/:id/onchain/prepare-mint', authArtistMiddleware, idempotency(), SongController.prepareMint); +router.post( + '/:id/onchain/prepare-mint', + authArtistMiddleware, + idempotency(), + SongController.prepareMint, +); router.post( '/:id/onchain/submit-mint', authArtistMiddleware, @@ -163,6 +172,42 @@ router.post( SongController.submitMint, ); +// Edit or remove a song (owner or admin; enforced in SongService) +router.patch('/:id', authArtistMiddleware, validateDTO(UpdateSongDTO), SongController.updateSong); +router.delete('/:id', authArtistMiddleware, SongController.deleteSong); + +// Lyrics are public, but only served once the lyrics rights are confirmed +router.get('/:id/lyrics', SongController.getLyrics); + +// Cover art: public read, owner-only replacement +router.get('/:id/cover', SongController.getCoverArt); +router.put( + '/:id/cover', + authArtistMiddleware, + coverReplaceUpload.single('cover'), + SongController.uploadCoverArt, +); + +// Version history (owner or admin; enforced in SongController) (Issue #86) +router.get('/:id/versions', requireAuth, SongController.listVersions); +router.get('/:id/versions/:version', requireAuth, SongController.getVersion); + +// Version rollback (owner-only; creates a new version copying historical metadata, Issue #441) +router.post( + '/:id/versions/:version/rollback', + authArtistMiddleware, + SongController.rollbackVersion, +); + +// Content reports: one per user per song (a repeat returns 409), 10 per hour per user +router.post( + '/:id/report', + requireAuth, + reportRateLimiter, + validateDTO(ReportSongDTO), + SongController.reportSong, +); + // Collaborator management (owner-only writes; credits are public to read) router.get('/:id/collaborators', SongController.listCollaborators); router.post( diff --git a/src/services/Song/SongVersionService.ts b/src/services/Song/SongVersionService.ts index 9bd38d2..5991396 100644 --- a/src/services/Song/SongVersionService.ts +++ b/src/services/Song/SongVersionService.ts @@ -159,6 +159,93 @@ export class SongVersionService { return version; } + /** + * Roll back a song's metadata and served revision to a prior version (Issue #441). + * + * Rather than mutating history, this creates a new version whose metadata + * and assets copy the specified historical version, and promotes it to active. + * + * @param songId - ID of the song. + * @param targetVersionNumber - The 1-based historical revision to restore. + * @param artistId - ID of the requesting artist. + * @param changeNote - Optional note explaining the rollback. + * @returns The newly created active version copying the historical version. + * @throws {AppError} 404 when song or version does not exist, 403 when caller is not the owning artist. + */ + async rollbackVersion( + songId: string, + targetVersionNumber: number, + artistId: string, + changeNote?: string, + ): Promise { + const song = await this.songRepo.findOneBy({ id: songId }); + if (!song) { + throw AppError.notFound('Song not found', undefined, 'SONG_NOT_FOUND'); + } + if (song.artistId !== artistId) { + throw AppError.authorization( + 'Not authorized to roll back this song', + undefined, + 'NOT_SONG_OWNER', + ); + } + + const targetVersion = await this.getVersion(songId, targetVersionNumber); + + const nextNumber = (await this.getLatestVersionNumber(songId)) + 1; + + // Demote existing revisions so only the new rollback version is active + await this.versionRepo.update({ songId, isActive: true }, { isActive: false }); + + // Create a new version copying the target version's fields (history is preserved, never mutated) + const newVersion = this.versionRepo.create({ + songId, + versionNumber: nextNumber, + isActive: true, + title: targetVersion.title, + description: targetVersion.description, + genre: targetVersion.genre, + composers: targetVersion.composers, + coverArtPath: targetVersion.coverArtPath, + s3OriginalUrl: targetVersion.s3OriginalUrl, + hlsMasterUrl: targetVersion.hlsMasterUrl, + ipfsCid: targetVersion.ipfsCid, + metadataCid: targetVersion.metadataCid, + duration: targetVersion.duration, + loudness: targetVersion.loudness, + status: targetVersion.status, + changeNote: changeNote ?? `Rolled back to version ${targetVersionNumber}`, + createdBy: artistId, + }); + + await this.versionRepo.save(newVersion); + + // Update parent song to point at the rolled-back revision + song.title = newVersion.title ?? song.title; + if (newVersion.description !== undefined) song.description = newVersion.description as string; + if (newVersion.genre !== undefined) song.genre = newVersion.genre as string; + if (newVersion.composers !== undefined) song.composers = newVersion.composers; + if (newVersion.coverArtPath !== undefined) song.coverArtPath = newVersion.coverArtPath; + if (newVersion.s3OriginalUrl !== undefined) song.s3OriginalUrl = newVersion.s3OriginalUrl; + if (newVersion.hlsMasterUrl !== undefined) song.hlsMasterUrl = newVersion.hlsMasterUrl; + if (newVersion.ipfsCid !== undefined) song.ipfsCid = newVersion.ipfsCid; + if (newVersion.metadataCid !== undefined) song.metadataCid = newVersion.metadataCid; + if (newVersion.duration !== undefined) song.duration = newVersion.duration; + if (newVersion.loudness !== undefined) song.loudness = newVersion.loudness; + song.status = newVersion.status; + song.errorReason = null; + await this.songRepo.save(song); + + await CacheService.clearSong(songId); + + logger.info( + { songId, fromVersion: targetVersionNumber, toVersion: nextNumber }, + 'Rolled back song version', + ); + + return newVersion; + } + /** * Version history can include unreleased audio, so only the owning artist * and admins may read it. diff --git a/src/services/SongService.ts b/src/services/SongService.ts index b2109e6..fd600c6 100644 --- a/src/services/SongService.ts +++ b/src/services/SongService.ts @@ -21,6 +21,7 @@ import { ActivityService } from './ActivityService'; import logger from '../config/logger'; import { songsUploadedTotal } from './MetricsService'; import { deleteS3Objects, deleteS3Prefix, s3KeyFromUrl } from '../utils/s3Cleanup'; +import { paginatedResponse } from '../utils/pagination'; const activityService = new ActivityService(); @@ -146,10 +147,6 @@ export class SongService { // Invalidate the cached streaming manifest so updated metadata is // reflected immediately, not only after TTL expiry (#335). await CacheService.clearSong(id); - if (result.status === 'ready' && !result.flagged) { - const indexable = await this.songRepo.findOne({ where: { id }, relations: ['user'] }); - if (indexable) SearchIndexService.scheduleIndexUpdate(indexable); - } return result; } @@ -172,13 +169,11 @@ export class SongService { song.flaggedBy = requester.id; song.flagReason = 'Hidden by owner (song is minted on-chain)'; await this.songRepo.save(song); - SearchIndexService.scheduleRemoval(id); await CacheService.clearSong(id); return { success: true, hidden: true }; } await this.songRepo.remove(song); - SearchIndexService.scheduleRemoval(id); // Invalidate cache on delete so a re-created song with the same ID // doesn't serve stale data (#335). await CacheService.clearSong(id); diff --git a/src/services/__tests__/SongVersionService.test.ts b/src/services/__tests__/SongVersionService.test.ts index 38a68df..ff24ecc 100644 --- a/src/services/__tests__/SongVersionService.test.ts +++ b/src/services/__tests__/SongVersionService.test.ts @@ -233,3 +233,74 @@ describe('SongVersionService.syncActiveVersion', () => { expect(mockVersionRepo.save).not.toHaveBeenCalled(); }); }); + +describe('SongVersionService.rollbackVersion', () => { + it('creates a new version copying target version metadata and promotes it to active', async () => { + const song = makeSong({ title: 'Current Title v2' }); + mockSongRepo.findOneBy.mockResolvedValue(song); + mockVersionRepo.findOne + // getVersion fetches target version + .mockResolvedValueOnce({ + versionNumber: 1, + title: 'Original Title v1', + description: 'Original Desc', + genre: 'afrobeats', + composers: 'Original Composer', + coverArtPath: 'cover-v1.png', + s3OriginalUrl: 's3://v1.mp3', + hlsMasterUrl: 'https://cdn/v1/master.m3u8', + ipfsCid: 'cid-v1', + metadataCid: 'meta-v1', + duration: 180, + loudness: -14, + status: 'ready', + } as SongVersion) + // getLatestVersionNumber fetches latest version + .mockResolvedValueOnce({ versionNumber: 2 } as SongVersion); + + const rolledBack = await new SongVersionService().rollbackVersion( + 'song-1', + 1, + 'artist-1', + 'Restoring original mix', + ); + + expect(rolledBack.versionNumber).toBe(3); + expect(rolledBack.isActive).toBe(true); + expect(rolledBack.title).toBe('Original Title v1'); + expect(rolledBack.description).toBe('Original Desc'); + expect(rolledBack.changeNote).toBe('Restoring original mix'); + expect(rolledBack.status).toBe('ready'); + + // Demoted prior active versions + expect(mockVersionRepo.update).toHaveBeenCalledWith( + { songId: 'song-1', isActive: true }, + { isActive: false }, + ); + + // Parent song was updated with rolled-back fields + expect(mockSongRepo.save).toHaveBeenCalledWith( + expect.objectContaining({ + title: 'Original Title v1', + description: 'Original Desc', + status: 'ready', + }), + ); + }); + + it('rejects rollback by a non-owner with 403', async () => { + mockSongRepo.findOneBy.mockResolvedValue(makeSong({ artistId: 'owner-id' })); + + await expect( + new SongVersionService().rollbackVersion('song-1', 1, 'intruder-id'), + ).rejects.toMatchObject({ statusCode: 403 }); + }); + + it('rejects rollback for a non-existent song with 404', async () => { + mockSongRepo.findOneBy.mockResolvedValue(null); + + await expect( + new SongVersionService().rollbackVersion('unknown-song', 1, 'artist-1'), + ).rejects.toMatchObject({ statusCode: 404 }); + }); +});