Improve docker cache mounts #1495
Conversation
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughThe Dockerfiles for backend, frontend, docs, and schema components were updated to add environment variables for package manager cache directories, create and link these cache directories, and use Docker build cache mounts to optimize caching of package installations. Build steps were modularized and reordered to improve build efficiency and reproducibility. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
backend
frontend
docker
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
frontend/docker/Dockerfile (1)
7-10: Ensure PNPM_HOME is applied to global npm installsThe
PNPM_HOMEenv var is set but npm’s default global prefix is still used, so the pnpm binary ends up in the default location (/usr/local/bin). Consider addingNPM_CONFIG_PREFIX=$PNPM_HOMEso thatnpm install -g pnpmplaces the binary in$PNPM_HOMEand leverages the cache mount effectively.Proposed diff:
ENV APK_CACHE_DIR="/app/.cache/apk" \ APK_SYMLINK_DIR="/etc/apk/cache" \ NPM_CACHE="/app/.npm" \ PNPM_HOME="/pnpm" \ + NPM_CONFIG_PREFIX="/pnpm"backend/docker/Dockerfile (1)
15-19: Optimize apk cache mount with update/upgradeSplitting out
apk update && apk upgradeinto this mount ensures the index is refreshed and stored in the cache. Consider adding--no-cachetoapk upgradeto avoid leaving index files in the image layers:RUN --mount=type=cache,target=${APK_CACHE_DIR} \ - apk update && apk upgrade && \ + apk update && apk upgrade --no-cache && \backend/docker/Dockerfile.local (3)
38-42: Clean up final stage environment variablesCache-related env vars (
APK_CACHE_DIR,PIP_CACHE_DIR) are only needed during build. Consider removing them from the runtime stage to reduce surface area.
44-45: Remove cache directory setup from runtime imageIf you don't install packages at runtime, the
mkdirandln -scommands can be removed to simplify the final image.
52-53: Avoid installing Poetry in the runtime stagePoetry is only needed to create the virtualenv in the builder. Installing it in the final image increases size and isn't used at runtime—consider removing this step.
frontend/docker/Dockerfile.e2e.test (1)
10-11: Cache-enabled global installation of pnpm
Separating the globalpnpminstall with a cache mount speeds up rebuilds by caching npm artifacts.Consider specifying an explicit cache
id(e.g.,id=npm-cache) so the cache persists reliably across builds:-RUN --mount=type=cache,target=${NPM_CACHE} \ - npm install --ignore-scripts -g pnpm --cache ${NPM_CACHE} +RUN --mount=type=cache,id=npm-cache,target=${NPM_CACHE} \ + npm install --ignore-scripts -g pnpm --cache ${NPM_CACHE}frontend/docker/Dockerfile.unit.test (1)
10-11: Cache-driven pnpm global install
Mounting the npm cache for the globalpnpminstall ensures faster rebuilds when upgrading or reinstalling.Optionally, add
id=npm-cacheto the mount to consistently reuse the cache across different build runs:-RUN --mount=type=cache,target=${NPM_CACHE} \ - npm install --ignore-scripts -g pnpm --cache ${NPM_CACHE} +RUN --mount=type=cache,id=npm-cache,target=${NPM_CACHE} \ + npm install --ignore-scripts -g pnpm --cache ${NPM_CACHE}frontend/docker/Dockerfile.local (2)
23-24: Global pnpm install with npm cache mount
Isolating the globalpnpminstallation step with--mount=type=cachespeeds up re-installs; consider addingid=npm-cachefor persistent caching.
49-50: Cache-enabled global pnpm installation in runtime
Adding the npm cache mount forpnpmin the runtime stage maintains consistent dependency versions and speeds up container start times.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (8)
backend/docker/Dockerfile(1 hunks)backend/docker/Dockerfile.local(2 hunks)backend/docker/Dockerfile.test(1 hunks)docs/docker/Dockerfile.local(1 hunks)frontend/docker/Dockerfile(1 hunks)frontend/docker/Dockerfile.e2e.test(1 hunks)frontend/docker/Dockerfile.local(2 hunks)frontend/docker/Dockerfile.unit.test(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (32)
frontend/docker/Dockerfile (2)
14-15: Symlink cache directory for apkCreating
${APK_SYMLINK_DIR}as a symlink to${APK_CACHE_DIR}is correct and ensuresapkuses the cache mount at/app/.cache/apk.
22-23: Leverage Docker cache for npm installGood use of a build cache mount for the npm install step; this will speed up subsequent builds by reusing the cache.
backend/docker/Dockerfile (3)
3-7: Centralize cache and permission environment variablesDefining cache directories (
APK_CACHE_DIR,PIP_CACHE_DIR) and user IDs in oneENVblock improves maintainability and consistency across stages.
12-13: Create cache directories and symlinkThe
mkdir -pandln -scommands correctly prepare the cache locations and redirect/etc/apk/cacheto${APK_CACHE_DIR}for persistent caching.
21-22: Cache pip installs for Poetry setupUsing a build cache mount when installing Poetry with pip will greatly reduce installation time on rebuilds.
docs/docker/Dockerfile.local (5)
5-9: Introduce explicit cache directory environment variablesDefining
APK_CACHE_DIR,APK_SYMLINK_DIR,PIP_CACHE_DIR, etc., at the top keeps this Dockerfile in sync with other backends and frontends.
14-15: Prepare cache directories and symlinkThe
mkdirandln -scommands correctly set up the cache paths.
17-21: Refresh APK index within cache mountRunning
apk update && apk upgradeunder a cache mount makes subsequent builds faster and more reliable.
23-24: Cache pip installation of PoetryMounting the pip cache for the Poetry install step is a great improvement for CI speed.
30-31: Cache Poetry dependenciesMounting
${POETRY_CACHE_DIR}forpoetry installensures dependency resolution is cached across builds.backend/docker/Dockerfile.test (4)
3-9: Align cache env vars in test builderAdding
APK_CACHE_DIR,APK_SYMLINK_DIR,PIP_CACHE_DIR, etc., ensures the test environment mirrors the main builder’s caching strategy.
12-13: Setup cache dirs and symlink in test builderCorrectly prepares
/etc/apk/cacheto point at the cache mount directory.
15-19: Leverage cache mount for apk in testsRefreshing and caching the APK index speeds up test image builds.
21-22: Use pip cache for Poetry installation in test builderThis cache mount will make CI runs for the test image much faster.
backend/docker/Dockerfile.local (6)
5-9: Add builder cache environment variablesSetting
APK_CACHE_DIR,PIP_CACHE_DIR, and related vars ensures the local builder shares the same caching logic.
14-15: Initialize cache directories and link for builderThe
mkdirand symlink steps are correct for redirecting/etc/apk/cache.
17-21: Cache and refresh APK index in local builderGreat use of a cache mount for
apk update && apk upgrade.
23-24: Cache pip install for Poetry in local builderThis will speed up local builds by reusing pip caches.
29-31: Cache Poetry dependencies and skip dev/test depsMounting
${POETRY_CACHE_DIR}and excluding dev/test groups is ideal for local builds.
46-50: Leverage cache for runtimeapk addUsing a cache mount when installing runtime packages (
postgresql-client,redis) is consistent with the builder stage and speeds up local rebuilds.frontend/docker/Dockerfile.e2e.test (3)
3-4: Introduce dedicated cache directories for npm and pnpm
DefiningNPM_CACHEandPNPM_HOMEisolates cache and installation paths, improving build cache reuse.
6-6: Consolidate npm retry configuration
AddingNPM_CONFIG_RETRYenhances resilience by retrying failed network requests during installs.
16-17: Use mounted cache for pnpm store during install
The--mount=type=cache,id=pnpm,target=/pnpm/storeensures the pnpm store is cached between builds, significantly speeding up dependency installs.frontend/docker/Dockerfile.unit.test (3)
3-4: Introduce NPM cache and pnpm home directories
DefiningNPM_CACHEandPNPM_HOMEcreates isolated, versioned caches for npm and pnpm, improving layer reuse.
6-6: Consolidate npm retry and timeout settings
AddingNPM_CONFIG_RETRYoptimizes install reliability; consolidating ENV blocks reduces layers.
16-18: Leverage pnpm store cache and set correct permissions
Using--mount=type=cache,id=pnpm,target=/pnpm/storespeeds up installs, and chainingchown node:node /appensures the unprivileged user can access project files.frontend/docker/Dockerfile.local (6)
5-8: Define explicit cache directories for APK, npm, and pnpm
IntroducingAPK_CACHE_DIR,APK_SYMLINK_DIR,NPM_CACHE, andPNPM_HOMEprovides clear, versioned cache locations for both Alpine packages and Node dependencies.
14-15: Setup APK cache symlink to optimize layer caching
Creating and linking${APK_CACHE_DIR}to/etc/apk/cachewithmkdir -pandln -sensures Alpine’s package cache is reused across builds.
17-21: Use cache mount for APK package installations
Mounting${APK_CACHE_DIR}before runningapk update && apk add gitaccelerates builds by caching downloaded Alpine packages.
36-38: Propagate cache environment variables to runtime stage
Re-declaringAPK_CACHE_DIR,APK_SYMLINK_DIR, andNPM_CACHEin the final image ensures cache mounts remain effective for runtime package operations.
40-42: Recreate APK cache symlink in runtime stage
Ensuring the cache directory and symlink exist in the final image prevents missing cache errors at runtime.
43-47: Cache-driven APK update and setup in runtime
Mounting${APK_CACHE_DIR}forapk update && apk add --no-cache gitand chowning/home/owaspensures both performance and correct permissions.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
frontend/docker/Dockerfile.e2e.test (2)
3-4: Explicit npm and pnpm cache directories set
DeclaringNPM_CACHEandPNPM_HOMEmakes the package manager cache locations clear and consistent.
10-11: Cache the global pnpm installation
Using--mount=type=cache,target=${NPM_CACHE}optimizes rebuilds. Consider adding an explicitid(e.g.,id=npm) to the cache mount to share it across different steps.
🧹 Nitpick comments (7)
frontend/docker/Dockerfile.unit.test (1)
10-11: Cache the global pnpm installation
Using--mount=type=cache,target=${NPM_CACHE}optimizes rebuilds. Consider adding an explicitid(e.g.,id=npm) to the cache mount to share it across different steps.frontend/docker/Dockerfile (1)
22-23: Cache the global pnpm installation
Leveraging the npm cache mount is great. As with other steps, consider specifyingid=npmto share this cache across different build stages.frontend/docker/Dockerfile.local (1)
23-24: Cache the global pnpm installation in builder
Consider specifyingid=npmon the cache mount to share it across steps (e.g.,--mount=type=cache,target=${NPM_CACHE},id=npm).backend/docker/Dockerfile.test (1)
21-22: Cache pip packages during Poetry installation
Mounting${PIP_CACHE_DIR}reduces pip download times. Optionally, you could add--no-cache-dirto thepip installcommand to prevent leftover caches in the final image.backend/docker/Dockerfile (2)
15-16: Optimize APK package operations with cache mounts
The--mount=type=cachedirective for${APK_CACHE_DIR}effectively caches index data. Post-installchownpreserves proper permissions. Consider adding--no-cachetoapk upgradeto avoid storing index metadata in the layer.Also applies to: 19-20
21-22: Accelerate Poetry installer via pip cache mount
Caching pip artifacts speeds up the Poetry bootstrap. As an enhancement, you might pass--no-cache-dirto pip to keep the image lean.docs/docker/Dockerfile.local (1)
23-24: Cache pip for Poetry installation
Mounting the pip cache greatly reduces Poetry installer runtime. If you want a slimmer image, consider passing--no-cache-dirto pip.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (8)
backend/docker/Dockerfile(1 hunks)backend/docker/Dockerfile.local(2 hunks)backend/docker/Dockerfile.test(1 hunks)docs/docker/Dockerfile.local(1 hunks)frontend/docker/Dockerfile(1 hunks)frontend/docker/Dockerfile.e2e.test(1 hunks)frontend/docker/Dockerfile.local(2 hunks)frontend/docker/Dockerfile.unit.test(1 hunks)
🔇 Additional comments (27)
frontend/docker/Dockerfile.unit.test (2)
3-4: Explicit npm and pnpm cache directories set
DeclaringNPM_CACHEandPNPM_HOMEmakes the package manager cache locations clear and consistent.
6-6: Configuration of npm retry and timeout is straightforward and looks good.frontend/docker/Dockerfile.e2e.test (1)
6-6: NPM retry and timeout configuration is standard and acceptable.frontend/docker/Dockerfile (3)
7-10: Define explicit cache directories for apk and npm
DeclaringAPK_CACHE_DIR,APK_SYMLINK_DIR,NPM_CACHE, andPNPM_HOMEcentralizes cache paths and improves maintainability.
14-15: Create and link cache directories
Ensuring${APK_CACHE_DIR}exists and is symlinked to the default apk cache directory maintains compatibility and leverages the cache mount correctly.
17-18: Optimize apk installations with build cache
Using--mount=type=cache,target=${APK_CACHE_DIR}beforeapk update && apk upgrade && apk add libc6-compatspeeds up rebuilds.frontend/docker/Dockerfile.local (3)
5-8: Define explicit builder cache directories
DeclaringAPK_CACHE_DIR,APK_SYMLINK_DIR,NPM_CACHE, andPNPM_HOMEin the builder stage standardizes cache paths.
14-15: Create and link builder cache directories
Ensuring${APK_CACHE_DIR}exists and points to/etc/apk/cacheallows the build cache to persist across runs.
17-21: Optimize builder APK installation with cache mount
Using--mount=type=cache,target=${APK_CACHE_DIR}speeds upapk update && apk add git.backend/docker/Dockerfile.local (8)
5-9: Define builder cache and user environment variables
DeclaringAPK_CACHE_DIR,APK_SYMLINK_DIR,OWASP_GID,PIP_CACHE_DIR, and related variables centralizes directory paths and user IDs.
14-15: Prepare and link builder cache directories
Creating${APK_CACHE_DIR}and symlinking to/etc/apk/cacheensures the APK cache persists across builds.
17-21: Optimize builder APK installation with cache mount
Mounting${APK_CACHE_DIR}beforeapk updateandapk addspeeds up rebuilds and leverages cached indexes.
23-24: Cache Poetry installer with pip cache mount
Using--mount=type=cache,target=${PIP_CACHE_DIR}forpip install poetryimproves rebuild performance.
37-41: Define runtime cache and PATH environment variables
DeclaringAPK_CACHE_DIR,APK_SYMLINK_DIR,PIP_CACHE_DIR, and updatingPATHfor the virtualenv sets up the runtime environment properly.
43-45: Prepare and link runtime cache directories
Ensuring${APK_CACHE_DIR}exists and is linked maintains caching behavior in the runtime stage.
46-47: Optimize runtime APK installation with cache mount
Leveraging the build cache forapk updateandapk addspeeds up container startup builds.Also applies to: 50-50
52-53: Cache Poetry installer again in runtime stage
Mounting${PIP_CACHE_DIR}forpip install poetryensures fast repeated builds.backend/docker/Dockerfile.test (3)
3-7: ENV declarations for cache and user variables are well-structured
CentralizingAPK_CACHE_DIR,APK_SYMLINK_DIR,PIP_CACHE_DIR, and UID/GID settings viaENVimproves clarity and makes future cache-directory tweaks straightforward.
12-13: Pre-create cache dirs and symlink before use
Creating${APK_CACHE_DIR}&${POETRY_CACHE_DIR}and symlinking/etc/apk/cacheensures the build mounts will be effective and keepsapkhappy.
15-16: Use Docker cache mount for APK indices and enforce ownership
Leveraging--mount=type=cache,target=${APK_CACHE_DIR}speeds up successiveapk update && apk upgraderuns. The subsequentchown -R owasp:owasp /home/owaspguarantees theowaspuser can access all files.Also applies to: 19-20
backend/docker/Dockerfile (3)
3-7: Consistent environment parameterization
DefiningAPK_CACHE_DIR,APK_SYMLINK_DIR,PIP_CACHE_DIR, and user IDs viaENVmatches the test and local Dockerfiles, promoting uniform cache behavior across environments.
12-13: Initialize cache dirs & symlink for apk
Early creation of the cache paths and symlink ensures subsequent mounts are wired correctly forapk.
29-30: Leverage cache for Poetry dependencies
Mounting${POETRY_CACHE_DIR}forpoetry installavoids re-downloading packages. Excluding dev/test groups further slims the image.docs/docker/Dockerfile.local (4)
5-7: Standardize cache & user ENV variables
IntroducingAPK_CACHE_DIR,APK_SYMLINK_DIR, andPIP_CACHE_DIRalongside UID/GID settings keeps this Dockerfile aligned with backend builds.
14-15: Establish cache dirs and symlink for apk
Pre-creating and linking the APK cache directory ensures Docker’s build cache mount works seamlessly withapk update/upgrade.
17-21: Improve build speed with APK cache mount + ownership fix
Using--mount=type=cache,target=${APK_CACHE_DIR}caches package indexes, andchown -R owasp:owaspmakes the home directory writable by the non-root user.
30-32: Cache Poetry deps and clean up lock files
Mounting${POETRY_CACHE_DIR}and removingdocs/poetry.lock&docs/pyproject.tomlin the same step keeps the image clean and reproducible.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/run-ci-cd.yaml (1)
174-181: Consider adding fallback restore-keys for the Poetry cache.
Theactions/cachestep pins on the exactbackend/poetry.lockhash, which may lead to cache misses on minor changes. To improve cache hit rates, you can add arestore-keyssection, e.g.:restore-keys: | buildx-backend-poetry-cache-${{ runner.os }}- buildx-backend-poetry-cache-This is an optional enhancement to reduce cold-starts.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
.github/workflows/run-ci-cd.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
🔇 Additional comments (5)
.github/workflows/run-ci-cd.yaml (5)
171-173: Ensure Buildx is initialized before caching and builds.
The newSet up Docker buildxstep correctly initializes BuildKit, which is required for the subsequent cache mounts and build steps.
182-190: Restore APK and Pip caches in one step.
The combined restore ofcache-apkandcache-pipis efficient and keys on the Dockerfile hash. Good job enabling caching for both Alpine packages and Python wheels.
191-199: Verify cache-map paths match Dockerfile mount points.
Please ensure that the host directories (cache-poetry,cache-apk,cache-pip) and the container paths (/home/owasp/.cache/pypoetry,/home/owasp/.cache/apk,/home/owasp/.cache/pip) correspond exactly to the locations created and used inbackend/docker/Dockerfile.test. Misalignment can lead to stale or unused caches.
205-207: Use GitHub Actions cache for Buildxcache-from.
Switching totype=ghaleverages GitHub Actions cache for build layers, and including the registry fallback (type=registry) is a robust approach. This aligns with best practices for cross-run caching.
208-208: Push Buildx cache to GitHub Actions.
Usingcache-to: type=gha,mode=maxensures that the build layers are retained for future workflow runs, maximizing cache reuse.
|
@coderabbitai Suggest a way to inject the cache came from restoring cache step to the docker builder |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@ahmedxgouda Here are several effective ways to inject cache from GitHub Actions restore steps into your Docker builder: 1. Extend the buildkit-cache-dance approach to backend buildsI see you're already using this for frontend tests. This approach creates a temporary directory structure that maps host caches to Docker build context paths: - name: Inject restored cache into Docker build environment
uses: reproducible-containers/buildkit-cache-dance@v2
with:
cache-source: ./cache-pnpm
cache-target: /root/.local/share/pnpm/store/v3You could apply the same to backend caches: - name: Inject poetry cache into Docker build environment
uses: reproducible-containers/buildkit-cache-dance@v2
with:
cache-source: ./poetry-cache
cache-target: /home/owasp/.cache/poetry
- name: Inject pip cache into Docker build environment
uses: reproducible-containers/buildkit-cache-dance@v2
with:
cache-source: ./pip-cache
cache-target: /home/owasp/.cache/pip2. Use Docker's --mount flag with BuildKitYou can directly mount the host cache directory when running - name: Build and push
uses: docker/build-push-action@v5
with:
context: ./backend
file: ./backend/docker/Dockerfile.test
push: false
load: true
tags: ${{ steps.meta.outputs.tags }}
cache-from: type=gha
cache-to: type=gha,mode=max
build-args: |
BUILDKIT_CONTEXT_KEEP_GIT_DIR=1
build-contexts: |
cache-poetry=local:./poetry-cache
cache-pip=local:./pip-cache
cache-apk=local:./apk-cacheThen in your Dockerfile: # Use the cache from build context
RUN --mount=type=bind,from=cache-poetry,source=/,target=/home/owasp/.cache/poetry \
--mount=type=bind,from=cache-pip,source=/,target=/home/owasp/.cache/pip \
# Your commands3. Use local BuildKit frontend cache (instead of GHA)If the GitHub Actions cache type doesn't provide enough flexibility, you can use a local directory: - name: Build and push
uses: docker/build-push-action@v5
with:
context: ./backend
file: ./backend/docker/Dockerfile.test
push: false
load: true
tags: ${{ steps.meta.outputs.tags }}
cache-from: type=local,src=./docker-cache
cache-to: type=local,dest=./docker-cache,mode=max4. Use multi-stage build with intermediate cache stagesCreate a dedicated cache stage in your Dockerfile: FROM alpine as cache-import
COPY ./poetry-cache /cache/poetry
COPY ./pip-cache /cache/pip
COPY ./apk-cache /cache/apk
FROM base-image
# Copy from cache stage
COPY --from=cache-import /cache/poetry /home/owasp/.cache/poetry
COPY --from=cache-import /cache/pip /home/owasp/.cache/pip
COPY --from=cache-import /cache/apk /etc/apk/cache5. Mount directories directly with Docker's --mount flagFor a simpler approach, directly mount your cache directories when running tests: - name: Run backend tests
run: |
docker run --rm \
--mount type=bind,source=${{ github.workspace }}/poetry-cache,target=/home/owasp/.cache/poetry \
--mount type=bind,source=${{ github.workspace }}/pip-cache,target=/home/owasp/.cache/pip \
--mount type=bind,source=${{ github.workspace }}/apk-cache,target=/etc/apk/cache \
your-image-nameThis approach eliminates the need to copy files back and forth after tests. RecommendationBased on your current setup, extending the buildkit-cache-dance approach to backend builds would be most consistent and efficient. It avoids the need to copy cache directories after the build completes and provides a unified approach across frontend and backend builds. 🏁 Script executed: Length of output: 10741 🏁 Script executed: Length of output: 5342 🏁 Script executed: Length of output: 2449 🌐 Web query: 💡 Result: The BuildKit Cache Dance is a GitHub Action designed to optimize Docker builds by preserving RUN --mount=type=cache caches between CI/CD runs, particularly on platforms like GitHub Actions where build environments are ephemeral. Purpose and Problem SolvedThe BuildKit Cache Dance addresses a significant limitation in Docker's BuildKit caching mechanism. While BuildKit automatically caches build results internally, these caches are typically lost between CI/CD runs since the environments are ephemeral[7]. This is particularly problematic for commands like package installations, asset compilation, and other time-consuming operations that benefit from caching[6]. By default, BuildKit doesn't preserve cache mounts in the GitHub Actions cache[1]. This means operations that use cache mounts (like How It WorksThe BuildKit Cache Dance works through a process of extracting and injecting cache data:
This workflow essentially "dances" the cache data in and out of Docker builds, preserving it between ephemeral CI environments. Implementation ExampleA typical implementation in GitHub Actions involves these steps: - name: Set up Docker Buildx
uses: docker/setup-buildx-action@v3
- name: Cache Docker layers
uses: actions/cache@v4
with:
path: cache-data
key: ${{ runner.os }}-buildx-${{ github.sha }}
restore-keys: |
${{ runner.os }}-buildx-
- name: BuildKit Cache Dance
uses: reproducible-containers/[email protected]
with:
cache-map: |
"dependency-cache": "/app/cache"
- name: Build
uses: docker/build-push-action@v6
with:
context: .
push: true
tags: user/app:latestIn your Dockerfile, you would use cache mounts: FROM node:16-alpine
WORKDIR /app
COPY package.json .
# This cache mount will be preserved between builds
RUN --mount=type=cache,target=/app/cache npm install
COPY . .
RUN npm run buildBenefits and LimitationsBenefits:
Limitations:
The BuildKit Cache Dance has proven especially valuable for large monolithic applications where build times can be prohibitively long without effective caching strategies[6]. Citations:
@ahmedxgouda I've examined your caching implementation and found you're already using buildkit-cache-dance for some caches. Here are improved ways to inject cache from GitHub Actions into your Docker builder: 1. Streamline your buildkit-cache-dance implementationI see you're already using buildkit-cache-dance for some cache directories but with multiple steps. You can simplify this by using a consistent approach with a single call: - name: Inject all caches into docker
uses: reproducible-containers/[email protected]
with:
cache-map: |
"./poetry-cache": "/home/owasp/.cache/pypoetry"
"./pip-cache": "/home/owasp/.cache/pip"
"./apk-cache": "/home/owasp/.cache/apk"2. Use BuildKit's cache-from with explicit sourceYour current approach uses the gha cache type, but you can be more explicit by using the local cache type with your restored cache directories: - name: Build backend test image
uses: docker/build-push-action@v5
with:
context: backend
file: backend/docker/Dockerfile.test
load: true
platforms: linux/amd64
tags: ${{ env.DOCKERHUB_USERNAME }}/owasp-nest-test-backend:latest
cache-from: |
type=gha
type=local,src=./cache-dir
cache-to: |
type=gha,mode=max3. Eliminate the copy operations post-testI noticed you're copying cache files out after tests run. Instead, modify your Dockerfile.test to use the injected caches directly without copying into intermediate GitHub directories: # In Dockerfile.test
# ENV block
ENV APK_CACHE_DIR="/home/owasp/.cache/apk" \
PIP_CACHE_DIR="/home/owasp/.cache/pip" \
POETRY_CACHE_DIR="/home/owasp/.cache/pypoetry" \
PYTHONUNBUFFERED=1
# Use the BuildKit cache mounts directly
RUN --mount=type=cache,target=${APK_CACHE_DIR} \
apk update && apk upgrade && ...
RUN --mount=type=cache,target=${PIP_CACHE_DIR} \
python -m pip install poetry --cache-dir ${PIP_CACHE_DIR}
# No copies to intermediate directories neededThen in your workflow, use volumes to preserve caches during test runs: - name: Run backend tests
id: run-tests
run: |
CONTAINER_ID=$(docker run -d \
-v ./poetry-cache:/home/owasp/.cache/pypoetry \
-v ./pip-cache:/home/owasp/.cache/pip \
-v ./apk-cache:/home/owasp/.cache/apk \
-e DJANGO_CONFIGURATION=Test \
${{ env.DOCKERHUB_USERNAME }}/owasp-nest-test-backend:latest pytest)
echo "container_id=$CONTAINER_ID" >> $GITHUB_OUTPUT
docker wait ${CONTAINER_ID}
docker logs ${CONTAINER_ID}4. Use BuildKit's build contexts for cache injectionThis is a more elegant solution that avoids external tools: - name: Build backend test image
uses: docker/build-push-action@v5
with:
context: backend
file: backend/docker/Dockerfile.test
load: true
platforms: linux/amd64
tags: ${{ env.DOCKERHUB_USERNAME }}/owasp-nest-test-backend:latest
build-contexts: |
poetry-cache=./poetry-cache
pip-cache=./pip-cache
apk-cache=./apk-cacheThen in your Dockerfile: # Copy from named build contexts
COPY --from=poetry-cache / /home/owasp/.cache/pypoetry/
COPY --from=pip-cache / /home/owasp/.cache/pip/
COPY --from=apk-cache / /home/owasp/.cache/apk/
# Then use cache mounts as normal
RUN --mount=type=cache,target=/home/owasp/.cache/pypoetry ... RecommendationI recommend the buildkit-cache-dance approach (#1) since you're already using it successfully for some caches. It handles the mapping between host and container paths elegantly, and avoids complicated copy operations. The volume mounting approach (#3) would be my second choice for simplicity and directness. |
|
I should stop using the VS Code GUI for commiting |
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (2)
frontend/src/components/Search.tsx (1)
4-4:⚠️ Potential issueIncomplete migration from GTM to GA event tracking.
The parameter keys have been updated but the import and function call still use
sendGTMEventinstead ofsendGAEvent. This inconsistency suggests an incomplete migration.Apply this diff to complete the migration:
-import { sendGTMEvent } from '@next/third-parties/google' +import { sendGAEvent } from '@next/third-parties/google'- sendGTMEvent({ - event: 'search', + sendGAEvent({ + event: 'homepageSearch', path: window.location.pathname, value: query, })Also applies to: 37-42
backend/apps/github/graphql/nodes/release.py (1)
51-55:⚠️ Potential issueCritical: Circular reference in url resolver
The resolver returns
self.urlwhich will cause an infinite recursion when the field is accessed.Based on the
Releasemodel snippet showing aurlproperty, this should work correctly if strawberry properly distinguishes between the resolver method and the model property. However, to avoid confusion, consider accessing it more explicitly:def url(self) -> str: """Resolve URL.""" - return self.url + return self.url # This refers to the model property, not the resolver methodNote: If this is causing issues, consider renaming the resolver method to avoid name collision with the model property.
♻️ Duplicate comments (1)
backend/tests/apps/owasp/graphql/queries/project_test.py (1)
64-65: Same resolver invocation pattern concern.The same unusual resolver invocation pattern is used here. Please ensure this is the correct way to test Strawberry resolvers.
🧹 Nitpick comments (8)
backend/Makefile (3)
15-17: Add error redirection and phony declaration forexec-db-command-itThe new
exec-db-command-ittarget is useful, but to mirrorexec-backend-command-it’s behavior you should either append2>/dev/nullhere or remove the redirection from both targets to maintain consistent error handling. Additionally, addexec-db-command-itto your.PHONYlist to avoid conflicts if a file by that name ever appears in the tree.
139-142: Expose failures inrecreate-schemaby removing silent redirectionSilencing the
psqlerrors with2>/dev/nullhides any issues during schema recreation and makes CI failures harder to troubleshoot. Drop the redirection so that any errors bubble up in your logs.
150-152: Declareshell-dbas phony and ensure proper Makefile indentationThe
shell-dbshortcut is handy, but add it to the.PHONYdeclaration at the top of your Makefile to prevent naming collisions. Also verify the recipe line is indented with a tab character rather than spaces, as required by GNU Make.backend/tests/apps/github/graphql/queries/issue_test.py (1)
22-36: Test structure is solid but consider more realistic mocking.The basic test correctly verifies the method behavior, but the mocking approach creates some coupling to implementation details.
Consider using Django's test database instead of mocking for more realistic testing:
-@patch("apps.github.models.issue.Issue.objects.select_related") -def test_recent_issues_basic(self, mock_select_related, mock_issue): +@pytest.mark.django_db +def test_recent_issues_basic(self): """Test fetching recent issues with default parameters.""" - mock_queryset = MagicMock() - mock_queryset.order_by.return_value = [mock_issue] - mock_select_related.return_value = mock_queryset + # Create test data using factories or model.objects.create() result = IssueQuery().recent_issues() - assert result == [mock_issue] - mock_select_related.assert_called_once_with( - "author", "repository", "repository__organization" - ) - mock_queryset.order_by.assert_called_once_with("-created_at") + # Assert on actual data and behaviorbackend/apps/owasp/graphql/queries/committee.py (1)
15-24: Update docstring to remove reference to unused parameter.The docstring still mentions the
infoparameter which is no longer used in Strawberry resolvers.Apply this diff to fix the docstring:
"""Resolve committee by key. Args: - info: GraphQL execution info. key (str): The key of the committee. Returns: CommitteeNode: The committee object if found, otherwise None. """backend/apps/github/graphql/nodes/repository.py (2)
38-43: Consider addressing the TODO commentThe TODO suggests renaming this method to
recent_issuesfor clarity, which would better describe what the method actually returns.
69-74: Consider addressing the TODO commentThe TODO suggests renaming this method to
recent_releasesfor clarity, which would better describe what the method actually returns.backend/tests/apps/github/graphql/nodes/repository_test.py (1)
1-1: Update the docstring to match the test content.The docstring still refers to "ProjectNode" which is correct, but given this is in a file named
repository_test.py, this might be confusing. Consider renaming the file toproject_test.pyto match the actual test content.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (67)
.github/workflows/run-ci-cd.yaml(1 hunks).pre-commit-config.yaml(1 hunks)backend/Makefile(3 hunks)backend/apps/common/graphql/nodes.py(0 hunks)backend/apps/common/graphql/queries.py(0 hunks)backend/apps/github/graphql/nodes/issue.py(1 hunks)backend/apps/github/graphql/nodes/milestone.py(1 hunks)backend/apps/github/graphql/nodes/organization.py(3 hunks)backend/apps/github/graphql/nodes/pull_request.py(1 hunks)backend/apps/github/graphql/nodes/release.py(1 hunks)backend/apps/github/graphql/nodes/repository.py(2 hunks)backend/apps/github/graphql/nodes/repository_contributor.py(1 hunks)backend/apps/github/graphql/nodes/user.py(1 hunks)backend/apps/github/graphql/queries/issue.py(1 hunks)backend/apps/github/graphql/queries/milestone.py(1 hunks)backend/apps/github/graphql/queries/organization.py(1 hunks)backend/apps/github/graphql/queries/pull_request.py(2 hunks)backend/apps/github/graphql/queries/release.py(1 hunks)backend/apps/github/graphql/queries/repository.py(2 hunks)backend/apps/github/graphql/queries/repository_contributor.py(1 hunks)backend/apps/github/graphql/queries/user.py(2 hunks)backend/apps/owasp/graphql/nodes/chapter.py(1 hunks)backend/apps/owasp/graphql/nodes/committee.py(1 hunks)backend/apps/owasp/graphql/nodes/common.py(1 hunks)backend/apps/owasp/graphql/nodes/event.py(1 hunks)backend/apps/owasp/graphql/nodes/post.py(1 hunks)backend/apps/owasp/graphql/nodes/project.py(2 hunks)backend/apps/owasp/graphql/nodes/snapshot.py(2 hunks)backend/apps/owasp/graphql/nodes/sponsor.py(1 hunks)backend/apps/owasp/graphql/nodes/stats.py(1 hunks)backend/apps/owasp/graphql/queries/chapter.py(1 hunks)backend/apps/owasp/graphql/queries/committee.py(1 hunks)backend/apps/owasp/graphql/queries/event.py(1 hunks)backend/apps/owasp/graphql/queries/post.py(1 hunks)backend/apps/owasp/graphql/queries/project.py(1 hunks)backend/apps/owasp/graphql/queries/snapshot.py(2 hunks)backend/apps/owasp/graphql/queries/sponsor.py(1 hunks)backend/apps/owasp/graphql/queries/stats.py(3 hunks)backend/docker/Dockerfile.test(1 hunks)backend/pyproject.toml(4 hunks)backend/settings/base.py(0 hunks)backend/settings/graphql.py(1 hunks)backend/settings/urls.py(2 hunks)backend/tests/apps/common/graphql/nodes_test.py(0 hunks)backend/tests/apps/common/graphql/queries_test.py(0 hunks)backend/tests/apps/github/graphql/nodes/issue_test.py(1 hunks)backend/tests/apps/github/graphql/nodes/milestone_test.py(2 hunks)backend/tests/apps/github/graphql/nodes/organization_test.py(2 hunks)backend/tests/apps/github/graphql/nodes/release_test.py(2 hunks)backend/tests/apps/github/graphql/nodes/repository_test.py(1 hunks)backend/tests/apps/github/graphql/nodes/user_test.py(2 hunks)backend/tests/apps/github/graphql/queries/issue_test.py(1 hunks)backend/tests/apps/github/graphql/queries/milestone_test.py(1 hunks)backend/tests/apps/github/graphql/queries/repository_test.py(5 hunks)backend/tests/apps/github/graphql/queries/user_test.py(1 hunks)backend/tests/apps/owasp/graphql/nodes/project_test.py(1 hunks)backend/tests/apps/owasp/graphql/queries/chapter_test.py(1 hunks)backend/tests/apps/owasp/graphql/queries/committee_test.py(1 hunks)backend/tests/apps/owasp/graphql/queries/project_test.py(1 hunks)frontend/src/app/about/page.tsx(2 hunks)frontend/src/app/layout.tsx(2 hunks)frontend/src/app/page.tsx(1 hunks)frontend/src/components/CardDetailsPage.tsx(2 hunks)frontend/src/components/MultiSearch.tsx(2 hunks)frontend/src/components/Search.tsx(1 hunks)frontend/src/server/queries/projectQueries.ts(1 hunks)frontend/src/types/project.ts(1 hunks)
💤 Files with no reviewable changes (5)
- backend/apps/common/graphql/queries.py
- backend/tests/apps/common/graphql/queries_test.py
- backend/apps/common/graphql/nodes.py
- backend/settings/base.py
- backend/tests/apps/common/graphql/nodes_test.py
✅ Files skipped from review due to trivial changes (5)
- .pre-commit-config.yaml
- frontend/src/app/page.tsx
- .github/workflows/run-ci-cd.yaml
- backend/apps/github/graphql/nodes/repository_contributor.py
- backend/apps/owasp/graphql/nodes/post.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/docker/Dockerfile.test
🧰 Additional context used
🧬 Code Graph Analysis (25)
backend/tests/apps/github/graphql/nodes/issue_test.py (1)
backend/apps/github/graphql/nodes/issue.py (1)
IssueNode(19-39)
frontend/src/app/layout.tsx (1)
frontend/src/utils/credentials.ts (1)
GTM_ID(5-5)
backend/apps/owasp/graphql/queries/post.py (2)
backend/apps/owasp/graphql/nodes/post.py (1)
PostNode(18-19)backend/apps/owasp/models/post.py (2)
Post(11-80)recent_posts(37-39)
backend/apps/owasp/graphql/nodes/event.py (1)
backend/apps/owasp/models/event.py (1)
Event(22-293)
backend/tests/apps/github/graphql/nodes/user_test.py (2)
backend/tests/apps/github/graphql/nodes/release_test.py (1)
test_meta_configuration(13-26)backend/tests/apps/github/graphql/nodes/organization_test.py (1)
test_meta_configuration(13-30)
backend/apps/owasp/graphql/queries/sponsor.py (2)
backend/apps/owasp/graphql/nodes/sponsor.py (1)
SponsorNode(17-18)backend/apps/owasp/models/sponsor.py (1)
Sponsor(12-156)
backend/settings/graphql.py (2)
backend/apps/github/graphql/queries/__init__.py (1)
GithubQuery(13-23)backend/apps/owasp/graphql/queries/__init__.py (1)
OwaspQuery(13-23)
backend/apps/owasp/graphql/nodes/sponsor.py (1)
backend/apps/owasp/models/sponsor.py (1)
Sponsor(12-156)
backend/tests/apps/github/graphql/nodes/release_test.py (4)
backend/apps/github/graphql/nodes/release.py (1)
ReleaseNode(20-54)backend/tests/apps/github/graphql/nodes/organization_test.py (1)
test_meta_configuration(13-30)backend/tests/apps/github/graphql/nodes/user_test.py (1)
test_meta_configuration(13-35)backend/apps/github/graphql/nodes/user.py (1)
UserNode(26-52)
backend/apps/owasp/graphql/queries/stats.py (2)
backend/apps/owasp/graphql/nodes/stats.py (1)
StatsNode(7-14)backend/apps/common/utils.py (1)
round_down(117-128)
backend/tests/apps/github/graphql/nodes/organization_test.py (5)
backend/apps/github/graphql/nodes/organization.py (2)
OrganizationNode(39-76)OrganizationStatsNode(13-20)backend/tests/apps/github/graphql/nodes/release_test.py (1)
test_meta_configuration(13-26)backend/tests/apps/github/graphql/nodes/user_test.py (1)
test_meta_configuration(13-35)backend/tests/apps/github/graphql/nodes/repository_test.py (1)
test_meta_configuration(15-39)backend/tests/apps/owasp/graphql/nodes/project_test.py (1)
test_meta_configuration(15-39)
backend/apps/owasp/graphql/queries/event.py (2)
backend/apps/owasp/graphql/nodes/event.py (1)
EventNode(22-23)backend/apps/owasp/models/event.py (2)
Event(22-293)upcoming_events(64-75)
backend/tests/apps/github/graphql/nodes/milestone_test.py (1)
backend/apps/github/graphql/nodes/milestone.py (1)
MilestoneNode(22-50)
backend/apps/github/graphql/queries/issue.py (2)
backend/apps/github/graphql/nodes/issue.py (1)
IssueNode(19-39)backend/apps/github/models/issue.py (1)
Issue(18-212)
backend/apps/owasp/graphql/nodes/committee.py (3)
backend/apps/owasp/graphql/nodes/common.py (1)
GenericEntityNode(9-35)backend/apps/owasp/graphql/queries/committee.py (1)
committee(14-28)backend/apps/owasp/models/committee.py (1)
Committee(15-86)
backend/apps/owasp/graphql/queries/chapter.py (2)
backend/apps/owasp/graphql/nodes/chapter.py (2)
ChapterNode(31-56)key(49-51)backend/apps/owasp/models/chapter.py (1)
Chapter(20-207)
backend/apps/github/graphql/nodes/organization.py (9)
backend/apps/github/graphql/nodes/repository.py (2)
organization(55-57)url(86-88)backend/apps/github/graphql/queries/organization.py (1)
organization(14-31)backend/apps/github/models/organization.py (1)
Organization(12-91)backend/apps/github/models/repository.py (1)
Repository(25-348)backend/apps/github/models/repository_contributor.py (1)
RepositoryContributor(14-207)backend/apps/github/graphql/nodes/pull_request.py (1)
url(40-42)backend/apps/github/graphql/nodes/release.py (1)
url(52-54)backend/apps/github/graphql/nodes/user.py (1)
url(50-52)backend/apps/owasp/graphql/nodes/common.py (1)
url(33-35)
backend/apps/github/graphql/queries/repository.py (2)
backend/apps/github/graphql/nodes/repository.py (2)
RepositoryNode(35-88)organization(55-57)backend/apps/github/models/repository.py (1)
Repository(25-348)
backend/apps/github/graphql/nodes/user.py (3)
backend/apps/github/graphql/queries/user.py (1)
user(40-56)backend/apps/github/models/user.py (1)
User(14-129)backend/apps/github/models/mixins/user.py (4)
idx_created_at(32-34)idx_issues_count(133-135)idx_releases_count(159-161)idx_updated_at(164-166)
backend/apps/owasp/graphql/queries/snapshot.py (2)
backend/apps/owasp/graphql/nodes/snapshot.py (2)
SnapshotNode(26-57)key(30-32)backend/apps/owasp/models/snapshot.py (1)
Snapshot(7-47)
backend/apps/owasp/graphql/nodes/chapter.py (3)
backend/apps/owasp/graphql/nodes/common.py (1)
GenericEntityNode(9-35)backend/apps/owasp/graphql/queries/chapter.py (1)
chapter(14-19)backend/apps/owasp/models/chapter.py (1)
Chapter(20-207)
backend/apps/github/graphql/nodes/repository.py (7)
backend/apps/github/graphql/nodes/issue.py (1)
IssueNode(19-39)backend/apps/github/graphql/nodes/milestone.py (1)
MilestoneNode(22-50)backend/apps/github/graphql/nodes/organization.py (2)
OrganizationNode(39-76)url(74-76)backend/apps/github/models/repository.py (5)
Repository(25-348)latest_release(123-130)recent_milestones(172-176)published_releases(163-169)url(188-190)backend/apps/github/graphql/nodes/release.py (2)
ReleaseNode(20-54)url(52-54)backend/apps/github/graphql/queries/repository_contributor.py (1)
top_contributors(14-59)backend/apps/github/graphql/nodes/repository_contributor.py (1)
RepositoryContributorNode(7-15)
backend/apps/github/graphql/nodes/issue.py (5)
backend/apps/github/graphql/nodes/user.py (1)
UserNode(26-52)backend/apps/github/graphql/nodes/milestone.py (3)
author(26-28)organization_name(31-37)repository_name(48-50)backend/apps/github/graphql/nodes/pull_request.py (3)
author(21-23)organization_name(26-32)repository_name(35-37)backend/apps/github/graphql/nodes/release.py (3)
author(24-26)organization_name(29-35)repository_name(47-49)backend/apps/github/graphql/nodes/repository.py (1)
organization(55-57)
backend/apps/github/graphql/nodes/release.py (5)
backend/apps/github/graphql/nodes/user.py (2)
UserNode(26-52)url(50-52)backend/apps/github/models/release.py (2)
Release(10-134)url(63-65)backend/apps/github/graphql/nodes/milestone.py (3)
author(26-28)organization_name(31-37)repository_name(48-50)backend/apps/github/graphql/nodes/issue.py (3)
author(23-25)organization_name(28-34)repository_name(37-39)backend/apps/github/graphql/nodes/pull_request.py (4)
author(21-23)organization_name(26-32)repository_name(35-37)url(40-42)
backend/apps/owasp/graphql/nodes/common.py (3)
backend/apps/github/graphql/nodes/repository_contributor.py (1)
RepositoryContributorNode(7-15)backend/apps/owasp/models/mixins/common.py (1)
idx_leaders(18-20)backend/apps/github/graphql/queries/repository_contributor.py (1)
top_contributors(14-59)
🔇 Additional comments (131)
frontend/src/components/MultiSearch.tsx (2)
11-11: LGTM! Analytics migration properly implemented.The import change from
sendGTMEventtosendGAEventcorrectly implements the migration from Google Tag Manager to Google Analytics tracking.
48-52: LGTM! Event tracking parameters updated correctly.The event tracking implementation has been properly updated with:
- Event name changed to
homepageSearchfor better specificity- Parameter keys updated to
valueandpathto align with GA conventions- Conditional logic preserved to ensure tracking only occurs for non-empty queries
frontend/src/server/queries/projectQueries.ts (2)
106-106: Good practice: Added pagination limit.Adding
limit: 25prevents over-fetching and improves query performance while providing sufficient data for UI display.
111-111: Enhanced data retrieval with state field.Adding the
statefield provides additional milestone information that aligns with the backend schema migration to Strawberry GraphQL.backend/tests/apps/github/graphql/nodes/user_test.py (2)
11-11: Correct migration to Strawberry GraphQL type checking.The test correctly validates that
UserNodeis a Strawberry GraphQL type by checking for the__strawberry_definition__attribute, replacing the previous Graphene-based inheritance check.
15-35: Proper Strawberry field validation.The meta configuration test has been correctly updated to extract field names from Strawberry's type definition. The expected field names are comprehensive and the testing pattern matches other migrated test files in the codebase.
backend/settings/urls.py (1)
13-13: LGTM! Proper Strawberry GraphQL migration.The migration from Graphene to Strawberry GraphQL is correctly implemented:
- Import updated to use Strawberry's GraphQLView
- Schema properly imported and passed to the view
- GraphiQL configuration preserved correctly
Also applies to: 20-20, 29-29
backend/tests/apps/github/graphql/nodes/issue_test.py (2)
9-10: Correct test update for Strawberry migration.The test properly checks for Strawberry-specific attributes instead of inheritance-based validation from Graphene.
12-23:✅ Verification successful
Verify field completeness against the actual IssueNode implementation.
The test correctly extracts field names from Strawberry's definition, but ensure all fields from the IssueNode are included in the expected set.
🏁 Script executed:
#!/bin/bash # Description: Verify all fields in IssueNode are covered by the test # Expected: All Strawberry field definitions should be included in test expectations # Search for field definitions in IssueNode ast-grep --pattern '@strawberry.field def $FIELD_NAME($_) -> $_: $$$' # Also search for any strawberry_django.field decorators rg -A 2 "@strawberry" backend/apps/github/graphql/nodes/issue.pyLength of output: 29831
🏁 Script executed:
#!/bin/bash # Extract the strawberry_django.type decorator and its fields list in IssueNode rg -n "@strawberry_django.type" -A 10 backend/apps/github/graphql/nodes/issue.pyLength of output: 288
All IssueNode fields are covered in the test
Theexpected_field_namesexactly matches the fields listed in the@strawberry_django.type(..., fields=[…])decorator (created_at,state,title,url) plus the three@strawberry.fieldmethods (author,organization_name,repository_name). No changes required.backend/pyproject.toml (3)
100-100: LGTM! Proper mypy plugin configuration.Adding the Strawberry mypy plugin is necessary for proper type checking of Strawberry GraphQL code.
110-112: Appropriate mypy overrides for migration.The error suppressions for GraphQL modules are reasonable during the migration period to avoid false positives while the codebase transitions to Strawberry.
80-80: Minor improvement to test coverage reporting.Increasing coverage precision provides more detailed coverage reporting.
frontend/src/app/about/page.tsx (2)
166-220: Excellent improvement to roadmap display logic.The filtering logic correctly excludes closed milestones from the roadmap display, which makes sense as closed milestones are completed and shouldn't appear in a forward-looking roadmap. The alphabetical sorting provides a consistent, predictable order.
232-232: Verify the rationale for changing stats rounding precision.The rounding logic was changed from nearest 5 to nearest 10, reducing display precision. While this may provide cleaner numbers, please ensure this aligns with design requirements and user expectations.
Consider whether this change:
- Improves user experience with simpler numbers
- Maintains sufficient accuracy for the intended purpose
- Aligns with design specifications
frontend/src/app/layout.tsx (1)
1-1: LGTM: Clean migration from GTM to GA.The import change from
GoogleTagManagertoGoogleAnalyticsis correct for the analytics migration.backend/apps/owasp/graphql/queries/sponsor.py (2)
3-3: LGTM: Proper strawberry import.The migration from
graphenetostrawberryis part of the broader GraphQL framework migration.
9-14: LGTM: Clean migration to strawberry decorators.The conversion from
BaseQueryinheritance to@strawberry.typedecorator and@strawberry.fieldmethod decorator follows strawberry best practices. The type annotationlist[SponsorNode]improves type safety while preserving the original sorting logic.backend/apps/owasp/graphql/nodes/event.py (2)
3-3: LGTM: Correct strawberry_django import.The import of
strawberry_djangois appropriate for Django model integration with strawberry GraphQL.
8-23: LGTM: Secure migration to strawberry_django decorators.The migration from
BaseNodeinheritance to@strawberry_django.typedecorator with explicit field listing is excellent. This approach:
- Provides better type safety through decorators
- Requires explicit field exposure, improving security
- Maintains compatibility with the
EventDjango model- Exposes appropriate fields without sensitive information
The explicit field list ensures only intended data is accessible via GraphQL.
backend/settings/graphql.py (2)
3-3: LGTM: Strawberry schema import.The import change from
graphenetostrawberrycompletes the GraphQL framework migration.
9-14: LGTM: Proper strawberry schema definition.The migration to strawberry is correctly implemented:
@strawberry.typedecorator properly defines the Query classstrawberry.Schema(query=Query)correctly instantiates the schema- Multiple inheritance from
GithubQueryandOwaspQueryis maintainedThis completes the GraphQL framework migration at the schema level.
frontend/src/components/CardDetailsPage.tsx (2)
169-174: LGTM! Component reordering looks good.The conditional logic correctly renders the Milestones component for the specified entity types (user, organization, repository, project). The component grouping change aligns with the backend GraphQL schema migration mentioned in the AI summary.
187-190: Good separation of concerns in component grouping.The RecentPullRequests and RecentReleases components are now properly grouped together in a separate conditional block, which makes logical sense from a UI perspective as both relate to repository activity.
backend/apps/owasp/graphql/nodes/sponsor.py (2)
3-4: Correct import for Strawberry Django integration.The import change from BaseNode to strawberry_django aligns with the broader migration strategy and follows Strawberry best practices.
8-17: Excellent migration to Strawberry decorator pattern.The
@strawberry_django.typedecorator approach is cleaner and more explicit than the previous BaseNode inheritance with Meta class. All specified fields (image_url,name,sponsor_type,url) exist in the Sponsor model according to the relevant code snippet, ensuring schema consistency.backend/tests/apps/github/graphql/queries/issue_test.py (1)
14-21: Good use of pytest fixtures.The mock_issue fixture is well-structured and provides a reusable mock object for tests.
backend/apps/owasp/graphql/nodes/stats.py (2)
3-3: Correct import for Strawberry framework.The import statement properly brings in the Strawberry library for the type decorator.
6-14: Excellent migration to Strawberry type annotations.The conversion from
graphene.ObjectTypewithgraphene.Int()fields to@strawberry.typewith Python type annotations is much cleaner and more Pythonic. The field names clearly indicate they represent statistical counts, and theinttype annotation is appropriate for all statistics fields.backend/tests/apps/owasp/graphql/queries/committee_test.py (2)
8-13: LGTM - Well-structured Strawberry migration test.The test correctly verifies that
CommitteeQueryis a valid Strawberry type by checking for the__strawberry_definition__attribute and confirms the presence of the "committee" field. This aligns with the migration from Graphene to Strawberry GraphQL.
15-29: Excellent detailed field configuration verification.The test thoroughly validates the "committee" field configuration by:
- Extracting the specific field from the Strawberry definition
- Verifying the field type is
CommitteeNode- Checking argument presence and types
This comprehensive approach ensures the Strawberry migration maintains the expected GraphQL schema structure.
frontend/src/types/project.ts (2)
37-37: Confirm field reordering is intentional.The
namefield in theauthorobject has been moved belowlogin. While this doesn't affect functionality, please confirm this reordering aligns with the backend GraphQL schema structure.
46-46: Good addition of state field.The new
state: stringfield correctly aligns with the backendMilestoneNodechanges mentioned in the AI summary, supporting the Strawberry GraphQL migration.backend/tests/apps/github/graphql/nodes/organization_test.py (5)
10-11: LGTM - Correct Strawberry migration pattern.The test properly verifies the presence of
__strawberry_definition__attribute, which is the correct approach for validating Strawberry GraphQL types after migration from Graphene.
13-30: Well-structured field configuration test.The test comprehensively validates all expected fields for
OrganizationNode. The field set matches the schema structure and uses proper Strawberry definition access patterns.
32-38: Correct field type verification.The test properly extracts and validates the
statsfield type using Strawberry's field definition structure. The identity comparisonis OrganizationStatsNodeis appropriate for type checking.
40-45: Appropriate URL field validation.The test correctly verifies the
urlfield type asstrusing Strawberry's field definition patterns.
57-60: Good validation of OrganizationStatsNode fields.The test properly validates all expected fields in the
OrganizationStatsNodeusing Strawberry definition patterns, ensuring the stats node structure is correct.backend/tests/apps/github/graphql/queries/repository_test.py (4)
17-19: Good enhancement of mock fixture.The addition of the
keyattribute to the mock repository fixture is appropriate and aligns with the repository model structure.
21-40: Correct migration to instance method calls.The test properly calls
RepositoryQuery().repository()as an instance method instead of a class method, which is the correct pattern for Strawberry GraphQL resolvers. The test logic and assertions remain sound.
42-61: Well-structured error handling test.The test correctly verifies the
Nonereturn value when a repository is not found, maintaining proper error handling behavior in the Strawberry implementation.
63-83: Comprehensive test for repositories list method.The new test properly validates the
repositoriesmethod with appropriate mocking of the queryset chain. The test verifies:
- Return type is a list
- Correct repository content
- Proper method calls with expected parameters
- Correct ordering by stars count
backend/tests/apps/github/graphql/nodes/release_test.py (3)
10-11: LGTM: Correct Strawberry inheritance check.The test has been properly updated to check for Strawberry's
__strawberry_definition__attribute instead of Graphene's base class inheritance.
13-26: LGTM: Proper Strawberry field definition testing.The meta configuration test correctly extracts field names from Strawberry's definition structure and validates against the expected field set.
28-32: LGTM: Correct Strawberry field type assertion.The author field test properly locates the field within Strawberry's definition and validates the type relationship with
UserNode.backend/apps/owasp/graphql/queries/committee.py (2)
9-10: LGTM: Correct Strawberry migration pattern.The class has been properly migrated from Graphene inheritance to Strawberry decorator-based definition.
13-14: LGTM: Proper Strawberry field definition.The field has been correctly migrated to use
@strawberry.fielddecorator with appropriate type annotations.backend/apps/owasp/graphql/queries/post.py (2)
9-10: LGTM: Correct Strawberry migration pattern.The class has been properly migrated to use Strawberry's decorator-based type definition.
13-16: LGTM: Proper field definition with consistent default value.The field migration is correct, and the default limit of 5 aligns with the docstring mentioning "5 most recent posts".
backend/apps/github/graphql/queries/organization.py (3)
9-10: LGTM: Correct Strawberry migration pattern.The class has been properly migrated to use Strawberry's decorator-based type definition.
13-18: LGTM: Proper field definition with keyword-only arguments.The field migration correctly uses
@strawberry.fielddecorator with appropriate type annotations and keyword-only argument syntax.
19-31: LGTM: Accurate documentation and logic.The docstring has been properly updated to reflect the new return type, and the core query logic remains unchanged and correct.
backend/apps/owasp/graphql/queries/event.py (1)
3-16: LGTM! Clean migration from Graphene to Strawberry.The GraphQL query migration is well-implemented:
- Proper use of
@strawberry.typeand@strawberry.fielddecorators- Correct type annotations for parameters and return types
- Method signature updated from static resolver to instance method
- Business logic preserved with the same default limit value
backend/tests/apps/github/graphql/queries/user_test.py (2)
17-35: LGTM! Test methods correctly updated for Strawberry migration.The test updates properly reflect the migration from static resolver methods to instance methods:
- Correctly changed from
UserQuery.resolve_user()toUserQuery().user()- Maintained proper mocking and assertions
- Test coverage preserved
37-56: LGTM! Good test coverage for new resolver method.The new test for
top_contributed_repositoriesis well-implemented:
- Proper mocking of the ORM query chain
- Correct assertions for method calls and return values
- Follows consistent testing patterns
backend/apps/owasp/graphql/queries/stats.py (2)
3-19: LGTM! Clean migration from Graphene to Strawberry.The GraphQL query migration is properly implemented:
- Correct use of
@strawberry.typeand@strawberry.fielddecorators- Proper type annotations with
StatsNodereturn type- Method signature updated from static resolver to instance method
40-40: Verify the intentional change in contributors stats rounding factor.The rounding factor for
contributors_statswas changed from 100 to 1000, which will significantly affect how contributor statistics are displayed to users. For example, 12,547 contributors will now round down to 12,000 instead of 12,500.Please confirm this change is intentional and aligns with the desired user experience for displaying contributor statistics.
backend/apps/owasp/graphql/queries/chapter.py (1)
3-24: LGTM! Excellent migration from Graphene to Strawberry.The GraphQL query migration is well-executed:
- Proper use of
@strawberry.typeand@strawberry.fielddecorators- Accurate type annotations including union types (
ChapterNode | None) and generic types (list[ChapterNode])- Method signatures correctly updated from static resolvers to instance methods
- Business logic preserved including exception handling and default parameters
- Key processing logic maintained (
f"www-chapter-{key}")backend/apps/github/graphql/queries/repository_contributor.py (3)
3-3: LGTM: Correct import for Strawberry migration.The import change from
graphenetostrawberryis appropriate for this GraphQL library migration.
9-10: LGTM: Proper Strawberry type decorator usage.The migration from inheriting
BaseQueryto using the@strawberry.typedecorator follows Strawberry's idiomatic approach for defining GraphQL types.
13-23: LGTM: Excellent Strawberry field implementation.The migration from
graphene.Listfield to@strawberry.fielddecorator with proper Python type hints is well-executed:
- Correct use of
@strawberry.fielddecorator- Proper type annotations for parameters and return type
- Updated method signature removes
rootandinfoparameters as expected in Strawberry- Maintains the same parameter defaults and functionality
backend/tests/apps/github/graphql/queries/milestone_test.py (1)
1-96: LGTM: Comprehensive and well-structured test implementation.This test file demonstrates excellent testing practices:
- Good use of pytest fixtures for reusable mock objects
- Parametrized testing for different state values (open, closed, all)
- Proper mocking of Django ORM methods with appropriate return value chaining
- Comprehensive test coverage including filters, distinct queries, and edge cases
- Clear test method names that describe the specific scenario being tested
The test structure aligns well with the Strawberry GraphQL migration and provides robust coverage for the
MilestoneQueryfunctionality.backend/tests/apps/owasp/graphql/nodes/project_test.py (4)
13-13: LGTM: Correct Strawberry schema introspection.The change from testing Graphene's inheritance to checking for Strawberry's
__strawberry_definition__attribute is the correct approach for validating Strawberry GraphQL types.
15-39: LGTM: Proper field validation using Strawberry definition.The test correctly uses Strawberry's field definition structure to validate field names. The use of
{field.name for field in ProjectNode.__strawberry_definition__.fields}is the appropriate way to introspect Strawberry schema fields.
41-44: LGTM: Clean helper method for field lookup.The
_get_field_by_namehelper method provides a clean and reusable way to retrieve specific fields from the Strawberry definition for individual testing.
46-94: LGTM: Comprehensive field type validation.The individual field tests properly validate both scalar types (
int,str,list[str]) and complex node relationships (e.g.,IssueNode,MilestoneNode). The use offield.type.of_typefor list types is correct for Strawberry's type introspection.backend/apps/github/graphql/queries/user.py (4)
3-3: LGTM: Correct import for Strawberry migration.The import change to
strawberryis appropriate and consistent with the GraphQL library migration.
11-12: LGTM: Proper Strawberry type decorator.The migration from inheriting
BaseQueryto using@strawberry.typedecorator follows Strawberry's recommended approach for GraphQL type definitions.
15-37: LGTM: Well-implemented Strawberry field method.The
top_contributed_repositoriesmethod demonstrates excellent Strawberry migration:
- Correct
@strawberry.fielddecorator usage- Proper type hints for parameters and return type
- Updated method signature removes
rootandinfoparameters- Preserves the original ORM query logic and performance optimizations
39-56: LGTM: Proper nullable return type handling.The
usermethod correctly implements:
@strawberry.fielddecorator- Union type annotation (
UserNode | None) for nullable return- Appropriate exception handling for
User.DoesNotExist- Clean method signature following Strawberry conventions
backend/apps/owasp/graphql/queries/snapshot.py (4)
3-3: LGTM: Clean import addition for Strawberry migration.The strawberry import is correctly added to support the migration from Graphene to Strawberry.
9-10: LGTM: Proper Strawberry decorator usage.The class has been correctly updated to use
@strawberry.typedecorator instead of inheriting fromBaseQuery, which aligns with Strawberry's decorator-based schema definition approach.
13-22: LGTM: Excellent method-based field resolver with preserved logic.The migration from
graphene.Fieldto@strawberry.fielddecorated method is well-executed:
- Proper type annotations using modern Python syntax (
SnapshotNode | None)- Method signature correctly updated from
(root, info, ...)to(self, ...)- Exception handling and query logic perfectly preserved
- Clean and readable implementation
24-31: LGTM: Consistent field resolver implementation.The
snapshotsmethod follows the same excellent migration pattern as thesnapshotmethod, with proper type annotations and preserved business logic.backend/tests/apps/github/graphql/nodes/milestone_test.py (2)
9-10: LGTM: Correct test adaptation for Strawberry migration.The test has been properly updated to check for Strawberry's
__strawberry_definition__attribute instead of the previous Graphene-based inheritance check.
12-27: LGTM: Proper field verification with Strawberry introspection.The test correctly uses Strawberry's field definition structure to verify the expected fields. The addition of the
"state"field in the expected set suggests this field was properly added during the migration, which aligns with the relevant code snippets showing the MilestoneNode implementation.backend/apps/github/graphql/queries/issue.py (4)
3-4: LGTM: Appropriate imports for Strawberry migration.The strawberry import and Django ORM imports are correctly added to support the migration and the existing query functionality.
10-11: LGTM: Consistent Strawberry decorator pattern.The class correctly uses
@strawberry.typedecorator instead of inheriting fromBaseQuery, maintaining consistency with the migration pattern across the codebase.
14-33: LGTM: Excellent method signature and type annotation migration.The method has been expertly migrated:
- Proper
@strawberry.fielddecorator usage- Correct method signature change from
(root, info, ...)to(self, ...)- Accurate type annotations using
list[IssueNode]- Well-maintained docstring with updated parameter descriptions
- Keyword-only arguments preserved with proper defaults
35-65: LGTM: Complex query logic perfectly preserved.The migration excellently preserves all the sophisticated query functionality:
- Select related and prefetch optimizations maintained
- Conditional filtering by login and organization preserved
- Complex distinct filtering with subqueries intact
- Proper ordering and limiting logic maintained
This demonstrates careful attention to preserving business logic during the framework migration.
backend/apps/github/graphql/queries/milestone.py (4)
3-3: LGTM: Proper strawberry import addition.The strawberry import is correctly added to support the migration to Strawberry GraphQL framework.
11-12: LGTM: Consistent decorator pattern maintained.The class correctly uses
@strawberry.typedecorator, maintaining consistency with the broader Strawberry migration across the codebase.
15-36: LGTM: Excellent method signature and documentation migration.The method migration is exemplary:
- Proper
@strawberry.fielddecorator usage- Correct signature change from
(root, info, ...)to(self, ...)- Accurate type annotations with
list[MilestoneNode]- Well-updated docstring removing GraphQL-specific parameters
- Keyword-only arguments with appropriate defaults preserved
38-80: LGTM: Sophisticated query logic flawlessly preserved.The migration demonstrates exceptional care in preserving complex business logic:
- State validation with pattern matching maintained
- Custom manager usage (
open_milestones,closed_milestones) preserved- Query optimizations (select_related, prefetch_related) intact
- Conditional filtering by login and organization maintained
- Complex distinct filtering with subqueries perfectly preserved
- Proper error handling with ValidationError maintained
This showcases excellent migration practices that prioritize business logic preservation.
backend/apps/github/graphql/nodes/organization.py (4)
54-56: Good defensive programming with safe dictionary access.The use of
.get()with fallback to 0 prevents potential KeyError exceptions if the aggregation returns None values. This is an improvement over direct dictionary access.
12-21: LGTM! Clean migration to Strawberry type system.The OrganizationStatsNode has been correctly migrated from graphene.ObjectType to @strawberry.type with proper type annotations for all fields.
23-38: LGTM! Proper Strawberry Django integration.The OrganizationNode has been correctly migrated to use @strawberry_django.type with explicit field selection. This follows Strawberry best practices for Django model integration.
73-76:✅ Verification successful
Verify that the Organization model has a
urlproperty.The method
urlreturnsself.url, but I don't see aurlproperty defined in the Organization model from the provided context. This could lead to infinite recursion or an AttributeError.Please verify that the Organization model has a
urlproperty or method:
🏁 Script executed:
#!/bin/bash # Description: Check if Organization model has a url property/method # Expected: Find url property definition in the model or its mixins # Search for url property/method in Organization model and its parent classes ast-grep --pattern $'class Organization($$$): $$$ url = $$$ $$$' # Also search for url property in mixins that Organization inherits from rg -A 3 -B 1 "class.*Model.*:" backend/apps/github/models/organization.py rg -A 5 -B 1 "def url" backend/apps/github/models/ rg -A 5 -B 1 "@property" backend/apps/github/models/ | rg -A 5 -B 1 "url"Length of output: 6857
🏁 Script executed:
#!/bin/bash # Inspect the Organization model for a `url` field or property sed -n '1,200p' backend/apps/github/models/organization.pyLength of output: 2821
🏁 Script executed:
#!/bin/bash # Inspect GenericUserModel for url property sed -n '1,200p' backend/apps/github/models/common.pyLength of output: 3007
🏁 Script executed:
#!/bin/bash # Inspect GraphQL node definition for Organization sed -n '1,200p' backend/apps/github/graphql/nodes/organization.pyLength of output: 2147
🏁 Script executed:
#!/bin/bash # Inspect GraphQL node definition for Repository sed -n '1,200p' backend/apps/github/graphql/nodes/repository.pyLength of output: 2643
No changes needed:
urlis inherited fromGenericUserModelThe
Organizationmodel inherits fromGenericUserModelinapps/github/models/common.py, which defines:@property def url(self) -> str: """Entity URL.""" return f"https://github.com/{self.login.lower()}"So calling
self.urlin the GraphQL resolver correctly invokes that property.backend/apps/owasp/graphql/nodes/snapshot.py (2)
17-25: LGTM! Clean migration to Strawberry Django.The decorator correctly specifies the Django model and explicit field selection, following Strawberry Django best practices.
29-57: LGTM! Proper field method implementation.All field methods are correctly decorated with @strawberry.field and have proper type annotations. The query logic and ordering are preserved from the original implementation.
backend/apps/github/graphql/queries/release.py (3)
10-11: LGTM! Correct migration to Strawberry type system.The class has been properly migrated from inheriting BaseQuery to using the @strawberry.type decorator.
14-22: LGTM! Proper method signature with type annotations.The method signature correctly uses keyword-only arguments with proper type annotations including the Python union syntax for optional types (
str | None). The return type annotation is accurate for Strawberry.
34-67: LGTM! Query logic preserved correctly.The filtering logic for releases, including the distinct functionality with subqueries, has been preserved correctly during the migration. The return of a sliced queryset is appropriate.
backend/apps/owasp/graphql/queries/project.py (3)
9-11: LGTM! Clean migration to Strawberry.The ProjectQuery class has been correctly migrated from inheriting BaseQuery to using the @strawberry.type decorator.
13-27: LGTM! Proper exception handling and type annotations.The project resolver method correctly handles the DoesNotExist exception and uses proper Python union type annotation for the optional return.
29-40: LGTM! Query logic preserved with correct type annotation.The recent_projects method maintains the original filtering and ordering logic while using proper list type annotation for Strawberry.
backend/apps/github/graphql/queries/pull_request.py (4)
3-4: LGTM: Proper import statements for Strawberry migration.The imports are correctly updated to include
strawberryand the necessary Django ORM imports are preserved.
11-12: LGTM: Correct Strawberry type decorator usage.The class is properly decorated with
@strawberry.type, which is the correct pattern for defining a GraphQL type in Strawberry.
15-25: LGTM: Proper field decorator and method signature.The method is correctly decorated with
@strawberry.fieldand the signature properly includesselfas the first parameter (instance method pattern). The return type annotationlist[PullRequestNode]is accurate and follows Python typing conventions.
37-38: LGTM: Updated return type documentation.The docstring return type is correctly updated to reflect the new type annotation format.
backend/apps/owasp/graphql/nodes/committee.py (3)
3-4: LGTM: Correct imports for Strawberry Django integration.The imports properly include both
strawberryandstrawberry_djangowhich are needed for Django model integration.
10-11: LGTM: Proper Strawberry Django type decorator.The
@strawberry_django.type(Committee, fields=["name", "summary"])decorator correctly specifies the Django model and explicitly lists the fields to be exposed, which is a good practice for controlling the GraphQL schema.
14-42: LGTM: Well-structured field resolvers with proper type annotations.All resolver methods are correctly decorated with
@strawberry.fieldand have appropriate return type annotations (int,float). The method signatures are clean and the logic is preserved from the original implementation.backend/tests/apps/owasp/graphql/queries/project_test.py (3)
13-18: LGTM: Proper Strawberry definition verification.The test correctly checks for the
__strawberry_definition__attribute and verifies the presence of expected fields, which is the appropriate way to test Strawberry type definitions.
22-34: LGTM: Thorough field configuration testing.The test properly inspects the Strawberry field definition, checking the field type and argument configuration. This ensures the GraphQL schema is correctly defined.
53-54:✅ Verification successful
Verify the resolver invocation pattern.
The resolver is being called using
query.__class__.__dict__["project"](query, key="test-project"). This seems unusual for Strawberry - typically you would callquery.project(key="test-project")directly.Please verify if this invocation pattern is necessary for Strawberry resolvers:
🏁 Script executed:
#!/bin/bash # Check how other Strawberry resolver tests are implemented in the codebase rg -A 5 -B 5 "__class__.__dict__" --type pyLength of output: 2083
🏁 Script executed:
#!/bin/bash # Locate the definition of ProjectQuery and its methods rg -A50 "class ProjectQuery" --type py # Show usage of strawberry decorators in the same file rg -n "@strawberry\.field" -A5 --type pyLength of output: 37902
Bypass Strawberry wrapper in unit test
Calling
query.project(key=…)would invoke Strawberry’s field wrapper (which expects GraphQL context), so the test intentionally pulls the raw resolver function out of the class dict and passes the instance asself. This is the correct way to unit-test the plain resolver logic. No change required.backend/tests/apps/owasp/graphql/queries/chapter_test.py (3)
12-18: LGTM: Proper Strawberry definition verification.The test correctly verifies the
__strawberry_definition__attribute and checks for both expected fields (chapterandrecent_chapters).
49-49: Consistent with improved pattern.This follows the same direct method call pattern, which is good for consistency within this test file.
54-67: LGTM: Comprehensive test for recent_chapters resolver.The test properly mocks the queryset chain, verifies the filtering and ordering logic, and checks the limit parameter. The assertions comprehensively cover the expected behavior.
backend/apps/github/graphql/queries/repository.py (3)
3-10: Migration to Strawberry looks good!The class decoration with
@strawberry.typeand the removal of base class inheritance correctly follows Strawberry's declarative pattern.
13-18: Proper Strawberry field implementation.The method signature with
selfparameter and union return type annotationRepositoryNode | Nonecorrectly follows Strawberry conventions.
37-43: Clean implementation with proper type annotations.The
repositoriesfield correctly useslist[RepositoryNode]return type and maintains the original query logic.backend/apps/owasp/graphql/nodes/chapter.py (3)
10-16: Well-defined geographic location type.The
GeoLocationTypeis correctly implemented with type-annotated fields using Strawberry's declarative style.
18-30: Proper strawberry_django integration.The
@strawberry_django.typedecorator with explicitfieldslist correctly exposes the Chapter model fields while maintaining control over the GraphQL schema.
40-46: Excellent null safety implementation.The
geo_locationresolver properly handles cases where latitude or longitude might be None, preventing potential GraphQL errors.backend/apps/github/graphql/nodes/user.py (3)
9-25: Clean strawberry_django type definition.The explicit
fieldslist in the decorator provides good control over which model fields are exposed in the GraphQL schema.
29-47: Consistent resolver implementations.The resolver methods correctly use the
idx_*properties from the model mixins for computed values like timestamps and counts.
49-52:❓ Verification inconclusive
Potential infinite recursion in url resolver.
The
urlmethod returnsself.url, which could cause infinite recursion if there's aurlproperty on the model that conflicts with this method name.
🏁 Script executed:
#!/bin/bash # Check if the User model has a url property that might conflict ast-grep --pattern 'class User($_) { $$$ def url($_) { $$$ } $$$ }' # Also check for url property definitions rg -A 3 -B 1 "url.*=" backend/apps/github/models/user.pyLength of output: 140
🏁 Script executed:
#!/bin/bash # Locate the User model file(s) under the GitHub app fd -t f -e py user.py backend/apps/github # Show the User class definition with surrounding lines rg -A5 -B2 "class User" backend/apps/github/models/user.py # Search for any url attribute or @property in that file rg -n "url" backend/apps/github/models/user.pyLength of output: 607
🏁 Script executed:
#!/bin/bash # Display the beginning of the GraphQL User node to inspect annotations and fields sed -n '1,100p' backend/apps/github/graphql/nodes/user.py # Display the full url resolver and its surrounding context sed -n '40,80p' backend/apps/github/graphql/nodes/user.pyLength of output: 1561
#!/bin/bash # Show mixin file content to locate URL field definitions sed -n '1,200p' backend/apps/github/models/mixins/user.py # Search specifically for url in the mixin rg -n "url" backend/apps/github/models/mixins/user.py
#!/bin/bash # Locate the mixin file to inspect URL field definitions fd -t f -e py mixins/user.py backend/apps/github/models # Display the content of the UserIndexMixin file sed -n '1,200p' backend/apps/github/models/mixins/user.py # Search for any url attribute or property in the mixin rg -n "url" backend/apps/github/models/mixins/user.py
#!/bin/bash # Inspect the main User model for any url attributes sed -n '1,200p' backend/apps/github/models/user.py rg -n "url" backend/apps/github/models/user.py # Inspect the UserIndexMixin for url definitions sed -n '1,200p' backend/apps/github/models/mixins/user.py rg -n "url" backend/apps/github/models/mixins/user.py
Verify
urlresolver to prevent potential recursion
I didn’t find aurlfield or property on theUsermodel, but the resolver is defined as:@strawberry.field def url(self) -> str: """Resolve URL.""" return self.urlIf
selfhere doesn’t refer to a model instance with aurlattribute,self.urlwill re-invoke this method, causing infinite recursion. Please confirm:
- Does
backend/apps/github/models/user.py(or any mixin) define aurlfield/property?- In Strawberry-Django, does
selfwithin this resolver map to the DjangoUserinstance (soself.urlis the model’s field)?If not, update the resolver to reference the underlying model (for example by accepting a
root/infoargument and usingroot.urlorself._root.url), or rename one of them to avoid the name collision.backend/apps/github/graphql/nodes/issue.py (3)
10-18: Proper strawberry_django type configuration.The decorator correctly specifies the Issue model and exposes the appropriate fields for the GraphQL schema.
22-25: Simple and correct author resolver.The author field correctly returns
UserNode | Nonematching the nullable relationship.
27-39: Excellent null safety in relationship resolvers.Both
organization_nameandrepository_nameresolvers properly handle the case where related objects might be None, preventing potential GraphQL runtime errors.backend/apps/owasp/graphql/nodes/project.py (1)
3-33: Migration to Strawberry looks good!The decorator-based approach with explicit field listing follows Strawberry best practices and provides better type safety.
backend/apps/github/graphql/nodes/pull_request.py (1)
3-43: Well-implemented Strawberry migration with proper null safety!The resolver methods correctly handle potential null values in relationships and provide appropriate fallbacks. The use of modern Python union type syntax (
|) is clean and readable.backend/apps/github/graphql/nodes/milestone.py (1)
3-51: Excellent migration with robust error handling!The progress calculation properly handles division by zero, and all resolver methods include appropriate null checks for related objects. The implementation is clean and follows Strawberry best practices.
backend/apps/owasp/graphql/nodes/common.py (1)
3-9: LGTM!The migration from graphene to strawberry is correctly implemented with appropriate decorator usage.
backend/tests/apps/github/graphql/nodes/repository_test.py (7)
11-13: LGTM! Correct migration to Strawberry framework.The class name change and the use of
__strawberry_definition__correctly reflects the migration from Graphene to Strawberry.
41-44: LGTM! Clean helper method implementation.The helper method provides a clean way to retrieve fields by name from the Strawberry definition. The implementation is correct and reusable.
86-94: LGTM! Consistent test pattern.The remaining field tests follow the same pattern established earlier. The logic is consistent with the helper method usage and type checking approach.
46-60:❓ Verification inconclusive
Verify type checking approach for Strawberry fields.
The type checking logic assumes direct type comparison (
field.type is int,field.type == list[str]). Verify this approach works correctly with Strawberry's type system, as it may wrap types differently than expected.
🏁 Script executed:
#!/bin/bash # Description: Verify Strawberry field type representation # Expected: Understand how Strawberry represents field types echo "Checking how Strawberry represents field types..." python3 -c " import sys sys.path.append('backend') import django import os os.environ.setdefault('DJANGO_SETTINGS_MODULE', 'config.settings.test') django.setup() from apps.owasp.graphql.nodes.project import ProjectNode # Check a few field types definition = ProjectNode.__strawberry_definition__ for field in definition.fields[:5]: print(f'Field: {field.name}, Type: {field.type}, Type repr: {repr(field.type)}') "Length of output: 675
Verify Strawberry field type representation
The current tests use direct comparisons (
field.type is int,field.type == list[str]), but Strawberry may wrap Python generics into its own types (e.g.StrawberryList[str],StrawberryOptional[str]). Please confirm the actual values returned byfield.typeby introspecting your node definitions in a Django shell or quick debug test:from apps.owasp.graphql.nodes.project import ProjectNode for f in ProjectNode.__strawberry_definition__.fields: print(f"{f.name!r}: {f.type!r}")Once you know the real representations, adjust your assertions accordingly—either compare against the Strawberry wrappers or inspect underlying types (e.g.
f.type.of_typefor lists).
61-85:✅ Verification successful
Verify relationship field type checking for list types.
The tests assume that relationship fields (like
recent_issues) have a.of_typeattribute to access the inner type. This pattern may not be correct for all Strawberry list type representations.
🏁 Script executed:
#!/bin/bash # Description: Verify how Strawberry represents list/relationship field types # Expected: Understand the correct way to check list element types in Strawberry echo "Checking Strawberry list field type structure..." python3 -c " import sys sys.path.append('backend') import django import os os.environ.setdefault('DJANGO_SETTINGS_MODULE', 'config.settings.test') django.setup() from apps.owasp.graphql.nodes.project import ProjectNode # Check relationship field types definition = ProjectNode.__strawberry_definition__ relationship_fields = ['recent_issues', 'recent_milestones', 'recent_pull_requests', 'repositories'] for field_name in relationship_fields: field = next((f for f in definition.fields if f.name == field_name), None) if field: print(f'Field: {field_name}') print(f' Type: {field.type}') print(f' Has of_type: {hasattr(field.type, \"of_type\")}') if hasattr(field.type, 'of_type'): print(f' of_type: {field.type.of_type}') print() "Length of output: 1097
Let’s verify Strawberry’s list‐type internals directly (without Django) by installing Strawberry and introspecting a simple field:
This will confirm whether
field.type.of_typeis the correct way to access the inner element type.
🏁 Script executed:
#!/bin/bash # Install Strawberry and inspect a List[int] field’s type object pip install strawberry-graphql --quiet python3 - << 'EOF' from typing import List import strawberry @strawberry.type class Dummy: x: List[int] definition = Dummy.__strawberry_definition__ field = next(f for f in definition.fields if f.name == "x") print("Field:", field.name) print("Type object:", field.type) print("Attributes on type object:", [a for a in dir(field.type) if not a.startswith('_')]) print("Has of_type?", hasattr(field.type, "of_type")) if hasattr(field.type, "of_type"): print("of_type:", field.type.of_type) EOFLength of output: 511
Verified: StrawberryList exposes
.of_typefor list element typesConfirmed with a standalone example that
strawberry.types.base.StrawberryListincludes anof_typeattribute returning the inner type. The existing assertions inbackend/tests/apps/github/graphql/nodes/repository_test.pyare correct and require no changes.
4-8:✅ Verification successful
Verify import consistency with the Strawberry migration.
The imports look correct for the Strawberry framework migration. However, ensure that all imported nodes (MilestoneNode, PullRequestNode, ProjectNode) are properly migrated to Strawberry as well.
🏁 Script executed:
#!/bin/bash # Description: Verify that imported nodes are properly migrated to Strawberry # Expected: All nodes should have strawberry decorators and no graphene imports echo "Checking for Strawberry decorators in imported nodes..." for node in "milestone" "pull_request" "project"; do echo "=== Checking ${node} node ===" fd "${node}.py" backend/apps --exec rg -A 3 -B 3 "@strawberry" done echo "Checking for any remaining Graphene imports..." for node in "milestone" "pull_request" "project"; do echo "=== Checking ${node} for graphene imports ===" fd "${node}.py" backend/apps --exec rg "graphene" doneLength of output: 5825
Import Consistency with Strawberry Verified
All imported nodes—
MilestoneNode,PullRequestNode, andProjectNode—are fully migrated to Strawberry (they include@strawberry.type/@strawberry.fielddecorators) and contain no remaining Graphene imports. No further action is required.
16-39:✅ Verification successful
Verify field enumeration approach and expected fields.
The field enumeration logic using
ProjectNode.__strawberry_definition__.fieldsis correct for Strawberry. However, verify that all expected fields are actually defined in the ProjectNode implementation.
🏁 Script executed:
#!/bin/bash # Description: Verify that expected fields are actually defined in ProjectNode # Expected: All fields in expected_field_names should exist in the actual implementation echo "Checking ProjectNode field definitions..." ast-grep --pattern $'@strawberry_django.type class ProjectNode($$$): $$$' echo "Checking for field resolvers in ProjectNode..." rg -A 10 "class ProjectNode" backend/apps/owasp/graphql/nodes/project.pyLength of output: 618
🏁 Script executed:
#!/bin/bash # Inspect the decorator and field configuration for ProjectNode echo "=== Decorator lines for ProjectNode ===" rg -n "@strawberry_django.type" backend/apps/owasp/graphql/nodes/project.py echo echo "=== Context around class definition ===" rg -n -C3 "class ProjectNode" backend/apps/owasp/graphql/nodes/project.pyLength of output: 496
🏁 Script executed:
#!/bin/bash # Retrieve the full @strawberry_django.type decorator invocation for ProjectNode rg -n -A30 "@strawberry_django.type" backend/apps/owasp/graphql/nodes/project.pyLength of output: 840
🏁 Script executed:
#!/bin/bash # List all strawberry.field resolvers in ProjectNode to verify expected computed fields rg -n -A1 "@strawberry.field" backend/apps/owasp/graphql/nodes/project.pyLength of output: 840
All expected ProjectNode fields are defined
Verified that the ten static fields (
contributors_count,created_at,forks_count,is_active,level,name,open_issues_count,stars_count,summary,type) are declared in the@strawberry_django.type(fields=[…])decorator (lines 19–32), and the ten computed fields (issues_count,key,languages,recent_issues,recent_milestones,recent_pull_requests,recent_releases,repositories,repositories_count,topics) are exposed via@strawberry.fieldresolvers in the class (lines 37–85). All names in the test’sexpected_field_namesset are accounted for.
| strawberry-graphql = {extras = ["django"], version = "^0.270.1"} | ||
| thefuzz = "^0.22.1" | ||
| strawberry-graphql-django = "^0.59.1" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify dependency versions for security and compatibility.
The Strawberry GraphQL dependencies are correctly added for the migration. Please ensure these versions are current and secure.
🌐 Web query:
What are the latest stable versions of strawberry-graphql and strawberry-graphql-django? Are there any known security vulnerabilities in versions 0.270.1 and 0.59.1 respectively?
💡 Result:
The latest stable version of strawberry-graphql is 0.270.2, which was released on May 24, 2025[1][2]. This release fixed an issue with subscriptions getting stuck when using the legacy graphql-ws WebSocket subprotocol with a non-existing operationName[2].
For strawberry-graphql-django, the search results don't explicitly mention the latest version number. While there's information about the package and its features[6], the specific version information is not provided in the search results.
Security Vulnerabilities
Regarding the specific versions you asked about (0.270.1 and 0.59.1), there are no explicitly mentioned security vulnerabilities for these exact versions in the search results. However, there are two notable security vulnerabilities that have been documented for strawberry-graphql:
-
CVE-2025-22151 (Type Resolution Vulnerability):
- This vulnerability affects the relay integration in Strawberry GraphQL when multiple GraphQL types are mapped to the same underlying model while using the relay node interface[7].
- It impacts multiple ORM integrations including Django, SQLAlchemy, and Pydantic[7].
- The fix was included in strawberry-graphql version 0.257.0 and strawberry-graphql-django version 0.54.0[7].
- Since the versions you asked about (0.270.1 and 0.59.1) are higher than these fixed versions, they should not be affected by this vulnerability.
-
CVE-2024-47082 (CSRF Vulnerability):
- This is a Cross-Site Request Forgery vulnerability that was fixed in version 0.243.0[8].
- The vulnerability arose from the default enabling of multipart file upload support in all Strawberry HTTP view integrations, exposing them to CSRF attacks if users did not enable CSRF protection[8].
- Again, since the versions you asked about (0.270.1 and 0.59.1) are higher than the fixed version, they should not be affected by this vulnerability.
It's worth noting that the search results don't contain information about any vulnerabilities specific to versions 0.270.1 or 0.59.1, suggesting that there are no known major security issues with these versions based on the provided information.
Citations:
- 1: https://pypi.org/project/strawberry-graphql/
- 2: https://github.com/strawberry-graphql/strawberry/releases
- 3: https://strawberry.rocks/docs/breaking-changes
- 4: https://strawberry.rocks
- 5: https://strawberry.rocks/release/0.262.5
- 6: https://github.com/strawberry-graphql/strawberry-django
- 7: https://advisories.gitlab.com/pkg/pypi/strawberry-graphql/CVE-2025-22151/
- 8: https://vulert.com/vuln-db/CVE-2024-47082
- 9: https://chillicream.com/docs/strawberryshake/v13/
- 10: https://djangopackages.org/packages/p/strawberry/
Update Strawberry GraphQL to latest patch and confirm Django integration version
The added dependencies are not affected by the known CVEs (CVE-2025-22151 and CVE-2024-47082 were fixed in 0.257.0/0.54.0 and 0.243.0 respectively), but there’s a newer patch release for Strawberry GraphQL that includes a critical subscription fix.
Please update and verify:
• File: backend/pyproject.toml (lines 51–53)
- strawberry-graphql: bump from
^0.270.1to^0.270.2(released May 24, 2025) - strawberry-graphql-django: currently
^0.59.1—confirm if a newer patch exists on PyPI and upgrade if available
Suggested diff:
- strawberry-graphql = {extras = ["django"], version = "^0.270.1"}
+ strawberry-graphql = {extras = ["django"], version = "^0.270.2"}📝 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.
| strawberry-graphql = {extras = ["django"], version = "^0.270.1"} | |
| thefuzz = "^0.22.1" | |
| strawberry-graphql-django = "^0.59.1" | |
| # backend/pyproject.toml (lines 51–53) | |
| strawberry-graphql = {extras = ["django"], version = "^0.270.2"} | |
| thefuzz = "^0.22.1" | |
| strawberry-graphql-django = "^0.59.1" |
🤖 Prompt for AI Agents
In backend/pyproject.toml around lines 51 to 53, update the strawberry-graphql
dependency version from ^0.270.1 to ^0.270.2 to include the latest patch with a
critical subscription fix. Also, check PyPI for any newer patch versions of
strawberry-graphql-django beyond ^0.59.1 and update it accordingly if a newer
stable version exists. This ensures the dependencies are up-to-date and secure.
| <ScrollToTop /> | ||
| </Providers> | ||
| </body> | ||
| <GoogleAnalytics gaId={GTM_ID} /> |
There was a problem hiding this comment.
Fix GoogleAnalytics component placement.
The GoogleAnalytics component is placed after the closing </body> tag but before </html>, which violates HTML standards. Components should be placed inside either <head> or <body> tags.
Move the component inside the <body> tag:
<Providers>
<Header />
<BreadCrumbs />
{children}
<Footer />
<ScrollToTop />
+ <GoogleAnalytics gaId={GTM_ID} />
</Providers>
</body>
- <GoogleAnalytics gaId={GTM_ID} />
</html>📝 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.
| <GoogleAnalytics gaId={GTM_ID} /> | |
| <Providers> | |
| <Header /> | |
| <BreadCrumbs /> | |
| {children} | |
| <Footer /> | |
| <ScrollToTop /> | |
| <GoogleAnalytics gaId={GTM_ID} /> | |
| </Providers> | |
| </body> | |
| </html> |
🤖 Prompt for AI Agents
In frontend/src/app/layout.tsx at line 77, the GoogleAnalytics component is
incorrectly placed outside the closing </body> tag but before </html>, violating
HTML standards. Move the <GoogleAnalytics gaId={GTM_ID} /> component so that it
is inside the <body> tag, ensuring it is rendered within the valid HTML
structure.
| def test_recent_issues_limit(self, mock_select_related, mock_issue): | ||
| """Test limiting the number of issues returned.""" | ||
| mock_queryset = MagicMock() | ||
| mock_queryset.order_by.return_value.__getitem__.return_value = [mock_issue] | ||
| mock_select_related.return_value = mock_queryset | ||
|
|
||
| result = IssueQuery().recent_issues(limit=1) | ||
|
|
||
| mock_select_related.assert_called_once() | ||
| mock_queryset.order_by.assert_called_once_with("-created_at") | ||
| assert result == [mock_issue] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential issue with limit testing approach.
The use of __getitem__ to simulate queryset slicing may not accurately represent Django's queryset behavior.
Consider a more accurate approach for testing limiting:
@patch("apps.github.models.issue.Issue.objects.select_related")
def test_recent_issues_limit(self, mock_select_related, mock_issue):
"""Test limiting the number of issues returned."""
mock_queryset = MagicMock()
- mock_queryset.order_by.return_value.__getitem__.return_value = [mock_issue]
+ limited_queryset = MagicMock()
+ limited_queryset.__iter__.return_value = iter([mock_issue])
+ mock_queryset.order_by.return_value.__getitem__.return_value = limited_queryset
mock_select_related.return_value = mock_queryset
result = IssueQuery().recent_issues(limit=1)
mock_select_related.assert_called_once()
mock_queryset.order_by.assert_called_once_with("-created_at")
+ mock_queryset.order_by.return_value.__getitem__.assert_called_once_with(slice(None, 1))
assert result == [mock_issue]📝 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.
| def test_recent_issues_limit(self, mock_select_related, mock_issue): | |
| """Test limiting the number of issues returned.""" | |
| mock_queryset = MagicMock() | |
| mock_queryset.order_by.return_value.__getitem__.return_value = [mock_issue] | |
| mock_select_related.return_value = mock_queryset | |
| result = IssueQuery().recent_issues(limit=1) | |
| mock_select_related.assert_called_once() | |
| mock_queryset.order_by.assert_called_once_with("-created_at") | |
| assert result == [mock_issue] | |
| def test_recent_issues_limit(self, mock_select_related, mock_issue): | |
| """Test limiting the number of issues returned.""" | |
| mock_queryset = MagicMock() | |
| - mock_queryset.order_by.return_value.__getitem__.return_value = [mock_issue] | |
| + limited_queryset = MagicMock() | |
| + # Make the sliced queryset iterable so that list(limited_queryset) == [mock_issue] | |
| + limited_queryset.__iter__.return_value = iter([mock_issue]) | |
| + mock_queryset.order_by.return_value.__getitem__.return_value = limited_queryset | |
| mock_select_related.return_value = mock_queryset | |
| result = IssueQuery().recent_issues(limit=1) | |
| mock_select_related.assert_called_once() | |
| mock_queryset.order_by.assert_called_once_with("-created_at") | |
| + # Verify we sliced with slice(None, 1) | |
| + mock_queryset.order_by.return_value.__getitem__.assert_called_once_with(slice(None, 1)) | |
| assert result == [mock_issue] |
🤖 Prompt for AI Agents
In backend/tests/apps/github/graphql/queries/issue_test.py around lines 67 to
77, the test uses __getitem__ to mock queryset slicing, which may not accurately
simulate Django queryset behavior. To fix this, replace the __getitem__ mock
with a proper mock of the queryset's slice method or chain the queryset methods
to simulate slicing more realistically, ensuring the limit parameter is
correctly applied and tested.
| @strawberry.field | ||
| def recent_milestones(self, limit: int = 5) -> list[MilestoneNode]: | ||
| """Resolve recent milestones.""" | ||
| return self.recent_milestones.select_related("author").order_by("-created_at")[:limit] |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Critical: Infinite recursion in recent_milestones resolver
The method name recent_milestones conflicts with the attribute being accessed on line 60, causing infinite recursion.
The resolver should access a different attribute. Based on the pattern in other resolvers, apply this fix:
@strawberry.field
def recent_milestones(self, limit: int = 5) -> list[MilestoneNode]:
"""Resolve recent milestones."""
- return self.recent_milestones.select_related("author").order_by("-created_at")[:limit]
+ return self.milestones.select_related("author").order_by("-created_at")[:limit]Let me verify the correct attribute name for milestones:
🏁 Script executed:
#!/bin/bash
# Description: Find the correct milestone-related attribute on the Project model
# Search for milestone relationships in the Project model
fd -e py | xargs rg -A 5 -B 5 "class Project" | rg -A 10 -B 10 "milestone"
# Also check for related_name definitions
ast-grep --pattern 'models.ForeignKey($$$, related_name=$_, $$$)'
ast-grep --pattern 'models.ManyToManyField($$$, related_name=$_, $$$)'Length of output: 37144
Fix infinite recursion and reference the correct relation in recent_milestones
The resolver as written calls itself and there is no recent_milestones relation on the Project model—this will blow the stack and won’t return any milestones. The Project model defines an owasp_repository FK to GitHub’s Repository, and GitHub’s Milestone model uses related_name="milestones" on its repository field. Update the resolver to:
• Guard against a missing repository
• Use self.owasp_repository.milestones instead of self.recent_milestones
File: backend/apps/owasp/graphql/nodes/project.py
Lines: 57–60
@strawberry.field
def recent_milestones(self, limit: int = 5) -> list[MilestoneNode]:
"""Resolve recent milestones."""
- return self.recent_milestones.select_related("author").order_by("-created_at")[:limit]
+ # If this Project isn’t linked to a GitHub repo, return an empty list
+ if not self.owasp_repository:
+ return []
+ return (
+ self.owasp_repository
+ .milestones
+ .select_related("author")
+ .order_by("-created_at")[:limit]
+ )📝 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.
| @strawberry.field | |
| def recent_milestones(self, limit: int = 5) -> list[MilestoneNode]: | |
| """Resolve recent milestones.""" | |
| return self.recent_milestones.select_related("author").order_by("-created_at")[:limit] | |
| @strawberry.field | |
| def recent_milestones(self, limit: int = 5) -> list[MilestoneNode]: | |
| """Resolve recent milestones.""" | |
| # If this Project isn’t linked to a GitHub repo, return an empty list | |
| if not self.owasp_repository: | |
| return [] | |
| return ( | |
| self.owasp_repository | |
| .milestones | |
| .select_related("author") | |
| .order_by("-created_at")[:limit] | |
| ) |
🤖 Prompt for AI Agents
In backend/apps/owasp/graphql/nodes/project.py around lines 57 to 60, the
recent_milestones resolver calls itself recursively causing infinite recursion.
To fix this, update the resolver to first check if self.owasp_repository exists,
then access milestones via
self.owasp_repository.milestones.select_related("author").order_by("-created_at")[:limit]
instead of self.recent_milestones. This avoids recursion and correctly
references the related milestones through the repository.
| @strawberry.field | ||
| def repository_name(self) -> str: | ||
| """Resolve repository name.""" | ||
| return self.repository.name if self.repository else None | ||
|
|
There was a problem hiding this comment.
Type annotation mismatch
The return type is declared as str but the method can return None.
Update the return type annotation:
- def repository_name(self) -> str:
+ def repository_name(self) -> str | None:
"""Resolve repository name."""
return self.repository.name if self.repository else 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.
| @strawberry.field | |
| def repository_name(self) -> str: | |
| """Resolve repository name.""" | |
| return self.repository.name if self.repository else None | |
| @strawberry.field | |
| def repository_name(self) -> str | None: | |
| """Resolve repository name.""" | |
| return self.repository.name if self.repository else None |
🤖 Prompt for AI Agents
In backend/apps/github/graphql/nodes/release.py around lines 46 to 50, the
method repository_name is annotated to return a str but it can return None if
self.repository is None. Change the return type annotation from str to
Optional[str] to accurately reflect that the method may return None.
| @strawberry.field | ||
| def author(self) -> UserNode | None: | ||
| """Resolve author.""" | ||
| return self.author | ||
|
|
There was a problem hiding this comment.
Critical: Circular reference in author resolver
The resolver returns self.author which will cause an infinite recursion when the field is accessed.
Based on the pattern from other nodes, this should access the underlying model field:
def author(self) -> UserNode | None:
"""Resolve author."""
- return self.author
+ return self.author # This refers to the model field, not the resolver methodNote: If this is causing issues, consider renaming the resolver method to avoid name collision with the model field.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/apps/github/graphql/nodes/release.py around lines 23 to 27, the
author resolver returns self.author causing infinite recursion. To fix this,
access the underlying model's author attribute instead of calling the resolver
recursively. If the model field is also named author, rename the resolver method
to avoid name collision and return the model's author field accordingly.
| @strawberry.field | ||
| def project_name(self) -> str: | ||
| """Resolve project name.""" | ||
| return ( | ||
| self.repository.project.name.lstrip(OWASP_ORGANIZATION_NAME) | ||
| if self.repository and self.repository.project | ||
| else None | ||
| ) |
There was a problem hiding this comment.
Type annotation mismatch
The return type is declared as str but the method can return None.
Update the return type annotation:
- def project_name(self) -> str:
+ def project_name(self) -> str | None:
"""Resolve project name."""
return (
self.repository.project.name.lstrip(OWASP_ORGANIZATION_NAME)
if self.repository and self.repository.project
else 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.
| @strawberry.field | |
| def project_name(self) -> str: | |
| """Resolve project name.""" | |
| return ( | |
| self.repository.project.name.lstrip(OWASP_ORGANIZATION_NAME) | |
| if self.repository and self.repository.project | |
| else None | |
| ) | |
| @strawberry.field | |
| def project_name(self) -> str | None: | |
| """Resolve project name.""" | |
| return ( | |
| self.repository.project.name.lstrip(OWASP_ORGANIZATION_NAME) | |
| if self.repository and self.repository.project | |
| else None | |
| ) |
🤖 Prompt for AI Agents
In backend/apps/github/graphql/nodes/release.py around lines 37 to 44, the
method project_name is annotated to return a str but can return None if
repository or project is missing. Update the return type annotation to
Optional[str] to accurately reflect that the method may return None.
| @strawberry.field | ||
| def latest_release(self) -> str: | ||
| """Resolve latest release.""" | ||
| return self.latest_release | ||
|
|
There was a problem hiding this comment.
Type mismatch in latest_release resolver
The return type is str but self.latest_release is a property that returns a Release object or None based on the model definition.
Update the return type and handle the None case:
- def latest_release(self) -> str:
+ def latest_release(self) -> ReleaseNode | None:
"""Resolve latest release."""
return self.latest_release📝 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.
| @strawberry.field | |
| def latest_release(self) -> str: | |
| """Resolve latest release.""" | |
| return self.latest_release | |
| @strawberry.field | |
| def latest_release(self) -> ReleaseNode | None: | |
| """Resolve latest release.""" | |
| return self.latest_release |
🤖 Prompt for AI Agents
In backend/apps/github/graphql/nodes/repository.py around lines 49 to 53, the
latest_release resolver is declared to return a str but actually returns a
Release object or None. Update the return type annotation to Optional[Release]
or the appropriate type representing Release or None, and modify the method to
handle the None case properly, such as returning None or a default value when
self.latest_release is None.
| @strawberry.field | ||
| def top_contributors(self) -> list[RepositoryContributorNode]: | ||
| """Resolve top contributors.""" | ||
| return self.idx_top_contributors | ||
|
|
There was a problem hiding this comment.
Type mismatch in top_contributors resolver
The method returns self.idx_top_contributors directly, but the return type indicates it should return a list of RepositoryContributorNode objects.
Based on the pattern from GenericEntityNode, transform the raw data into node objects:
def top_contributors(self) -> list[RepositoryContributorNode]:
"""Resolve top contributors."""
- return self.idx_top_contributors
+ return [
+ RepositoryContributorNode(
+ avatar_url=tc["avatar_url"],
+ contributions_count=tc["contributions_count"],
+ login=tc["login"],
+ name=tc["name"],
+ project_key=tc.get("project_key", ""),
+ project_name=tc.get("project_name", ""),
+ )
+ for tc in self.idx_top_contributors
+ ]📝 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.
| @strawberry.field | |
| def top_contributors(self) -> list[RepositoryContributorNode]: | |
| """Resolve top contributors.""" | |
| return self.idx_top_contributors | |
| @strawberry.field | |
| def top_contributors(self) -> list[RepositoryContributorNode]: | |
| """Resolve top contributors.""" | |
| return [ | |
| RepositoryContributorNode( | |
| avatar_url=tc["avatar_url"], | |
| contributions_count=tc["contributions_count"], | |
| login=tc["login"], | |
| name=tc["name"], | |
| project_key=tc.get("project_key", ""), | |
| project_name=tc.get("project_name", ""), | |
| ) | |
| for tc in self.idx_top_contributors | |
| ] |
🤖 Prompt for AI Agents
In backend/apps/github/graphql/nodes/repository.py around lines 75 to 79, the
top_contributors method returns raw data directly instead of a list of
RepositoryContributorNode objects as indicated by the return type. To fix this,
transform each item in self.idx_top_contributors into a
RepositoryContributorNode instance before returning the list, following the
pattern used in GenericEntityNode for similar transformations.
ahmedxgouda
force-pushed
the
feature/improved-cache-mounts
branch
from
5f6056b to
f86a664
Compare
|
It was not that hard, just reverted to the old commit before GHA |
There was a problem hiding this comment.
@ahmedxgouda it looks very promising!
Could you take care of the schema image too?
Please also keep FORCE_COLOR where possible.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
schema/docker/Dockerfile.test (3)
18-23: Pin cache mount IDs for reproducibility
Using BuildKit cache mounts without an explicitidcan lead to unintended sharing or collisions. Namespace your APK cache mount:- RUN --mount=type=cache,target=${APK_CACHE_DIR} \ + RUN --mount=type=cache,id=apk-cache,target=${APK_CACHE_DIR} \ apk update && apk upgrade && \ addgroup -S owasp -g ${OWASP_GID} && \ adduser -S -h /home/owasp -u ${OWASP_UID} -G owasp owasp && \ mkdir -p /home/owasp && \ chown owasp:owasp /home/owaspThis change scopes the cache to a dedicated identifier.
25-26: Leverage cache IDs for pip cache mount
Similarly, namespace your pip cache to avoid cross-build pollution:- RUN --mount=type=cache,target=${PIP_CACHE_DIR} \ + RUN --mount=type=cache,id=pip-cache,target=${PIP_CACHE_DIR} \ python -m pip install poetry --cache-dir ${PIP_CACHE_DIR}Optionally, prepend
python -m pip install --upgrade pipto guarantee the latest pip before installing Poetry.
32-33: Isolate Poetry cache with explicit ID
Add a dedicated ID to the Poetry cache mount for deterministic behavior, and consider suppressing interactive output in CI:- RUN --mount=type=cache,target=${POETRY_CACHE_DIR},uid=${OWASP_UID},gid=${OWASP_GID} \ - poetry install --no-root + RUN --mount=type=cache,id=poetry-cache,target=${POETRY_CACHE_DIR},uid=${OWASP_UID},gid=${OWASP_GID} \ + poetry install --no-root --no-interaction --no-ansiThis helps keep CI logs clean and the cache isolated.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
backend/docker/Dockerfile(2 hunks)backend/docker/Dockerfile.local(2 hunks)backend/docker/Dockerfile.test(1 hunks)docs/docker/Dockerfile.local(2 hunks)frontend/docker/Dockerfile(1 hunks)frontend/docker/Dockerfile.e2e.test(1 hunks)frontend/docker/Dockerfile.local(2 hunks)frontend/docker/Dockerfile.unit.test(1 hunks)schema/docker/Dockerfile.test(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- frontend/docker/Dockerfile.unit.test
- frontend/docker/Dockerfile.e2e.test
- frontend/docker/Dockerfile.local
- backend/docker/Dockerfile.test
- backend/docker/Dockerfile.local
- frontend/docker/Dockerfile
- docs/docker/Dockerfile.local
- backend/docker/Dockerfile
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run schema tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (3)
schema/docker/Dockerfile.test (3)
3-3: Consistent shell withpipefailensures robust error handling
AddingSHELL ["/bin/sh", "-o", "pipefail", "-c"]guarantees that any failure in a piped command will result in a non-zero exit code, aligning this Dockerfile with best practices observed in other stages.
5-11: Cache directory environment variables added
Good addition of explicit cache directory paths for APK, pip, and Poetry. Centralizing these locations via ENV makes future updates and cache-mount references more maintainable.
41-41: Consistent shell settings in final stage
ReapplyingSHELL ["/bin/sh", "-o", "pipefail", "-c"]ensures that both build stages share the same robust error-handling behavior—nice consistency.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cspell/Dockerfile (1)
11-12: Ensure BuildKit is enabled & add explicit cache ID
YourRUN --mount=type=cache,target=${NPM_CACHE}relies on Docker BuildKit. To guarantee compatibility across environments, consider adding a syntax directive at the top of this Dockerfile (e.g.,# syntax=docker/dockerfile:1.4) and explicitly name the cache mount so it can be shared or referenced later:+# syntax=docker/dockerfile:1.4 ... RUN --mount=type=cache,id=npm,target=${NPM_CACHE} \ npm install --ignore-scripts -g pnpm --cache ${NPM_CACHE}This makes the cache reproducible and reusable in other steps.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cspell/Dockerfile(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (1)
cspell/Dockerfile (1)
6-8: Good: Centralized npm cache directory
Introducing theNPM_CACHEenvironment variable cleanly centralizes the npm cache path and makes the subsequent--mount=type=cacheinvocation more readable and maintainable.
Uh oh!
There was an error while loading. Please reload this page.