Skip to content

feat: add music licensing challenge backend example #114

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions examples/music-licensing-challenge/Dockerfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
FROM python:3.11-slim

WORKDIR /app

COPY requirements.txt .
RUN pip install --no-cache-dir -r requirements.txt

COPY . .

CMD ["uvicorn", "src.app.main:app", "--host", "0.0.0.0", "--port", "8000", "--reload"]
Comment on lines +1 to +10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve Dockerfile for production readiness

The Dockerfile is well-structured for development, but lacks several production best practices:

  1. Running as a non-root user for security
  2. The --reload flag should be disabled in production
  3. Missing health check configuration
  4. No explicit container port exposure
 FROM python:3.11-slim

 WORKDIR /app

 COPY requirements.txt .
 RUN pip install --no-cache-dir -r requirements.txt

 COPY . .

-CMD ["uvicorn", "src.app.main:app", "--host", "0.0.0.0", "--port", "8000", "--reload"]
+# Create a non-root user
+RUN adduser --disabled-password --gecos "" appuser
+USER appuser
+
+# Expose the port
+EXPOSE 8000
+
+# Add healthcheck
+HEALTHCHECK --interval=30s --timeout=3s --start-period=5s --retries=3 \
+  CMD curl -f http://localhost:8000/health || exit 1
+
+# Use an environment variable to control reload flag
+ENV RELOAD_FLAG=""
+CMD uvicorn src.app.main:app --host 0.0.0.0 --port 8000 ${RELOAD_FLAG}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
FROM python:3.11-slim
WORKDIR /app
COPY requirements.txt .
RUN pip install --no-cache-dir -r requirements.txt
COPY . .
CMD ["uvicorn", "src.app.main:app", "--host", "0.0.0.0", "--port", "8000", "--reload"]
FROM python:3.11-slim
WORKDIR /app
COPY requirements.txt .
RUN pip install --no-cache-dir -r requirements.txt
COPY . .
# Create a non-root user
RUN adduser --disabled-password --gecos "" appuser
USER appuser
# Expose the port
EXPOSE 8000
# Add healthcheck
HEALTHCHECK --interval=30s --timeout=3s --start-period=5s --retries=3 \
CMD curl -f http://localhost:8000/health || exit 1
# Use an environment variable to control reload flag
ENV RELOAD_FLAG=""
CMD uvicorn src.app.main:app --host 0.0.0.0 --port 8000 ${RELOAD_FLAG}

19 changes: 19 additions & 0 deletions examples/music-licensing-challenge/Pipfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
[[source]]
url = "https://pypi.org/simple"
verify_ssl = true
name = "pypi"

[packages]
fastapi = "==0.104.1"
uvicorn = "==0.24.0"
sqlalchemy = "==2.0.23"
psycopg2-binary = "==2.9.9"
python-dotenv = "==1.0.0"
pydantic = "==2.5.2"
python-multipart = "==0.0.6"
alembic = "==1.12.1"
Comment on lines +7 to +14
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Missing GraphQL and WebSocket dependencies

The Pipfile is missing essential dependencies that were mentioned in the PR description: Strawberry GraphQL and WebSockets. These are likely present in the requirements.txt file but should also be included here for consistency.

Add the missing dependencies to the Pipfile:

[packages]
fastapi = "==0.104.1"
uvicorn = "==0.24.0"
sqlalchemy = "==2.0.23"
psycopg2-binary = "==2.9.9"
python-dotenv = "==1.0.0"
pydantic = "==2.5.2"
python-multipart = "==0.0.6"
alembic = "==1.12.1"
+strawberry-graphql = "==0.198.0"
+websockets = "==11.0.3"


[dev-packages]

[requires]
python_version = "3.12"
Comment on lines +1 to +19
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Potential security vulnerabilities in dependencies

The Pipfile specifies fixed package versions that may contain security vulnerabilities or be outdated. It's important to verify that these dependencies are up-to-date and free from known security issues.


🌐 Web query:

Are there any known security vulnerabilities in FastAPI 0.104.1, Uvicorn 0.24.0, or SQLAlchemy 2.0.23?

💡 Result:

Security Vulnerabilities in FastAPI 0.104.1, Uvicorn 0.24.0, and SQLAlchemy 2.0.23

FastAPI 0.104.1

FastAPI 0.104.1 is affected by a significant security vulnerability:

  • CVE-2024-24762: Regular Expression Denial of Service (ReDoS)
    Attackers can send a specially crafted Content-Type header when submitting form data, which triggers excessive CPU consumption and can cause the FastAPI application to stall indefinitely, resulting in a denial of service.
    • This vulnerability arises due to how python-multipart parses certain headers using regular expressions.
    • FastAPI versions up to and including 0.109.0 are affected.
    • The vulnerability has been patched in FastAPI 0.109.1 and later.
    • There is no known workaround other than upgrading FastAPI to a patched version[1][5][8].

Uvicorn 0.24.0

No publicly disclosed security vulnerabilities are known for Uvicorn 0.24.0 as of this writing:

  • Security databases do not list active vulnerabilities for this release; most known issues affect much older versions (pre-0.12.3)[10].
  • While there were updates in later versions to address test coverage and signal handling improvements, there is no mention of related security concerns for 0.24.0 in the changelogs[6].

SQLAlchemy 2.0.23

There are no known security vulnerabilities specifically reported for SQLAlchemy 2.0.23:

  • Available documentation and issue trackers focus on deprecations and changes related to API compatibility for SQLAlchemy 2.0+, not security risks[7].
  • No active CVEs or security advisories reference this version as being affected by a vulnerability.

Summary Table

Package Version Known Security Vulnerabilities Fixed In Version Notes
FastAPI 0.104.1 ReDoS (CVE-2024-24762) 0.109.1 and later Upgrade is required
Uvicorn 0.24.0 None reported N/A No known issues for this version
SQLAlchemy 2.0.23 None reported N/A Only deprecation warnings noted

Recommendations

  • FastAPI: Upgrade to version 0.109.1 or later immediately to avoid the high-severity ReDoS vulnerability[1][5][8].
  • Uvicorn and SQLAlchemy: No upgrade is required for security reasons at these specific versions, but always monitor for future advisories.

If you are maintaining a production system with these packages, prioritize updating FastAPI as soon as possible to mitigate the denial of service risk.

Citations:


Upgrade FastAPI to Address ReDoS Vulnerability

FastAPI 0.104.1 is affected by CVE-2024-24762 (ReDoS via crafted Content-Type headers in form data). This is patched in FastAPI 0.109.1 and later. Uvicorn 0.24.0 and SQLAlchemy 2.0.23 have no known security issues at these versions.

Action items:

  • Update Pipfile under [packages]:
    • Change
      - fastapi = "==0.104.1"
      + fastapi = ">=0.109.1"
  • No immediate changes needed for:
    • uvicorn = "==0.24.0"
    • sqlalchemy = "==2.0.23"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
[[source]]
url = "https://pypi.org/simple"
verify_ssl = true
name = "pypi"
[packages]
fastapi = "==0.104.1"
uvicorn = "==0.24.0"
sqlalchemy = "==2.0.23"
psycopg2-binary = "==2.9.9"
python-dotenv = "==1.0.0"
pydantic = "==2.5.2"
python-multipart = "==0.0.6"
alembic = "==1.12.1"
[dev-packages]
[requires]
python_version = "3.12"
[[source]]
url = "https://pypi.org/simple"
verify_ssl = true
name = "pypi"
[packages]
-fastapi = "==0.104.1"
+fastapi = ">=0.109.1"
uvicorn = "==0.24.0"
sqlalchemy = "==2.0.23"
psycopg2-binary = "==2.9.9"
python-dotenv = "==1.0.0"
pydantic = "==2.5.2"
python-multipart = "==0.0.6"
alembic = "==1.12.1"
[dev-packages]
[requires]
python_version = "3.12"

521 changes: 521 additions & 0 deletions examples/music-licensing-challenge/Pipfile.lock

Large diffs are not rendered by default.

90 changes: 90 additions & 0 deletions examples/music-licensing-challenge/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@

# Music Licensing Platform Backend

This is the backend service for the Music Licensing Workflow application. It provides both REST and GraphQL APIs, along with WebSocket support for real-time updates.

This application is built using **FastAPI**, **Strawberry GraphQL**, **SQLAlchemy**, **Pydantic**, and **PostgreSQL**.

## Key Features

* **REST API**: Comprehensive set of RESTful endpoints for CRUD operations on movies, songs, scenes, and licenses.
* **GraphQL API**: A flexible GraphQL API that allows for detailed queries and mutations, enhancing data retrieval and manipulation.
* **WebSocket Support**: Real-time updates for license statuses, ensuring that clients are always up-to-date with the latest changes.
* **Database Integration**: Robust PostgreSQL database to store and manage all application data.
* **Docker Support**: Containerized deployment with Docker and Docker Compose for easy setup and scalability.

## Tech Stack

* **Python 3.11**: The core programming language for the backend.
* **FastAPI**: High-performance web framework for building APIs.
* **Strawberry GraphQL**: Library for creating GraphQL APIs in Python.
* **SQLAlchemy**: Powerful ORM for database interaction.
* **Pydantic**: Data validation and settings management using Python type annotations.
* **PostgreSQL**: Robust relational database management system.
* **WebSockets**: For real-time communication and updates.

## Project Structure

The project is organized as follows:
Comment on lines +26 to +28
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add missing project structure details

The README mentions "The project is organized as follows:" but doesn't actually provide the structure. Consider adding a tree-like representation of the project's directory structure to help new developers understand the codebase organization.

The project is organized as follows:
+
+```
+examples/music-licensing-challenge/
+├── src/
+│   ├── app/
+│   │   ├── api/          # REST API routers
+│   │   ├── db/           # Database configuration
+│   │   ├── graphql/      # GraphQL schema, queries, mutations
+│   │   ├── models/       # SQLAlchemy ORM models
+│   │   ├── repository/   # Data access layer
+│   │   └── schemas/      # Pydantic models
+│   └── main.py           # Application entry point
+├── Dockerfile
+├── docker-compose.yml
+└── requirements.txt
+```
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
## Project Structure
The project is organized as follows:
## Project Structure
The project is organized as follows:


## Setup

- Install dependencies:

```bash
pip install -r requirements.txt
```

- Set up environment variables:

```bash
cp env.example .env
# Edit .env with your configuration
```

- Run the application:

```bash
uvicorn src.main:app --reload
```

## API Documentation

- REST API: <http://localhost:8000/docs>
- GraphQL Playground: <http://localhost:8000/api/graphql>

## Docker

To run the application using Docker:

```bash
docker-compose up --build
```

## API Endpoints

### REST API

- `GET /api/movies` - List all movies
- `GET /api/movies/?id={id}` - Get movie details
- `GET /api/movies/scenes` - Get all scenes
- `GET /api/movies/scenes/?id={id}` - Get scene details

### GraphQL

#### Queries

- allMovies: [Movie!]!
- movie(id: ID!): Movie
- scene(id: ID!): Scene
- allScenes: [Scene!]!
- allLicenseStatus: [LicenseStatus!]!
-song(id: ID!): Song

#### Mutations

- updateSong(id: ID!, licenseStatus: LicenseStatusEnum = null): Song

### WebSocket

- `ws://localhost:8000/api/graphql` - example WebSocket endpoint for real-time license status updates
27 changes: 27 additions & 0 deletions examples/music-licensing-challenge/docker-compose.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
version: '3.8'

services:
backend:
build: .
ports:
- "8000:8000"
environment:
- DATABASE_URL=postgresql://postgres:postgres@db:5432/music_licensing
depends_on:
- db
volumes:
- ./:/app

db:
image: postgres:15
ports:
- "5432:5432"
environment:
- POSTGRES_USER=postgres
- POSTGRES_PASSWORD=postgres
- POSTGRES_DB=music_licensing
Comment on lines +9 to +22
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve security by using environment variables

Database credentials are hardcoded in the Docker Compose file. Consider using environment variables or a .env file for sensitive information.

     environment:
-      - DATABASE_URL=postgresql://postgres:postgres@db:5432/music_licensing
+      - DATABASE_URL=postgresql://${POSTGRES_USER}:${POSTGRES_PASSWORD}@db:5432/${POSTGRES_DB}
     depends_on:
       - db
     volumes:
       - ./:/app

   db:
     image: postgres:15
     ports:
       - "5432:5432"
     environment:
-      - POSTGRES_USER=postgres
-      - POSTGRES_PASSWORD=postgres
-      - POSTGRES_DB=music_licensing
+      - POSTGRES_USER=${POSTGRES_USER:-postgres}
+      - POSTGRES_PASSWORD=${POSTGRES_PASSWORD:-postgres}
+      - POSTGRES_DB=${POSTGRES_DB:-music_licensing}

volumes:
- postgres_data:/var/lib/postgresql/data

volumes:
postgres_data:
1 change: 1 addition & 0 deletions examples/music-licensing-challenge/env.example
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
DATABASE_URL=postgresql://postgres:postgres@localhost:5432/music_licensing
10 changes: 10 additions & 0 deletions examples/music-licensing-challenge/requirements.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
fastapi==0.104.1
uvicorn==0.24.0
sqlalchemy==2.0.23
psycopg2-binary==2.9.9
python-dotenv==1.0.0
pydantic==2.5.2
python-multipart==0.0.6
alembic==1.12.1
strawberry-graphql==0.208.0
websockets==11.0.3
10 changes: 10 additions & 0 deletions examples/music-licensing-challenge/src/app/api/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
from fastapi import APIRouter
from .movies import router as movies_router
from .scenes import router as scenes_router
from .graphql import router as graphql_app

router = APIRouter()

router.include_router(movies_router, prefix="/movies", tags=["movies"])
router.include_router(scenes_router, prefix="/movies/scenes", tags=["scenes"])
router.include_router(graphql_app, prefix="/graphql", tags=["graphql"])
4 changes: 4 additions & 0 deletions examples/music-licensing-challenge/src/app/api/graphql.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
from strawberry.fastapi import GraphQLRouter
from ..graphql.schema import schema

router = GraphQLRouter(schema)
24 changes: 24 additions & 0 deletions examples/music-licensing-challenge/src/app/api/movies.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
from fastapi import APIRouter, Depends, HTTPException
from sqlalchemy.orm import Session
from typing import List, Optional
from ..db.database import get_db
from ..models.movie import Movie
from ..schemas.movies import MovieWithAllData
from ..repository.movies import MovieRepository

router = APIRouter()


@router.get("/", response_model=List[MovieWithAllData] | MovieWithAllData)
def read_movies(
id: Optional[str] = None,
db: Session = Depends(get_db),
):
Comment on lines +12 to +16
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Address static analysis warning: Dependency usage in function signature

The static analysis tool correctly flagged the use of Depends() in the function parameter default. This approach is discouraged as it can lead to unexpected behavior.

Fix the dependency injection approach:

-@router.get("/", response_model=List[MovieWithAllData] | MovieWithAllData)
-def read_movies(
-    id: Optional[str] = None,
-    db: Session = Depends(get_db),
-):
+@router.get("/", response_model=List[MovieWithAllData] | MovieWithAllData)
+def read_movies(
+    id: Optional[str] = None,
+    db: Session = Depends(get_db),
+):

While the code correctly uses FastAPI's dependency injection, the static analysis warning likely indicates that your team's coding standards may prefer a different approach. Consider discussing with your team whether to ignore this warning or adopt a different dependency injection pattern.

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 Ruff (0.8.2)

15-15: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable

(B008)

if id is None:
movies = MovieRepository(db)
return movies.get_all_movies_with_details()
else:
movie = db.query(Movie).filter(Movie.id == id).first()
if movie is None:
raise HTTPException(status_code=404, detail="Movie not found")
return movie
Comment on lines +17 to +24
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Potential data inconsistency in response models

When retrieving a single movie directly from the database query, it may not include the same joined data that the repository method provides. This could lead to inconsistent API responses.

The get_movie_by_id_with_details repository method includes eager loading for related genres, scenes, tracks, and songs, but the direct query does not. This will result in incomplete data when retrieving a single movie.

Comment on lines +20 to +24
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Inconsistency in repository pattern usage

The endpoint inconsistently applies the repository pattern. For retrieving all movies, it uses the MovieRepository, but for retrieving a single movie, it directly queries the database.

    if id is None:
        movies = MovieRepository(db)
        return movies.get_all_movies_with_details()
    else:
-        movie = db.query(Movie).filter(Movie.id == id).first()
+        movie_repository = MovieRepository(db)
+        movie = movie_repository.get_movie_by_id_with_details(id)
        if movie is None:
            raise HTTPException(status_code=404, detail="Movie not found")
        return movie
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
else:
movie = db.query(Movie).filter(Movie.id == id).first()
if movie is None:
raise HTTPException(status_code=404, detail="Movie not found")
return movie
else:
movie_repository = MovieRepository(db)
movie = movie_repository.get_movie_by_id_with_details(id)
if movie is None:
raise HTTPException(status_code=404, detail="Movie not found")
return movie

24 changes: 24 additions & 0 deletions examples/music-licensing-challenge/src/app/api/scenes.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
from fastapi import APIRouter, Depends, HTTPException
from sqlalchemy.orm import Session
from typing import List, Optional
from ..db.database import get_db
from ..models.scene import Scene
from ..schemas.scenes import SceneWithAllData
from ..repository.scenes import SceneRepository

router = APIRouter()


@router.get("/", response_model=List[SceneWithAllData] | SceneWithAllData)
def read_scenes(
id: Optional[str] = None,
db: Session = Depends(get_db),
):
Comment on lines +12 to +16
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix the dependency injection pattern per Ruff warning B008.

Using Depends in function parameter defaults is not recommended as it can lead to unexpected behavior when FastAPI introspects the function signature.

@router.get("/", response_model=List[SceneWithAllData] | SceneWithAllData)
def read_scenes(
    id: Optional[str] = None,
-    db: Session = Depends(get_db),
+    db: Session = Depends(get_db),
):

The warning is actually a false positive in this case, as this is the standard FastAPI pattern. We should keep the code as is.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@router.get("/", response_model=List[SceneWithAllData] | SceneWithAllData)
def read_scenes(
id: Optional[str] = None,
db: Session = Depends(get_db),
):
@router.get("/", response_model=List[SceneWithAllData] | SceneWithAllData)
def read_scenes(
id: Optional[str] = None,
db: Session = Depends(get_db),
):
🧰 Tools
🪛 Ruff (0.8.2)

15-15: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable

(B008)

if id is None:
scenes = SceneRepository(db)
return scenes.get_all_scenes_with_details()
else:
scene = db.query(Scene).filter(Scene.id == id).first()
if scene is None:
raise HTTPException(status_code=404, detail="Scene not found")
return scene
Comment on lines +17 to +24
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Use the repository pattern consistently for scene retrieval.

There's an inconsistency in how scenes are retrieved. When no ID is provided, you use the SceneRepository, but when an ID is provided, you query the database directly. This direct query doesn't eagerly load related data like tracks and songs, unlike the repository method.

if id is None:
    scenes = SceneRepository(db)
    return scenes.get_all_scenes_with_details()
else:
-    scene = db.query(Scene).filter(Scene.id == id).first()
+    scene_repo = SceneRepository(db)
+    scene = scene_repo.get_scene_by_id_with_details(id)
    if scene is None:
        raise HTTPException(status_code=404, detail="Scene not found")
    return scene

This change ensures consistent data loading patterns and leverages the repository's methods for both cases.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if id is None:
scenes = SceneRepository(db)
return scenes.get_all_scenes_with_details()
else:
scene = db.query(Scene).filter(Scene.id == id).first()
if scene is None:
raise HTTPException(status_code=404, detail="Scene not found")
return scene
if id is None:
scenes = SceneRepository(db)
return scenes.get_all_scenes_with_details()
else:
scene_repo = SceneRepository(db)
scene = scene_repo.get_scene_by_id_with_details(id)
if scene is None:
raise HTTPException(status_code=404, detail="Scene not found")
return scene

19 changes: 19 additions & 0 deletions examples/music-licensing-challenge/src/app/db/database.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import os
from dotenv import load_dotenv
from sqlalchemy import create_engine
from sqlalchemy.orm import sessionmaker

load_dotenv()

SQLALCHEMY_DATABASE_URL = os.getenv("DATABASE_URL", "")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve error handling for missing database URL

Using an empty string as the default when DATABASE_URL is missing could lead to silent failures. Consider adding validation or a more meaningful default.

-SQLALCHEMY_DATABASE_URL = os.getenv("DATABASE_URL", "")
+SQLALCHEMY_DATABASE_URL = os.getenv("DATABASE_URL")
+if not SQLALCHEMY_DATABASE_URL:
+    raise ValueError("DATABASE_URL environment variable is not set")


engine = create_engine(SQLALCHEMY_DATABASE_URL, future=True)
SessionLocal = sessionmaker(autocommit=False, autoflush=False, bind=engine)


def get_db():
db = SessionLocal()
try:
return db
finally:
db.close()
Comment on lines +14 to +19
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Modify get_db to use yield for FastAPI dependency injection

The get_db function should use yield instead of return to work properly as a FastAPI dependency that ensures proper session closure.

 def get_db():
+    """Provides a database session and ensures proper closure."""
     db = SessionLocal()
     try:
-        return db
+        yield db
     finally:
         db.close()
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def get_db():
db = SessionLocal()
try:
return db
finally:
db.close()
def get_db():
"""Provides a database session and ensures proper closure."""
db = SessionLocal()
try:
yield db
finally:
db.close()

Empty file.
28 changes: 28 additions & 0 deletions examples/music-licensing-challenge/src/app/graphql/mutations.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
from typing import Optional

from sqlalchemy.orm import Session
from strawberry import ID, mutation, type

from ..db.database import get_db
from ..repository.songs import SongRepository
from ..schemas.songs import LicenseStatusEnum
from .pubsub import trigger_license_change_subscription
from .types.song import Song


@type
class Mutations:
@mutation
async def update_song(
self,
id: ID,
license_status: Optional[LicenseStatusEnum] = None,
) -> Optional[Song]:
db: Session = get_db()
song_repository = SongRepository(db)
song = song_repository.update_song(id, license_status)
if song is None:
return None
if license_status:
await trigger_license_change_subscription(song)
return Song.from_model(song)
Comment on lines +16 to +28
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve session management and error handling

The mutation correctly implements the song update functionality, but the database session management could be improved. The current implementation doesn't properly close the database session, which might lead to connection leaks.

@mutation
async def update_song(
    self,
    id: ID,
    license_status: Optional[LicenseStatusEnum] = None,
) -> Optional[Song]:
-    db: Session = get_db()
-    song_repository = SongRepository(db)
-    song = song_repository.update_song(id, license_status)
-    if song is None:
-        return None
-    if license_status:
-        await trigger_license_change_subscription(song)
-    return Song.from_model(song)
+    db: Session = get_db()
+    try:
+        song_repository = SongRepository(db)
+        song = song_repository.update_song(id, license_status)
+        if song is None:
+            return None
+        if license_status:
+            await trigger_license_change_subscription(song)
+        return Song.from_model(song)
+    finally:
+        db.close()
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
async def update_song(
self,
id: ID,
license_status: Optional[LicenseStatusEnum] = None,
) -> Optional[Song]:
db: Session = get_db()
song_repository = SongRepository(db)
song = song_repository.update_song(id, license_status)
if song is None:
return None
if license_status:
await trigger_license_change_subscription(song)
return Song.from_model(song)
@mutation
async def update_song(
self,
id: ID,
license_status: Optional[LicenseStatusEnum] = None,
) -> Optional[Song]:
db: Session = get_db()
try:
song_repository = SongRepository(db)
song = song_repository.update_song(id, license_status)
if song is None:
return None
if license_status:
await trigger_license_change_subscription(song)
return Song.from_model(song)
finally:
db.close()

10 changes: 10 additions & 0 deletions examples/music-licensing-challenge/src/app/graphql/pubsub.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import asyncio

from .types.song import Song

song_update_queue: asyncio.Queue[Song] = asyncio.Queue()


async def trigger_license_change_subscription(song_model):
song = Song.from_model(song_model)
await song_update_queue.put(song)
52 changes: 52 additions & 0 deletions examples/music-licensing-challenge/src/app/graphql/queries.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
from typing import List, Optional

from strawberry import ID, field, type

from ..db.database import get_db
from ..repository.licenses import LicenseRepository
from ..repository.movies import MovieRepository
from ..repository.scenes import SceneRepository
from ..repository.songs import SongRepository
from .types.movie import Movie
from .types.scene import Scene
from .types.song import Song
from .types.song import LicenseStatus


@type
class Query:
@field
def all_movies(self) -> List[Movie]:
db = get_db()
movies = MovieRepository(db).get_all_movies_with_details()
return [Movie.from_model(m) for m in movies]

@field
def movie(self, id: ID) -> Optional[Movie]:
db = get_db()
movie = MovieRepository(db).get_movie_by_id_with_details(id)
return Movie.from_model(movie) if movie else None

@field
def scene(self, id: ID) -> Optional[Scene]:
db = get_db()
scene = SceneRepository(db).get_scene_by_id_with_details(id)
return Scene.from_model(scene) if scene else None

@field
def all_scenes(self) -> List[Scene]:
db = get_db()
scenes = SceneRepository(db).get_all_scenes_with_details()
return [Scene.from_model(s) for s in scenes]

@field
def all_license_status(self) -> List[LicenseStatus]:
db = get_db()
licenses = LicenseRepository(db).get_all_licenses()
return [LicenseStatus.from_model(s) for s in licenses]

@field
def song(self, id: ID) -> Optional[Song]:
db = get_db()
song = SongRepository(db).get_song_by_id(id)
return Song.from_model(song) if song else None
Comment on lines +18 to +52
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add exception handling for database operations

The current implementation doesn't handle potential exceptions from database operations. Consider adding try-except blocks to catch and handle database errors gracefully, especially for production environments.

Here's an example implementation for one of the methods:

@field
def movie(self, id: ID) -> Optional[Movie]:
-    db = get_db()
-    movie = MovieRepository(db).get_movie_by_id_with_details(id)
-    return Movie.from_model(movie) if movie else None
+    try:
+        db = get_db()
+        movie = MovieRepository(db).get_movie_by_id_with_details(id)
+        return Movie.from_model(movie) if movie else None
+    except Exception as e:
+        # Log the error
+        # Consider returning a GraphQL error instead of None
+        return None
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@field
def all_movies(self) -> List[Movie]:
db = get_db()
movies = MovieRepository(db).get_all_movies_with_details()
return [Movie.from_model(m) for m in movies]
@field
def movie(self, id: ID) -> Optional[Movie]:
db = get_db()
movie = MovieRepository(db).get_movie_by_id_with_details(id)
return Movie.from_model(movie) if movie else None
@field
def scene(self, id: ID) -> Optional[Scene]:
db = get_db()
scene = SceneRepository(db).get_scene_by_id_with_details(id)
return Scene.from_model(scene) if scene else None
@field
def all_scenes(self) -> List[Scene]:
db = get_db()
scenes = SceneRepository(db).get_all_scenes_with_details()
return [Scene.from_model(s) for s in scenes]
@field
def all_license_status(self) -> List[LicenseStatus]:
db = get_db()
licenses = LicenseRepository(db).get_all_licenses()
return [LicenseStatus.from_model(s) for s in licenses]
@field
def song(self, id: ID) -> Optional[Song]:
db = get_db()
song = SongRepository(db).get_song_by_id(id)
return Song.from_model(song) if song else None
@field
def movie(self, id: ID) -> Optional[Movie]:
try:
db = get_db()
movie = MovieRepository(db).get_movie_by_id_with_details(id)
return Movie.from_model(movie) if movie else None
except Exception as e:
# Log the error
# Consider returning a GraphQL error instead of None
return None

Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import strawberry

from .mutations import Mutations
from .queries import Query
from .subscriptions import Subscription

schema = strawberry.Schema(query=Query, mutation=Mutations, subscription=Subscription)
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
from typing import AsyncGenerator

import strawberry

from .pubsub import song_update_queue
from .types.song import Song


@strawberry.type
class Subscription:
@strawberry.subscription
async def license_changed(self) -> AsyncGenerator[Song, None]:
while True:
song = await song_update_queue.get()
yield song
Empty file.
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import strawberry

from ...models.genre import Genre as GenreModel


@strawberry.type
class Genre:
id: int
name: str

@classmethod
def from_model(cls, model: GenreModel) -> "Genre":
return cls(
id=model.id,
name=model.name,
)
Loading
Loading