diff --git a/.github/workflows/actions-static-analysis.yml b/.github/workflows/actions-static-analysis.yml new file mode 100644 index 0000000000..bb72567963 --- /dev/null +++ b/.github/workflows/actions-static-analysis.yml @@ -0,0 +1,34 @@ +name: GitHub Actions Static Analysis + +on: + push: + branches: + - "main" + paths: + - ".github/workflows/**" + pull_request: + paths: + - ".github/workflows/**" + +permissions: {} + +jobs: + zizmor: + name: Run zizmor + runs-on: ubuntu-latest + permissions: + contents: read + actions: read + steps: + - name: Checkout repository + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + + - name: Run zizmor 🌈 + uses: zizmorcore/zizmor-action@3dc1ecc9bcb9e94e9b2c709687979e1298497054 # v0.6.2 + with: + inputs: ".github/workflows/" + min-severity: medium + min-confidence: medium + advanced-security: false diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 827325367f..7ae468c785 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1,5 +1,9 @@ name: CI on: [push] + +permissions: + contents: read + jobs: python-tests: runs-on: ubuntu-24.04 @@ -28,7 +32,9 @@ jobs: - 6379:6379 steps: - - uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6 + - uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1 + with: + persist-credentials: false - name: update apt run: sudo apt-get update -y @@ -37,7 +43,7 @@ jobs: run: cat Aptfile | sudo xargs apt-get install - name: Install uv - uses: astral-sh/setup-uv@v7 + uses: astral-sh/setup-uv@37802adc94f370d6bfd71619e3f0bf239e1f3b78 # v7.6.0 with: enable-cache: true @@ -84,8 +90,10 @@ jobs: javascript-tests: runs-on: ubuntu-24.04 steps: - - uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6 - - uses: actions/setup-node@6044e13b5dc448c55e2357c09f80417699197238 # v6 + - uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1 + with: + persist-credentials: false + - uses: actions/setup-node@6044e13b5dc448c55e2357c09f80417699197238 # v6.2.0 with: node-version: "^24.0.0" cache: yarn @@ -117,10 +125,11 @@ jobs: uses: SimenB/github-actions-cpu-cores@97ba232459a8e02ff6121db9362b09661c875ab8 # v2 - name: Tests - run: yarn test --max-workers ${{ steps.cpu-cores.outputs.count }} + run: yarn test --max-workers ${STEPS_CPU_CORES_OUTPUTS_COUNT} env: CODECOV: true NODE_ENV: test + STEPS_CPU_CORES_OUTPUTS_COUNT: ${{ steps.cpu-cores.outputs.count }} - name: Upload coverage to CodeCov uses: codecov/codecov-action@671740ac38dd9b0130fbe1cec585b89eea48d3de # v5.5.2 @@ -132,7 +141,9 @@ jobs: runs-on: ubuntu-24.04 steps: - name: Checkout - uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6 + uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1 + with: + persist-credentials: false - name: Build the Docker image env: @@ -147,9 +158,11 @@ jobs: runs-on: ubuntu-24.04 steps: - name: Checkout - uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6 + uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1 + with: + persist-credentials: false - - uses: actions/setup-node@6044e13b5dc448c55e2357c09f80417699197238 # v6 + - uses: actions/setup-node@6044e13b5dc448c55e2357c09f80417699197238 # v6.2.0 with: node-version: "^24.0.0" cache: yarn @@ -171,8 +184,10 @@ jobs: GENERATOR_OUTPUT_DIR_VC: ./frontends/api/src/generated/v0 runs-on: ubuntu-24.04 steps: - - uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6 - - uses: actions/setup-node@6044e13b5dc448c55e2357c09f80417699197238 # v6 + - uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1 + with: + persist-credentials: false + - uses: actions/setup-node@6044e13b5dc448c55e2357c09f80417699197238 # v6.2.0 with: node-version: "^24.0.0" cache: yarn @@ -182,7 +197,7 @@ jobs: run: yarn install --immutable - name: Generate Fresh API Client - uses: openapi-generators/openapitools-generator-action@v1 + uses: openapi-generators/openapitools-generator-action@515e8d70646f72de54d61c7f54b9447b7a1176f0 # v1 with: generator: typescript-axios openapi-file: $OPENAPI_SCHEMA @@ -210,8 +225,10 @@ jobs: GENERATOR_OUTPUT_DIR_VC: ./frontends/api/src/generated/v1 runs-on: ubuntu-24.04 steps: - - uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6 - - uses: actions/setup-node@6044e13b5dc448c55e2357c09f80417699197238 # v6 + - uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1 + with: + persist-credentials: false + - uses: actions/setup-node@6044e13b5dc448c55e2357c09f80417699197238 # v6.2.0 with: node-version: "^24.0.0" cache: yarn @@ -221,7 +238,7 @@ jobs: run: yarn install --immutable - name: Generate Fresh API Client - uses: openapi-generators/openapitools-generator-action@v1 + uses: openapi-generators/openapitools-generator-action@515e8d70646f72de54d61c7f54b9447b7a1176f0 # v1 with: generator: typescript-axios openapi-file: $OPENAPI_SCHEMA diff --git a/.github/workflows/openapi-diff.yml b/.github/workflows/openapi-diff.yml index cbb7770a22..b0c571520d 100644 --- a/.github/workflows/openapi-diff.yml +++ b/.github/workflows/openapi-diff.yml @@ -3,17 +3,22 @@ on: [pull_request] jobs: openapi-diff: runs-on: ubuntu-24.04 + permissions: + contents: read + pull-requests: write steps: - name: Checkout HEAD - uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6 + uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1 with: ref: ${{ github.head_ref }} path: head + persist-credentials: false - name: Checkout BASE - uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6 + uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1 with: ref: ${{ github.base_ref }} path: base + persist-credentials: false - name: Generate oasdiff summary id: oasdif_changelog run: | diff --git a/.github/workflows/publish-pages.yml b/.github/workflows/publish-pages.yml index aa123c6ca2..fb86ab697a 100644 --- a/.github/workflows/publish-pages.yml +++ b/.github/workflows/publish-pages.yml @@ -8,14 +8,19 @@ on: # Allows you to run this workflow manually from the Actions tab workflow_dispatch: +permissions: + contents: read + jobs: build: runs-on: ubuntu-24.04 steps: - name: Checkout - uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6 + uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1 + with: + persist-credentials: false - - uses: actions/setup-node@6044e13b5dc448c55e2357c09f80417699197238 # v6 + - uses: actions/setup-node@6044e13b5dc448c55e2357c09f80417699197238 # v6.2.0 with: node-version: "^24.0.0" cache: yarn diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index e538e2bd3a..edaf1ca459 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -122,6 +122,11 @@ repos: hooks: - id: shellcheck args: ["--severity=warning"] + - repo: https://github.com/zizmorcore/zizmor-pre-commit + rev: v1.29.0 + hooks: + - id: zizmor + args: [--no-progress, --min-severity=medium, --min-confidence=medium] - repo: local hooks: - id: drf-serializer-orm-check diff --git a/RELEASE.rst b/RELEASE.rst index 20d5e75bd0..4c9a90e832 100644 --- a/RELEASE.rst +++ b/RELEASE.rst @@ -1,6 +1,21 @@ Release Notes ============= +Version 0.77.3 +-------------- + +- fix broken images in subscription emails (#3737) +- Fix: render podcast show descriptions as HTML instead of raw markup (#3721) +- feat(content_feedback): allow anonymous submissions (#3738) +- Fix Canvas archive change detection: deterministic checksum, save after load (#3728) +- Skip unchanged edX course archives before downloading from S3 (#3722) +- remove GITHUB_ACCESS_TOKEN (#3730) +- Fix: HTML leaking into meta description tags (#3727) +- Make recreate index resilient to pod culling (#3716) +- Fix PostHog view-event ETL crash from duplicate view events (#3714) +- Use CMS Certificate Title for program LinkedIn "Add to Profile" (#3518) +- Harden GH Actions supply chain: add zizmor static analysis + 7-day dependency cool-down (#3712) + Version 0.77.2 (Released August 10, 2026) -------------- diff --git a/content_feedback/views.py b/content_feedback/views.py index f91f49fc8b..ff4c0708d8 100644 --- a/content_feedback/views.py +++ b/content_feedback/views.py @@ -2,7 +2,7 @@ from drf_spectacular.utils import OpenApiResponse, extend_schema from rest_framework.generics import CreateAPIView -from rest_framework.permissions import IsAuthenticated +from rest_framework.permissions import AllowAny from content_feedback.models import ContentFeedback from content_feedback.serializers import ContentFeedbackSerializer @@ -22,10 +22,14 @@ class ContentFeedbackView(CreateAPIView): queryset = ContentFeedback.objects.all() serializer_class = ContentFeedbackSerializer - permission_classes = (IsAuthenticated,) + # AllowAny: courseware-only learners have no mit-learn/APISIX session, so + # requiring auth would 403 nearly all of them. Authenticated rows record the + # user; anonymous rows store null (mirrors learn-ai/AskTIM). + permission_classes = (AllowAny,) throttle_classes = (RedisScopedRateThrottle,) throttle_scope = "content_feedback" def perform_create(self, serializer): - """Attribute the feedback to the authenticated user (server-side).""" - serializer.save(user=self.request.user) + """Attribute to the request user when authenticated, else store null.""" + user = self.request.user if self.request.user.is_authenticated else None + serializer.save(user=user) diff --git a/content_feedback/views_test.py b/content_feedback/views_test.py index c3b998f09c..a647a06166 100644 --- a/content_feedback/views_test.py +++ b/content_feedback/views_test.py @@ -2,6 +2,7 @@ import pytest from django.urls import reverse +from rest_framework.test import APIClient from content_feedback.factories import ContentFeedbackFactory from content_feedback.models import ContentFeedback @@ -26,10 +27,24 @@ def _payload(**overrides): return payload -def test_submit_requires_authentication(client): - """Anonymous users cannot submit feedback.""" +def test_submit_allows_anonymous(): + """Anonymous users can submit without a CSRF token; the record has no user.""" + # enforce_csrf_checks=True mirrors production SessionAuthentication: CSRF is + # only enforced for session-authenticated callers, so an anonymous POST with + # no token still succeeds. + client = APIClient(enforce_csrf_checks=True) response = client.post(reverse("content_feedback:v0:content_feedback"), _payload()) - assert response.status_code in (401, 403) + assert response.status_code == 201 + assert ContentFeedback.objects.count() == 1 + assert ContentFeedback.objects.get().user is None + + +def test_authenticated_without_csrf_token_rejected(user): + """A session-authenticated POST without a CSRF token is rejected (403).""" + client = APIClient(enforce_csrf_checks=True) + client.force_login(user) + response = client.post(reverse("content_feedback:v0:content_feedback"), _payload()) + assert response.status_code == 403 assert ContentFeedback.objects.count() == 0 @@ -119,7 +134,6 @@ def test_factory_builds_valid_record(): def test_submit_rate_limited(user_client, mocker): """Exceeding the per-user rate returns 429; a different user is unaffected.""" from django.core.cache.backends.locmem import LocMemCache - from rest_framework.test import APIClient from main.factories import UserFactory from main.throttles import RedisScopedRateThrottle @@ -141,6 +155,8 @@ def test_submit_rate_limited(user_client, mocker): # The limit is keyed per authenticated user: a different user still gets # through even after the first user is throttled. (The user_client fixture # shares one APIClient, so build a distinct client for the second user.) + # NB: anonymous requests instead key on client IP, which is spoofable via + # X-Forwarded-For -- tracked as a follow-up (mitodl/hq#12775). other_client = APIClient() other_client.force_login(UserFactory.create()) assert other_client.post(url, _payload()).status_code == 201 diff --git a/frontends/main/src/app-pages/PodcastPage/PodcastDetailPage.test.tsx b/frontends/main/src/app-pages/PodcastPage/PodcastDetailPage.test.tsx index cce80ce425..2a43638a36 100644 --- a/frontends/main/src/app-pages/PodcastPage/PodcastDetailPage.test.tsx +++ b/frontends/main/src/app-pages/PodcastPage/PodcastDetailPage.test.tsx @@ -45,12 +45,15 @@ const makePodcastEpisodes = (count: number): PodcastEpisodeResource[] => const setupApis = ({ episodesPage1, episodesPage2, + podcastOverrides = {}, }: { episodesPage1: LearningResource[] episodesPage2?: LearningResource[] + podcastOverrides?: Partial }) => { const podcast = factories.learningResources.resource({ resource_type: ResourceTypeEnum.Podcast, + ...podcastOverrides, }) // Episodes of this podcast reference it as their parent, as they would in @@ -202,6 +205,43 @@ describe("PodcastDetailPage", () => { await screen.findByText(episodes[0].title!) }) + test("renders a formatted show description", async () => { + const episodes = makePodcastEpisodes(1) + const { podcast } = setupApis({ + episodesPage1: episodes, + podcastOverrides: { + description: "

Daryl Morey & Jessica Gelman

", + }, + }) + + renderWithProviders() + + expect( + await screen.findByText("Daryl Morey & Jessica Gelman"), + ).toBeInTheDocument() + }) + + test("opens external links in the show description in a new tab", async () => { + const episodes = makePodcastEpisodes(1) + const { podcast } = setupApis({ + episodesPage1: episodes, + podcastOverrides: { + // rel="noopener noreferrer" mirrors real backend output: nh3 adds it + // to every during ETL sanitization, regardless of destination. + description: + 'Relevant Resources: OCW and Search.', + }, + }) + + renderWithProviders() + + const externalLink = await screen.findByRole("link", { name: "OCW" }) + expect(externalLink).toHaveAttribute("target", "_blank") + + const internalLink = screen.getByRole("link", { name: "Search" }) + expect(internalLink).not.toHaveAttribute("target") + }) + test("shows an error when the podcast fails to load", async () => { const podcast = factories.learningResources.resource({ resource_type: ResourceTypeEnum.Podcast, diff --git a/frontends/main/src/app-pages/PodcastPage/PodcastDetailPage.tsx b/frontends/main/src/app-pages/PodcastPage/PodcastDetailPage.tsx index edc1606ace..d4d0e9c2ee 100644 --- a/frontends/main/src/app-pages/PodcastPage/PodcastDetailPage.tsx +++ b/frontends/main/src/app-pages/PodcastPage/PodcastDetailPage.tsx @@ -1,7 +1,7 @@ "use client" -import React from "react" -import { Typography, Skeleton, styled } from "ol-components" +import React, { useMemo } from "react" +import { Typography, Skeleton, styled, TypographyProps } from "ol-components" import { Button } from "@mitodl/smoot-design" import { RiPlayFill, RiPauseFill } from "@remixicon/react" import { @@ -12,6 +12,7 @@ import { ResourceTypeEnum } from "api/v1" import type { LearningResource } from "api/v1" import { formatDate } from "ol-utilities" import { HOME, podcastEpisodePageView } from "@/common/urls" +import { addExternalLinkTargets } from "@/common/utils" import PodcastContainer from "./PodcastContainer" import PodcastBreadcrumbs from "./PodcastBreadcrumbs" import { usePodcastPage } from "./usePodcastPage" @@ -66,18 +67,28 @@ const MetaLine = styled(Typography)(({ theme }) => ({ }, })) -const Description = styled(Typography)(({ theme }) => ({ - color: theme.custom.colors.darkGray2, - display: "block", - marginBottom: "16px", - ...theme.typography.body1, - lineHeight: "26px", - [theme.breakpoints.down("sm")]: { - marginBottom: "8px", - ...theme.typography.body2, - lineHeight: "22px", - }, -})) +const Description = styled(Typography)>( + ({ theme }) => ({ + color: theme.custom.colors.darkGray2, + display: "block", + marginBottom: "16px", + ...theme.typography.body1, + lineHeight: "26px", + a: { + textDecoration: "underline", + color: theme.custom.colors.darkGray2, + fontWeight: theme.typography.fontWeightMedium, + }, + "a:hover": { + textDecoration: "none", + }, + [theme.breakpoints.down("sm")]: { + marginBottom: "8px", + ...theme.typography.body2, + lineHeight: "22px", + }, + }), +) const LatestEpisodeLine = styled(Typography)(({ theme }) => ({ color: theme.custom.colors.silverGrayDark, @@ -274,6 +285,21 @@ export const PodcastDetailPage: React.FC = ({ const handlePlayClick = (episode: LearningResource) => toggle(episode, id) + // Podcast descriptions are sanitized on the backend with nh3 during ETL + // (only is allowed), so the HTML is safe to render verbatim + // — the same trust model as podcast episode descriptions. Rendering it + // directly keeps server and client output identical, avoiding a hydration + // mismatch; target="_blank" is added via addExternalLinkTargets so it's + // part of the HTML fed to dangerouslySetInnerHTML on both server and + // client, keeping SSR output byte-identical to the client's first render. + const description = useMemo( + () => + resource?.description + ? addExternalLinkTargets(resource.description) + : null, + [resource?.description], + ) + return ( <> @@ -315,10 +341,14 @@ export const PodcastDetailPage: React.FC = ({ )} - {resource?.description && ( - - {resource.description} - + {description && ( + )} {latestEpisode && ( diff --git a/frontends/main/src/app-pages/PodcastPage/PodcastsListingPage/PodcastSection.test.tsx b/frontends/main/src/app-pages/PodcastPage/PodcastsListingPage/PodcastSection.test.tsx index f67ae94f00..51317c7f6f 100644 --- a/frontends/main/src/app-pages/PodcastPage/PodcastsListingPage/PodcastSection.test.tsx +++ b/frontends/main/src/app-pages/PodcastPage/PodcastsListingPage/PodcastSection.test.tsx @@ -71,6 +71,47 @@ describe("PodcastSection", () => { ) }) + it("renders a sanitized, formatted summary for featured series", () => { + const series = makeSeries({ + description: + '

Teaching & learning at MIT

', + }) + renderWithProviders( + , + ) + expect(screen.getByText("Teaching & learning at MIT")).toBeInTheDocument() + expect(document.querySelector("script")).not.toBeInTheDocument() + }) + + it("strips links from the featured summary, keeping their text, since the card is itself a link", () => { + const series = makeSeries({ + description: + 'Relevant Resources: OCW and Search.', + }) + renderWithProviders( + , + ) + expect( + screen.getByText("Relevant Resources: OCW and Search.", { + exact: false, + }), + ).toBeInTheDocument() + // Only the card's own outer link should be present — no nested . + expect(screen.getAllByRole("link", { name: /Chalk Radio/ })).toHaveLength(1) + }) + it("renders 'More Podcasts' rows with title and offered_by", () => { const series = makeSeries({ title: "The Aggregate", diff --git a/frontends/main/src/app-pages/PodcastPage/PodcastsListingPage/PodcastSection.tsx b/frontends/main/src/app-pages/PodcastPage/PodcastsListingPage/PodcastSection.tsx index 2cbbe0eae1..c5c3d5e4a9 100644 --- a/frontends/main/src/app-pages/PodcastPage/PodcastsListingPage/PodcastSection.tsx +++ b/frontends/main/src/app-pages/PodcastPage/PodcastsListingPage/PodcastSection.tsx @@ -4,9 +4,11 @@ import { Typography, Skeleton, styled } from "ol-components" import type { TypographyProps } from "ol-components" import { ButtonLink } from "@mitodl/smoot-design" import { RiArrowRightLine, RiArrowRightSLine } from "@remixicon/react" +import DOMPurify from "isomorphic-dompurify" import { formatDate } from "ol-utilities" import type { LearningResource } from "api/v1" import { SEARCH_PODCASTS, podcastPageView } from "@/common/urls" +import { stripAnchorTags } from "@/common/utils" import { Section, SectionHeader, @@ -109,7 +111,9 @@ const FeaturedPodcastTitle = styled(Typography)< marginBottom: "8px", })) -const FeaturedPodcastSummary = styled(Typography)(({ theme }) => ({ +const FeaturedPodcastSummary = styled(Typography)< + Pick +>(({ theme }) => ({ color: theme.custom.colors.silverGrayDark, lineHeight: "24px", marginBottom: "16px", @@ -117,6 +121,18 @@ const FeaturedPodcastSummary = styled(Typography)(({ theme }) => ({ WebkitBoxOrient: "vertical", WebkitLineClamp: 2, overflow: "hidden", + maxWidth: "100%", + overflowWrap: "break-word", + wordBreak: "break-word", + "& p": { + display: "inline", + margin: 0, + maxWidth: "100%", + overflowWrap: "break-word", + wordBreak: "break-word", + whiteSpace: "normal", + textWrap: "wrap", + }, })) const FeaturedPodcastMeta = styled(Typography)(({ theme }) => ({ @@ -324,9 +340,15 @@ const PodcastSection: React.FC = ({ {item.title} {item.description && ( - - {item.description} - + )} {[ diff --git a/frontends/main/src/common/certificateUtils.test.ts b/frontends/main/src/common/certificateUtils.test.ts index a0e3d0b0d7..1a358a2db2 100644 --- a/frontends/main/src/common/certificateUtils.test.ts +++ b/frontends/main/src/common/certificateUtils.test.ts @@ -1,9 +1,12 @@ import { + CertificateType, getCertificateBadgeLines, getCertificateBadgeTypography, getCertificateInfo, + getCertificateLinkedInUrl, getCertificateTitle, } from "./certificateUtils" +import { factories } from "api/test-utils" describe("getCertificateInfo", () => { it("returns default certificate label when no program type is provided", () => { @@ -148,3 +151,44 @@ describe("getCertificateBadgeTypography", () => { ) }) }) + +describe("getCertificateLinkedInUrl", () => { + const pageUrl = "https://example.com/certificate/program/abc" + + it("uses the CMS product name for program certificates", () => { + const certificate = factories.mitxonline.programCertificate() + certificate.certificate_page.product_name = "Universal AI" + certificate.program.title = "Fundamentals of Programming and ML" + + const url = new URL( + getCertificateLinkedInUrl(CertificateType.Program, certificate, pageUrl), + ) + + expect(url.searchParams.get("name")).toBe("Universal AI") + }) + + it("falls back to the program title when product name is empty", () => { + const certificate = factories.mitxonline.programCertificate() + certificate.certificate_page.product_name = "" + certificate.program.title = "Fundamentals of Programming and ML" + + const url = new URL( + getCertificateLinkedInUrl(CertificateType.Program, certificate, pageUrl), + ) + + expect(url.searchParams.get("name")).toBe( + "Fundamentals of Programming and ML", + ) + }) + + it("uses the course title for course certificates", () => { + const certificate = factories.mitxonline.courseCertificate() + certificate.course_run.course.title = "Intro to Python" + + const url = new URL( + getCertificateLinkedInUrl(CertificateType.Course, certificate, pageUrl), + ) + + expect(url.searchParams.get("name")).toBe("Intro to Python") + }) +}) diff --git a/frontends/main/src/common/certificateUtils.ts b/frontends/main/src/common/certificateUtils.ts index d40fdacb74..218985f106 100644 --- a/frontends/main/src/common/certificateUtils.ts +++ b/frontends/main/src/common/certificateUtils.ts @@ -253,10 +253,17 @@ export const getCertificateLinkedInUrl = ( certificateData: V2ProgramCertificate | V2CourseRunCertificate, pageUrl: string, ): string => { - const credentialName = - certificateType === CertificateType.Course - ? (certificateData as V2CourseRunCertificate).course_run.course.title - : (certificateData as V2ProgramCertificate).program.title + let credentialName: string + if (certificateType === CertificateType.Course) { + credentialName = (certificateData as V2CourseRunCertificate).course_run + .course.title + } else { + const programCert = certificateData as V2ProgramCertificate + credentialName = getCertificateTitle( + programCert.certificate_page?.product_name, + programCert.program.title, + ) + } const certId = certificateData.uuid const linkedinUrl = new URL(LINKEDIN_ADD_TO_PROFILE_BASE_URL) linkedinUrl.searchParams.set("startTask", "CERTIFICATION_NAME") diff --git a/frontends/main/src/common/htmlToPlainText.test.ts b/frontends/main/src/common/htmlToPlainText.test.ts new file mode 100644 index 0000000000..f1762ced8c --- /dev/null +++ b/frontends/main/src/common/htmlToPlainText.test.ts @@ -0,0 +1,43 @@ +import { htmlToPlainText } from "./htmlToPlainText" + +describe("htmlToPlainText", () => { + it("strips tags and decodes entities", () => { + expect(htmlToPlainText("

Daryl Morey & Jessica Gelman

")).toBe( + "Daryl Morey & Jessica Gelman", + ) + }) + + it("keeps a space between adjacent block-level elements", () => { + expect(htmlToPlainText("

First

Second

")).toBe("First Second") + }) + + it("keeps a space where a
separates lines", () => { + expect(htmlToPlainText("Line one
Line two")).toBe("Line one Line two") + }) + + it("keeps a space between adjacent table cells", () => { + expect( + htmlToPlainText("
AB
"), + ).toBe("A B") + }) + + it("keeps a space after a blockquote or pre block", () => { + expect( + htmlToPlainText("
Quote
Code
Text"), + ).toBe("Quote Code Text") + }) + + it("strips links but keeps their text", () => { + expect( + htmlToPlainText('

OCW resources

'), + ).toBe("OCW resources") + }) + + it("leaves plain text unchanged", () => { + expect(htmlToPlainText("Just plain text")).toBe("Just plain text") + }) + + it("returns an empty string for empty input", () => { + expect(htmlToPlainText("")).toBe("") + }) +}) diff --git a/frontends/main/src/common/htmlToPlainText.ts b/frontends/main/src/common/htmlToPlainText.ts new file mode 100644 index 0000000000..d2f540e882 --- /dev/null +++ b/frontends/main/src/common/htmlToPlainText.ts @@ -0,0 +1,33 @@ +import DOMPurify from "isomorphic-dompurify" +import { collapseWhitespace } from "@/common/utils" + +const BLOCK_BOUNDARY_TAGS = + /<\/(?:p|div|li|h[1-6]|td|th|tr|blockquote|pre)>|/gi + +/** + * Converts a sanitized-HTML string (e.g. a resource `description`) to plain + * text suitable for contexts that must not contain markup, like , og:description, and twitter:description. Strips all + * tags and decodes entities (& -> &); a space is inserted at block-level + * boundaries first so adjacent paragraphs/list items don't get mashed + * together once their tags are removed. + * + * Kept out of common/utils.ts and imported only by server-only code (e.g. + * metadata.ts): isomorphic-dompurify has no `sideEffects: false`, so any + * client component importing anything from utils.ts would otherwise pull + * DOMPurify into its bundle even when htmlToPlainText itself is unused. + */ +const htmlToPlainText = (html: string): string => { + if (!html) return "" + const withBreaks = html.replace(BLOCK_BOUNDARY_TAGS, (match) => `${match} `) + // RETURN_DOM_FRAGMENT gives back real DOM nodes rather than a serialized + // HTML string, so reading .textContent decodes entities for free (a + // serialized-string result stays HTML-escaped, e.g. "&", since it's + // meant to be re-inserted as HTML). + const fragment = DOMPurify.sanitize(withBreaks, { + RETURN_DOM_FRAGMENT: true, + }) + return collapseWhitespace(fragment.textContent ?? "") +} + +export { htmlToPlainText } diff --git a/frontends/main/src/common/metadata.test.ts b/frontends/main/src/common/metadata.test.ts index 32c935b42e..7abefca8c6 100644 --- a/frontends/main/src/common/metadata.test.ts +++ b/frontends/main/src/common/metadata.test.ts @@ -68,6 +68,18 @@ describe("safeGenerateMetadata", () => { }) }) +describe("standardizeMetadata", () => { + test("converts an HTML description to plain text in all description fields", async () => { + const meta = await standardizeMetadata({ + description: "

Daryl Morey & Jessica Gelman

", + }) + + expect(meta.description).toBe("Daryl Morey & Jessica Gelman") + expect(meta.openGraph?.description).toBe("Daryl Morey & Jessica Gelman") + expect(meta.twitter?.description).toBe("Daryl Morey & Jessica Gelman") + }) +}) + describe("getMetadataAsync drawer canonical", () => { test("emits a slugged separate-param canonical for a valid ?resource=", async () => { const resource = factories.learningResources.course() diff --git a/frontends/main/src/common/metadata.ts b/frontends/main/src/common/metadata.ts index 3cae33da60..53aa8e9111 100644 --- a/frontends/main/src/common/metadata.ts +++ b/frontends/main/src/common/metadata.ts @@ -4,6 +4,7 @@ import { RESOURCE_DRAWER_PARAMS, } from "@/common/urls" import { parseResourceId } from "@/common/slugs" +import { htmlToPlainText } from "@/common/htmlToPlainText" import type { AxiosError } from "axios" import type { Metadata } from "next" import * as Sentry from "@sentry/nextjs" @@ -86,7 +87,7 @@ export const getMetadataAsync = async ({ learningResourceQueries.detail(learningResourceId), ) title = data?.title - description = data?.description?.replace(/<\/[^>]+(>|$)/g, "") ?? "" + description = data?.description ?? "" image = data?.image?.url || image imageAlt = image === data?.image?.url ? imageAlt : data?.image?.alt || "" alts.canonical = canonicalResourceDrawerUrl(learningResourceId, data?.title) @@ -118,6 +119,7 @@ export const standardizeMetadata = ({ ...otherMeta }: MetadataProps = {}): Metadata => { title = `${title} | ${env("NEXT_PUBLIC_SITE_NAME")}` + description = htmlToPlainText(description) const socialMetadata = social ? { openGraph: { diff --git a/learning_resources/etl/canvas.py b/learning_resources/etl/canvas.py index 24337d749c..4a71c35d7e 100644 --- a/learning_resources/etl/canvas.py +++ b/learning_resources/etl/canvas.py @@ -15,6 +15,7 @@ PlatformType, ) from learning_resources.etl.canvas_utils import ( + canvas_course_checksum, canvas_course_url, canvas_url_config, get_published_items, @@ -22,7 +23,6 @@ ) from learning_resources.etl.constants import ETLSource from learning_resources.etl.utils import ( - calc_checksum, get_edx_module_id, process_olx_path, ) @@ -52,38 +52,46 @@ def sync_canvas_archive(bucket, key: str, overwrite): course_archive_path = Path(export_tempdir, key.rsplit("/", maxsplit=1)[-1]) bucket.download_file(key, course_archive_path) url_config = canvas_url_config(bucket, export_tempdir, url_config_file) + checksum = canvas_course_checksum(course_archive_path, url_config) resource_readable_id, run = run_for_canvas_archive( - course_archive_path, course_folder=course_folder, overwrite=overwrite + course_archive_path, + course_folder=course_folder, + checksum=checksum, + overwrite=overwrite, ) - checksum = calc_checksum(course_archive_path) if run: canvas_content_files = list( transform_canvas_content_files( course_archive_path, run, url_config=url_config, overwrite=overwrite ) ) - load_content_files( + content_files_ids = load_content_files( run, canvas_content_files, ) - load_problem_files( - run, + canvas_problem_files = list( transform_canvas_problem_files( course_archive_path, run, overwrite=overwrite - ), + ) ) - run.checksum = checksum - run.save() + problem_files_ids = load_problem_files(run, canvas_problem_files) + content_loaded = content_files_ids or not canvas_content_files + # load_problem_file swallows per-file errors and returns None + problems_loaded = any(problem_files_ids) or not canvas_problem_files + if content_loaded and problems_loaded: + # a failed or empty load must be retried on the next sync, so + # only mark processed once everything loaded (or was unpublished) + run.checksum = checksum + run.save(update_fields=["checksum"]) return resource_readable_id -def run_for_canvas_archive(course_archive_path, course_folder, overwrite): +def run_for_canvas_archive(course_archive_path, course_folder, checksum, overwrite): """ Generate and return a LearningResourceRun for a Canvas course """ - checksum = calc_checksum(course_archive_path) course_info = parse_canvas_settings(course_archive_path) course_title = course_info.get("title", f"canvas course {course_folder}") url = canvas_course_url(course_archive_path) @@ -130,8 +138,6 @@ def run_for_canvas_archive(course_archive_path, course_folder, overwrite): if run.checksum == checksum and not overwrite: log.debug("Checksums match for %s, skipping load", readable_id) return resource_readable_id, None - run.checksum = checksum - run.save() return resource_readable_id, run diff --git a/learning_resources/etl/canvas_test.py b/learning_resources/etl/canvas_test.py index 2e1517a308..e182d6febd 100644 --- a/learning_resources/etl/canvas_test.py +++ b/learning_resources/etl/canvas_test.py @@ -3,19 +3,23 @@ import zipfile from datetime import timedelta from pathlib import Path +from types import SimpleNamespace from unittest.mock import MagicMock import pytest from defusedxml import ElementTree +from freezegun import freeze_time from learning_resources.constants import LearningResourceType, PlatformType from learning_resources.etl.canvas import ( run_for_canvas_archive, + sync_canvas_archive, transform_canvas_content_files, transform_canvas_problem_files, ) from learning_resources.etl.canvas_utils import ( _compact_element, + canvas_course_checksum, get_published_items, is_file_published, parse_canvas_files, @@ -179,12 +183,11 @@ def test_run_for_canvas_archive_creates_resource_and_run(tmp_path, mocker): return_value={"course_id": "123", "canvas_domain": "mit.edu"}, ) - mocker.patch("learning_resources.etl.canvas.calc_checksum", return_value="abc123") # No resource exists yet zip_path = tmp_path / "archive.zip" _, run = run_for_canvas_archive( - zip_path, course_folder=course_folder, overwrite=True + zip_path, course_folder=course_folder, checksum="abc123", overwrite=True ) resource = LearningResource.objects.get(readable_id=f"{course_folder}-TEST101") assert resource.title == "Test Course" @@ -193,7 +196,9 @@ def test_run_for_canvas_archive_creates_resource_and_run(tmp_path, mocker): assert resource.platform.code == PlatformType.canvas.name assert run is not None assert run.learning_resource == resource - assert run.checksum == "abc123" + # checksum is only saved after a successful content load in + # sync_canvas_archive, never by run_for_canvas_archive + assert run.checksum is None @pytest.mark.django_db @@ -210,9 +215,6 @@ def test_run_for_canvas_archive_creates_run_if_none_exists(tmp_path, mocker): "learning_resources.etl.canvas_utils.parse_context_xml", return_value={"course_id": "123", "canvas_domain": "mit.edu"}, ) - mocker.patch( - "learning_resources.etl.canvas.calc_checksum", return_value="checksum104" - ) # Create resource with no runs resource = LearningResourceFactory.create( readable_id=f"{course_folder}-TEST104", @@ -226,11 +228,14 @@ def test_run_for_canvas_archive_creates_run_if_none_exists(tmp_path, mocker): course_archive_path = tmp_path / "archive4.zip" course_archive_path.write_text("dummy") _, run = run_for_canvas_archive( - course_archive_path, course_folder=course_folder, overwrite=True + course_archive_path, + course_folder=course_folder, + checksum="checksum104", + overwrite=True, ) assert run is not None assert run.learning_resource == resource - assert run.checksum == "checksum104" + assert run.checksum is None def make_canvas_zip( @@ -1994,7 +1999,9 @@ def test_ingestion_finishes_with_missing_xml_files( "learning_resources.etl.utils.extract_text_metadata", return_value={"content": "test"}, ) - _, run = run_for_canvas_archive(zip_path, tmp_path, overwrite=True) + _, run = run_for_canvas_archive( + zip_path, tmp_path, checksum="abc123", overwrite=True + ) content_results = list( transform_canvas_content_files( Path(zip_path), run, url_config={}, overwrite=True @@ -2088,3 +2095,283 @@ def test_empty_pdf_is_skipped(tmp_path): ) ) assert result == [] + + +LOCK_FILES_XML_TEMPLATE = """ + + + + {display_name} + {unlock_at} + uncategorized + + + +""" + +LOCK_MANIFEST_XML = b""" + + + + + + + +""" + + +def make_timed_lock_zip( # noqa: PLR0913 + tmp_path, + unlock_at, + name="lock_course.zip", + settings_xml=DEFAULT_SETTINGS_XML, + manifest_xml=LOCK_MANIFEST_XML, + display_name="file3", + content=b"one", +): + """Course archive with one file whose visibility is gated by unlock_at""" + zip_path = tmp_path / name + with zipfile.ZipFile(zip_path, "w") as zf: + zf.writestr("course_settings/course_settings.xml", settings_xml) + zf.writestr( + "course_settings/files_meta.xml", + LOCK_FILES_XML_TEMPLATE.format( + unlock_at=unlock_at, display_name=display_name + ).encode(), + ) + zf.writestr("imsmanifest.xml", manifest_xml) + zf.writestr("web_resources/file3.html", content) + return zip_path + + +UNLOCKED = "2001-01-01T00:00:00" + + +def test_canvas_course_checksum_ignores_export_noise(tmp_path): + """ + Archives differing only in imsmanifest.xml bytes and course_settings/* + bytes AND size must produce the same digest — Canvas rewrites these on + every no-op export (verified across 13 prod course pairs). + """ + a = make_timed_lock_zip(tmp_path, UNLOCKED, name="a.zip") + b = make_timed_lock_zip( + tmp_path, + UNLOCKED, + name="b.zip", + settings_xml=DEFAULT_SETTINGS_XML + b"", + manifest_xml=LOCK_MANIFEST_XML.replace(b"", b""), + ) + assert canvas_course_checksum(a, {}) == canvas_course_checksum(b, {}) + + +def test_canvas_course_checksum_detects_same_size_content_edit(tmp_path): + """ + A content member with identical name and size but different bytes must + change the digest — parity with the per-file archive_checksum gate that + the archive-level skip would otherwise shadow. + """ + a = make_timed_lock_zip( + tmp_path, UNLOCKED, name="a.zip", content=b"one" + ) + b = make_timed_lock_zip( + tmp_path, UNLOCKED, name="b.zip", content=b"two" + ) + assert canvas_course_checksum(a, {}) != canvas_course_checksum(b, {}) + + +def test_canvas_course_checksum_independent_of_member_order(tmp_path): + """Zip directory order must not affect the digest""" + members = [ + ("web_resources/x.html", b"xx"), + ("web_resources/y.html", b"yy"), + ("course_settings/course_settings.xml", DEFAULT_SETTINGS_XML), + ] + digests = [] + for name, order in (("a.zip", members), ("b.zip", members[::-1])): + with zipfile.ZipFile(tmp_path / name, "w") as zf: + for member, content in order: + zf.writestr(member, content) + digests.append(canvas_course_checksum(tmp_path / name, {})) + assert digests[0] == digests[1] + + +def test_canvas_course_checksum_sensitive_to_url_config(tmp_path): + """ + url_config (from the .metadata.json sidecar) feeds ContentFile urls; a + changed url must change the digest even when the archive is unchanged. + """ + zip_path = make_timed_lock_zip(tmp_path, UNLOCKED) + with_url = {"/file3.html": {"url": "https://mit.edu/f/1"}} + reminted = {"/file3.html": {"url": "https://mit.edu/f/2"}} + assert canvas_course_checksum(zip_path, with_url) != canvas_course_checksum( + zip_path, reminted + ) + + +def test_canvas_course_checksum_sensitive_to_display_name(tmp_path): + """ + A same-length display_name change lives in the excluded files_meta.xml; + the publish-set component must still catch it (it feeds content_title). + """ + a = make_timed_lock_zip(tmp_path, UNLOCKED, name="a.zip", display_name="fileA") + b = make_timed_lock_zip(tmp_path, UNLOCKED, name="b.zip", display_name="fileB") + assert canvas_course_checksum(a, {}) != canvas_course_checksum(b, {}) + + +def test_canvas_course_checksum_changes_when_lock_boundary_passes(tmp_path): + """ + The same archive must produce a different checksum once a timed lock + boundary passes — publish status depends on wall-clock time, and the gate + must reprocess when it flips. + """ + zip_path = make_timed_lock_zip(tmp_path, "2026-09-01T00:00:00") + with freeze_time("2026-08-15"): + while_locked = canvas_course_checksum(zip_path, {}) + with freeze_time("2026-09-15"): + after_unlock = canvas_course_checksum(zip_path, {}) + assert while_locked != after_unlock + + +def test_canvas_course_checksum_is_cwd_independent(tmp_path, monkeypatch): + """Checksum must not embed the worker process's working directory""" + zip_path = make_timed_lock_zip(tmp_path, "2001-01-01T00:00:00") + digest = canvas_course_checksum(zip_path, {}) + other_cwd = tmp_path / "elsewhere" + other_cwd.mkdir() + monkeypatch.chdir(other_cwd) + assert canvas_course_checksum(zip_path, {}) == digest + + +@pytest.fixture +def sync_mocks(mocker, tmp_path): + """Bucket/url_config/loader mocks for sync_canvas_archive""" + zip_path = make_timed_lock_zip(tmp_path, "2001-01-01T00:00:00") + + def fake_download(key, dest): + Path(dest).write_bytes(zip_path.read_bytes()) + + bucket = MagicMock() + bucket.download_file.side_effect = fake_download + mocker.patch("learning_resources.etl.canvas.canvas_url_config", return_value={}) + mocker.patch( + "learning_resources.etl.utils.extract_text_metadata", + return_value={"content": "TEXT"}, + ) + mocker.patch( + "learning_resources.etl.canvas_utils.parse_context_xml", + return_value={"course_id": "123", "canvas_domain": "mit.edu"}, + ) + load_content = mocker.patch("learning_resources.etl.loaders.load_content_files") + load_problems = mocker.patch("learning_resources.etl.loaders.load_problem_files") + return SimpleNamespace( + bucket=bucket, load_content=load_content, load_problems=load_problems + ) + + +def _canvas_run(readable_id): + return LearningResource.objects.get(readable_id=readable_id).runs.first() + + +def test_sync_canvas_archive_skips_unchanged_archive(sync_mocks): + """A second sync of an unchanged archive must not reload content""" + readable_id = sync_canvas_archive( + sync_mocks.bucket, "canvas/course_content/1/abc.imscc", overwrite=False + ) + assert sync_mocks.load_content.call_count == 1 + first_checksum = _canvas_run(readable_id).checksum + assert first_checksum + + sync_canvas_archive( + sync_mocks.bucket, "canvas/course_content/1/abc.imscc", overwrite=False + ) + assert sync_mocks.load_content.call_count == 1 + assert _canvas_run(readable_id).checksum == first_checksum + + +def test_sync_canvas_archive_saves_checksum_only_after_successful_load(sync_mocks): + """ + A failed load must leave the checksum unset so the next sync retries + instead of silently skipping content that was never ingested. + """ + sync_mocks.load_content.side_effect = Exception("load failed") + with pytest.raises(Exception, match="load failed"): + sync_canvas_archive( + sync_mocks.bucket, "canvas/course_content/1/abc.imscc", overwrite=False + ) + resource = LearningResource.objects.get(etl_source=ETLSource.canvas.name) + assert resource.runs.first().checksum is None + + sync_mocks.load_content.side_effect = None + sync_canvas_archive( + sync_mocks.bucket, "canvas/course_content/1/abc.imscc", overwrite=False + ) + assert sync_mocks.load_content.call_count == 2 + assert resource.runs.first().checksum + + +def test_sync_canvas_archive_retries_when_nothing_loads(sync_mocks): + """ + load_content_files returning [] (all files failed without raising) must + NOT save the checksum — the next sync retries instead of skipping content + that was never ingested. + """ + sync_mocks.load_content.return_value = [] + readable_id = sync_canvas_archive( + sync_mocks.bucket, "canvas/course_content/1/abc.imscc", overwrite=False + ) + assert _canvas_run(readable_id).checksum is None + + sync_mocks.load_content.return_value = [1] + sync_canvas_archive( + sync_mocks.bucket, "canvas/course_content/1/abc.imscc", overwrite=False + ) + assert sync_mocks.load_content.call_count == 2 + assert _canvas_run(readable_id).checksum + + +def test_sync_canvas_archive_retries_when_problem_files_fail(mocker, sync_mocks): + """ + Tutor problem files yielded but none loaded (load_problem_file swallows + per-file errors and returns None) must NOT save the checksum, so the next + sync retries. + """ + mocker.patch( + "learning_resources.etl.canvas.transform_canvas_problem_files", + side_effect=lambda *_args, **_kwargs: iter( + [{"source_path": "tutorbot/p1/problem.pdf"}] + ), + ) + sync_mocks.load_problems.return_value = [None] + readable_id = sync_canvas_archive( + sync_mocks.bucket, "canvas/course_content/1/abc.imscc", overwrite=False + ) + assert _canvas_run(readable_id).checksum is None + + sync_mocks.load_problems.return_value = [7] + sync_canvas_archive( + sync_mocks.bucket, "canvas/course_content/1/abc.imscc", overwrite=False + ) + assert _canvas_run(readable_id).checksum + + +def test_sync_canvas_archive_saves_checksum_for_legitimately_empty_course( + mocker, sync_mocks, tmp_path +): + """ + A course with nothing published yields no payloads and loads nothing; + that's not a failure — save the checksum so it isn't reprocessed weekly. + (The publish-set digest component unfreezes it if anything is published.) + """ + locked_zip = make_timed_lock_zip( + tmp_path, "2099-01-01T00:00:00", name="all_locked.zip" + ) + + def fake_download(key, dest): + Path(dest).write_bytes(locked_zip.read_bytes()) + + sync_mocks.bucket.download_file.side_effect = fake_download + sync_mocks.load_content.return_value = [] + readable_id = sync_canvas_archive( + sync_mocks.bucket, "canvas/course_content/1/abc.imscc", overwrite=False + ) + assert _canvas_run(readable_id).checksum diff --git a/learning_resources/etl/canvas_utils.py b/learning_resources/etl/canvas_utils.py index 5686bd3d3f..4132011053 100644 --- a/learning_resources/etl/canvas_utils.py +++ b/learning_resources/etl/canvas_utils.py @@ -1,9 +1,11 @@ import json import logging +import os import sys import zipfile from collections import defaultdict from datetime import UTC +from hashlib import md5 from pathlib import Path from urllib.parse import unquote, unquote_plus from zoneinfo import ZoneInfo @@ -706,3 +708,40 @@ def get_published_items(zipfile_path, url_config): published_items[embedded_path] = embedded return published_items + + +# Export-noise carriers: Canvas rewrites these on every no-op export +# (imsmanifest.xml embeds the export timestamp and reshuffles resources; +# course_settings/* change bytes AND size — verified across 13 prod course +# pairs). They are excluded from the byte digest; their semantic content +# (publish state, titles) is covered by the publish-set component below. +# Nothing under these paths is ever ingested as a content file (IGNORE_FILES). +CHECKSUM_EXCLUDED = ("imsmanifest.xml", "course_settings/") + + +def canvas_course_checksum(course_archive_path, url_config: dict) -> str: + """ + Digest of archive content + effective publish state + url metadata. + + Changes when a content member's bytes change (name/size/CRC, excluding + the export-noise carriers), when the published set or a display title + changes (publish toggles, timed lock/unlock boundaries passing), or when + the .metadata.json url config changes — every way a course's ingested + content can change. Stable across Canvas's no-op scheduled exports. + """ + hasher = md5() # noqa: S324 - non-cryptographic change detection + with zipfile.ZipFile(course_archive_path, "r") as course_archive: + for info in sorted(course_archive.infolist(), key=lambda i: i.filename): + if info.filename.startswith(CHECKSUM_EXCLUDED): + continue + hasher.update(f"{info.filename}\0{info.file_size}\0{info.CRC}\0".encode()) + published = get_published_items(course_archive_path, url_config) + # relpath: get_published_items keys are resolve()'d against the process + # cwd, which must not leak into the digest + for path, title in sorted( + (os.path.relpath(path), item.get("title") or "") + for path, item in published.items() + ): + hasher.update(f"{path}\0{title}\0".encode()) + hasher.update(json.dumps(url_config, sort_keys=True, default=str).encode()) + return hasher.hexdigest() diff --git a/learning_resources/etl/edx_shared.py b/learning_resources/etl/edx_shared.py index 365210123d..e75c93bde5 100644 --- a/learning_resources/etl/edx_shared.py +++ b/learning_resources/etl/edx_shared.py @@ -1,6 +1,7 @@ """Shared functions for EdX sites""" import logging +from itertools import chain from pathlib import Path from tarfile import ReadError from tempfile import TemporaryDirectory @@ -114,9 +115,10 @@ def build_run_lookup( def process_course_archive( bucket, key: str, run: LearningResourceRun, *, overwrite: bool = False -) -> None: +) -> bool: """ - Download and process a course archive from S3. + Download and process a course archive from S3, skipping the download + entirely when run.archive_key already matches the content-addressed key. Args: bucket: S3 bucket object @@ -125,8 +127,11 @@ def process_course_archive( overwrite(bool): Whether to overwrite existing content files Returns: - bool: True if successfully processed, False if skipped due to matching checksum + bool: False if skipped via matching archive_key, True otherwise """ + if run.archive_key == key and not overwrite: + log.debug("Archive key unchanged for %s, skipping download", key) + return False with TemporaryDirectory() as export_tempdir: course_tarpath = Path(export_tempdir, key.rsplit("/", maxsplit=1)[-1]) log.info("course tarpath for run %s is %s", run.run_id, course_tarpath) @@ -135,20 +140,35 @@ def process_course_archive( checksum = calc_checksum(course_tarpath) except ReadError: log.exception("Error reading tar file %s, skipping", course_tarpath) - return False + return True if run.checksum == checksum and not overwrite: + # unchanged content under a new key: record it to skip future downloads + run.archive_key = key + run.save(update_fields=["archive_key"]) log.info("Checksums match for %s, skipping load", key) - return False + return True try: + content_files_data = iter( + transform_content_files(course_tarpath, run, overwrite=overwrite) + ) + first = next(content_files_data, None) + if first is None: + # empty archive: stop re-downloading it + run.archive_key = key + run.save(update_fields=["archive_key"]) + return True content_files_ids = load_content_files( - run, - transform_content_files(course_tarpath, run, overwrite=overwrite), + run, chain([first], content_files_data) ) if content_files_ids: run.checksum = checksum - run.save(update_fields=["checksum"]) + run.archive_key = key + run.save(update_fields=["checksum", "archive_key"]) + # else: files yielded but none loaded — save nothing so the next + # sync retries except: # noqa: E722 log.exception("Error ingesting OLX content data for %s", key) + return True def get_most_recent_course_archives(etl_source: str) -> list[str]: @@ -315,6 +335,7 @@ def sync_edx_course_files( bucket = get_bucket_by_name(settings.COURSE_ARCHIVE_BUCKET_NAME) run_lookup = build_run_lookup(etl_source, ids) + skipped = processed = 0 for key in keys: normalized_key_id = extract_run_id_from_key(etl_source, key) matching_runs = run_lookup.get(normalized_key_id) @@ -327,4 +348,13 @@ def sync_edx_course_files( log.warning("There are %d runs for %s", len(matching_runs), key) run = matching_runs[0] - process_course_archive(bucket, key, run, overwrite=overwrite) + if process_course_archive(bucket, key, run, overwrite=overwrite): + processed += 1 + else: + skipped += 1 + log.info( + "%s content file sync: %d unchanged archives skipped, %d processed", + etl_source, + skipped, + processed, + ) diff --git a/learning_resources/etl/edx_shared_test.py b/learning_resources/etl/edx_shared_test.py index d702f84861..31a0070fa2 100644 --- a/learning_resources/etl/edx_shared_test.py +++ b/learning_resources/etl/edx_shared_test.py @@ -161,6 +161,39 @@ def test_sync_edx_course_files_matching_checksum(mocker, mock_course_archive_buc mock_load.assert_not_called() mock_index.assert_not_called() + run.refresh_from_db() + assert run.archive_key == key + + +def test_sync_edx_course_files_skips_unchanged_archive_keys(mocker): + """Keys matching a run's archive_key skip without downloading; summary logged""" + run = LearningResourceFactory.create( + is_course=True, create_runs=True, etl_source=ETLSource.mitxonline.name + ).best_run + key = ( + f"{get_s3_prefix_for_source(ETLSource.mitxonline.name)}/{run.run_id}/foo.tar.gz" + ) + run.archive_key = key + run.save() + bucket = mocker.MagicMock() + mocker.patch( + "learning_resources.etl.edx_shared.get_bucket_by_name", + return_value=bucket, + ) + mock_load = mocker.patch("learning_resources.etl.edx_shared.load_content_files") + mock_log = mocker.patch("learning_resources.etl.edx_shared.log.info") + + sync_edx_course_files("mitxonline", [run.learning_resource.id], [key]) + + bucket.download_file.assert_not_called() + mock_load.assert_not_called() + mock_log.assert_any_call( + "%s content file sync: %d unchanged archives skipped, %d processed", + "mitxonline", + 1, + 0, + ) + @pytest.mark.parametrize("source", [ETLSource.mitxonline.value, ETLSource.xpro.value]) def test_sync_edx_course_files_invalid_tarfile( @@ -257,7 +290,10 @@ def test_sync_edx_course_files_error(mock_course_archive_bucket, mocker, source) sync_edx_course_files(source, [run.learning_resource.id], [key]) assert mock_transform.call_count == 1 assert str(mock_transform.call_args[0][0]).endswith("foo.tar.gz") is True - mock_load_content_files.assert_called_once_with(run, fake_data) + mock_load_content_files.assert_called_once() + called_run, called_data = mock_load_content_files.call_args[0] + assert called_run == run + assert list(called_data) == list(fake_data) assert mock_log.call_args[0][0].startswith("Error ingesting OLX content data for ") @@ -358,8 +394,13 @@ def test_sync_edx_course_files_test_mode_all_runs_processed( assert mock_load_content_files.call_count == 3 # Verify each run was processed + called_runs = [call.args[0] for call in mock_load_content_files.call_args_list] + called_data = [ + list(call.args[1]) for call in mock_load_content_files.call_args_list + ] for run in runs: - mock_load_content_files.assert_any_call(run, fake_data) + assert run in called_runs + assert called_data == [list(fake_data)] * 3 @pytest.mark.parametrize("source", [ETLSource.mit_edx.value, ETLSource.xpro.value]) @@ -681,7 +722,10 @@ def test_sync_edx_archive_success( sync_edx_archive(etl_source, s3_key, overwrite=False) mock_transform.assert_called_once() - mock_load.assert_called_once_with(run, '{"key": "data"}') + mock_load.assert_called_once() + called_run, called_data = mock_load.call_args[0] + assert called_run == run + assert list(called_data) == list('{"key": "data"}') run.refresh_from_db() assert run.checksum is not None @@ -868,7 +912,10 @@ def test_sync_edx_archive_test_mode_all_runs( sync_edx_archive(etl_source, s3_key, overwrite=False) mock_transform.assert_called_once() - mock_load.assert_called_once_with(old_run, '{"key": "data"}') + mock_load.assert_called_once() + called_run, called_data = mock_load.call_args[0] + assert called_run == old_run + assert list(called_data) == list('{"key": "data"}') @pytest.mark.parametrize("etl_source", [ETLSource.mitxonline.name, ETLSource.xpro.name]) @@ -1315,8 +1362,8 @@ def test_build_run_lookup_cross_format_prefix_match(): assert lookup[normalized_key][0].id == run.id -def test_process_course_archive_does_not_set_checksum_on_empty_ingest(mocker): - """process_course_archive should not update run.checksum if load_content_files returns empty list""" +def test_process_course_archive_saves_nothing_when_all_files_fail_ingest(mocker): + """process_course_archive should not update run.checksum or run.archive_key if all files fail to load""" run = LearningResourceRunFactory.create(published=True, checksum=None) bucket = mocker.MagicMock() key = "mitxonline/courses/course-v1:Test+Course+R1/archive.tar.gz" @@ -1327,17 +1374,22 @@ def test_process_course_archive_does_not_set_checksum_on_empty_ingest(mocker): ) mocker.patch( "learning_resources.etl.edx_shared.transform_content_files", - return_value=iter([]), + return_value=iter([{"key": "content.txt"}]), ) + + def fake_load(run_arg, data, **kwargs): + list(data) # consume the generator like the real loader + return [] + mocker.patch( - "learning_resources.etl.edx_shared.load_content_files", - return_value=[], + "learning_resources.etl.edx_shared.load_content_files", side_effect=fake_load ) process_course_archive(bucket, key, run) run.refresh_from_db() assert run.checksum is None + assert run.archive_key is None def test_process_course_archive_sets_checksum_on_successful_ingest(mocker): @@ -1352,7 +1404,7 @@ def test_process_course_archive_sets_checksum_on_successful_ingest(mocker): ) mocker.patch( "learning_resources.etl.edx_shared.transform_content_files", - return_value=iter([]), + return_value=iter([{"key": "content.txt"}]), ) mocker.patch( "learning_resources.etl.edx_shared.load_content_files", @@ -1388,3 +1440,128 @@ def test_process_course_archive_does_not_set_checksum_on_exception(mocker): run.refresh_from_db() assert run.checksum == "oldchecksum" + + +def test_process_course_archive_skips_download_when_key_matches(mocker): + """A stored archive_key equal to the S3 key should skip without downloading""" + key = "mitxonline/openedx/raw_data/course_xml/course-v1:Test+Course+R1/abc123.xml.tar.gz" + run = LearningResourceRunFactory.create( + published=True, archive_key=key, checksum="oldchecksum" + ) + bucket = mocker.MagicMock() + mock_load = mocker.patch("learning_resources.etl.edx_shared.load_content_files") + + process_course_archive(bucket, key, run) + + bucket.download_file.assert_not_called() + mock_load.assert_not_called() + run.refresh_from_db() + assert run.archive_key == key + assert run.checksum == "oldchecksum" + + +def test_process_course_archive_stamps_key_on_checksum_match(mocker): + """A matching checksum with a stale archive_key should stamp the key, no load""" + key = "mitxonline/openedx/raw_data/course_xml/course-v1:Test+Course+R1/abc123.xml.tar.gz" + run = LearningResourceRunFactory.create( + published=True, archive_key=None, checksum="samechecksum" + ) + bucket = mocker.MagicMock() + mocker.patch( + "learning_resources.etl.edx_shared.calc_checksum", return_value="samechecksum" + ) + mock_load = mocker.patch("learning_resources.etl.edx_shared.load_content_files") + + process_course_archive(bucket, key, run) + + bucket.download_file.assert_called_once() + mock_load.assert_not_called() + run.refresh_from_db() + assert run.archive_key == key + assert run.checksum == "samechecksum" + + +def test_process_course_archive_saves_both_fields_on_load(mocker): + """A changed archive should load and save both checksum and archive_key""" + key = "mitxonline/openedx/raw_data/course_xml/course-v1:Test+Course+R1/abc123.xml.tar.gz" + run = LearningResourceRunFactory.create( + published=True, archive_key=None, checksum=None + ) + bucket = mocker.MagicMock() + mocker.patch( + "learning_resources.etl.edx_shared.calc_checksum", return_value="newchecksum" + ) + mocker.patch( + "learning_resources.etl.edx_shared.transform_content_files", + return_value=iter([{"key": "content.txt"}]), + ) + + def fake_load(run_arg, data, **kwargs): + list(data) # consume the generator like the real loader + return [1, 2, 3] + + mocker.patch( + "learning_resources.etl.edx_shared.load_content_files", side_effect=fake_load + ) + + process_course_archive(bucket, key, run) + + run.refresh_from_db() + assert run.archive_key == key + assert run.checksum == "newchecksum" + + +def test_process_course_archive_stamps_key_for_empty_archive(mocker): + """An archive whose transform yields no payloads should stamp archive_key only""" + key = "mitxonline/openedx/raw_data/course_xml/course-v1:Test+Course+R1/abc123.xml.tar.gz" + run = LearningResourceRunFactory.create( + published=True, archive_key=None, checksum=None + ) + bucket = mocker.MagicMock() + mocker.patch( + "learning_resources.etl.edx_shared.calc_checksum", return_value="newchecksum" + ) + mocker.patch( + "learning_resources.etl.edx_shared.transform_content_files", + return_value=iter([]), + ) + mock_load = mocker.patch("learning_resources.etl.edx_shared.load_content_files") + + process_course_archive(bucket, key, run) + + mock_load.assert_not_called() + run.refresh_from_db() + assert run.archive_key == key + assert run.checksum is None + + +def test_process_course_archive_overwrite_bypasses_key_gate(mocker): + """overwrite=True should download and load even when the stored key matches""" + key = "mitxonline/openedx/raw_data/course_xml/course-v1:Test+Course+R1/abc123.xml.tar.gz" + run = LearningResourceRunFactory.create( + published=True, archive_key=key, checksum=None + ) + bucket = mocker.MagicMock() + mocker.patch( + "learning_resources.etl.edx_shared.calc_checksum", return_value="newchecksum" + ) + mocker.patch( + "learning_resources.etl.edx_shared.transform_content_files", + return_value=iter([{"key": "content.txt"}]), + ) + + def fake_load(run_arg, data, **kwargs): + list(data) + return [1] + + mock_load = mocker.patch( + "learning_resources.etl.edx_shared.load_content_files", side_effect=fake_load + ) + + process_course_archive(bucket, key, run, overwrite=True) + + bucket.download_file.assert_called_once() + mock_load.assert_called_once() + run.refresh_from_db() + assert run.archive_key == key + assert run.checksum == "newchecksum" diff --git a/learning_resources/etl/loaders_test.py b/learning_resources/etl/loaders_test.py index fa8c962b4a..f6e254c280 100644 --- a/learning_resources/etl/loaders_test.py +++ b/learning_resources/etl/loaders_test.py @@ -2019,6 +2019,10 @@ def test_load_test_mode_resource_content_files( autospec=True, return_value=[], ) + mocker.patch( + "learning_resources.etl.edx_shared.transform_content_files", + return_value=iter(content_data), + ) mocker.patch( "learning_resources_search.plugins.tasks.deindex_run_content_files", autospec=True, @@ -2045,7 +2049,8 @@ def test_load_test_mode_resource_content_files( ) if test_mode: - assert len(mock_load_content_files.mock_calls[0].args) == len(content_data) + _, data_arg = mock_load_content_files.mock_calls[0].args + assert list(data_arg) == content_data else: assert mock_load_content_files.call_count == 0 diff --git a/learning_resources/etl/podcast.py b/learning_resources/etl/podcast.py index 43838ca212..4eb78c2208 100644 --- a/learning_resources/etl/podcast.py +++ b/learning_resources/etl/podcast.py @@ -40,10 +40,7 @@ def github_podcast_config_files(): A list of pyGithub contentFile objects """ # noqa: D401 - if settings.GITHUB_ACCESS_TOKEN: - github_client = github.Github(settings.GITHUB_ACCESS_TOKEN) - else: - github_client = github.Github() + github_client = github.Github() repo = github_client.get_repo(CONFIG_FILE_REPO) diff --git a/learning_resources/etl/podcast_test.py b/learning_resources/etl/podcast_test.py index 1025c188a5..64d57abddf 100644 --- a/learning_resources/etl/podcast_test.py +++ b/learning_resources/etl/podcast_test.py @@ -326,10 +326,8 @@ def test_generate_aggregate_podcast_rss(): assert result == bs(expected_rss, "xml").prettify() -@pytest.mark.parametrize("github_token", [None, "token"]) -def test_github_podcast_config_files(settings, mock_github_client, github_token): +def test_github_podcast_config_files(settings, mock_github_client): """Test the logic for retrieving podcast config files from github""" - settings.GITHUB_ACCESS_TOKEN = github_token mock_github_client.return_value.get_repo.return_value.get_contents.return_value = [ mock_podcast_file(), mock_podcast_file(), diff --git a/learning_resources/etl/posthog.py b/learning_resources/etl/posthog.py index b5b9ea4dd0..2d0f63184f 100644 --- a/learning_resources/etl/posthog.py +++ b/learning_resources/etl/posthog.py @@ -10,6 +10,7 @@ import boto3 import pandas as pd from django.conf import settings +from django.db import IntegrityError, transaction from learning_resources.models import LearningResource, LearningResourceViewEvent from learning_resources.utils import resource_upserted_actions @@ -28,6 +29,7 @@ class PostHogLearningResourceViewEvent: resource_id: int event_date: datetime + event_uuid: str def posthog_extract_lrd_view_events() -> Generator[dict, None, None]: @@ -94,11 +96,23 @@ def posthog_transform_lrd_view_events( # The PostHog data files contain other kinds of events, for example llm calls. # We only want to the resource views - if resource_id: - yield PostHogLearningResourceViewEvent( - resource_id=resource_id, - event_date=event.get("timestamp"), + if not resource_id: + continue + + event_uuid = event.get("uuid") + if not event_uuid: + # PostHog assigns every event a uuid, so this indicates + # malformed source data + log.warning( + "Skipping lrd_view event without a uuid for resource %s", resource_id ) + continue + + yield PostHogLearningResourceViewEvent( + resource_id=resource_id, + event_date=event.get("timestamp"), + event_uuid=event_uuid, + ) def load_posthog_lrd_view_event( @@ -136,9 +150,45 @@ def load_posthog_lrd_view_event( log.warning(skip_warning) return None - lr_event, _ = LearningResourceViewEvent.objects.update_or_create( - learning_resource=learning_resource, - event_date=event.event_date, + # The newest S3 file is re-read on every run (its last_modified is always + # later than the events it holds), so most events arrive already stored. + # Return early: stamping this uuid onto another legacy duplicate below + # would violate the unique index. + existing = LearningResourceViewEvent.objects.filter( + event_uuid=event.event_uuid + ).first() + if existing: + return existing + + # Adopt a matching legacy row (loaded before event_uuid existed) so re-read + # S3 files don't duplicate it. Stamp only the oldest one; the legacy tail + # can hold duplicate (resource, event_date) rows. + legacy_row = ( + LearningResourceViewEvent.objects.filter( + learning_resource=learning_resource, + event_date=event.event_date, + event_uuid__isnull=True, + ) + .order_by("id") + .first() + ) + if legacy_row: + try: + # Savepoint: the check above is not a lock, so a concurrent run can + # adopt a different duplicate for this uuid first. get_or_create + # below then finds that row. + with transaction.atomic(): + LearningResourceViewEvent.objects.filter(pk=legacy_row.pk).update( + event_uuid=event.event_uuid + ) + except IntegrityError: + log.info( + "Legacy view event row for resource %s was adopted concurrently", + event.resource_id, + ) + + lr_event, _ = LearningResourceViewEvent.objects.get_or_create( + event_uuid=event.event_uuid, defaults={ "learning_resource": learning_resource, "event_date": event.event_date, diff --git a/learning_resources/etl/posthog_test.py b/learning_resources/etl/posthog_test.py index 3fe0cc2bd9..75ece8f240 100644 --- a/learning_resources/etl/posthog_test.py +++ b/learning_resources/etl/posthog_test.py @@ -4,6 +4,7 @@ from pathlib import Path import pytest +from django.db import IntegrityError from freezegun import freeze_time from learning_resources.etl import posthog @@ -137,3 +138,81 @@ def test_load_posthog_lrd_view_events( else: assert LearningResourceViewEvent.objects.count() == 0 assert len([event for event in loaded_events if event is not None]) == 0 + + +@pytest.mark.django_db +@pytest.mark.parametrize("runs", [1, 2]) +def test_load_posthog_lrd_view_events_duplicate_legacy_rows( + mocker, mock_posthog_event_bucket, settings, runs +): + """Duplicate legacy rows are tolerated, and re-running the ETL stays idempotent""" + LearningResourceViewEvent.objects.all().delete() + bucket = mock_posthog_event_bucket.bucket + settings.POSTHOG_EVENT_S3_BUCKET = bucket.name + settings.POSTHOG_EVENT_S3_PREFIX = "events/" + with Path.open(Path("test_json/posthog/test_data.parquet.zst"), "rb") as infile: + bucket.put_object( + Key="events/file1.parquet.zst", + Body=infile.read(), + ACL="public-read", + ) + + resource = LearningResourceFactory.create(id=3235) + mocker.patch( + "learning_resources.etl.posthog.resource_upserted_actions", + autospec=True, + ) + + event_date = datetime(2025, 8, 28, 15, 20, 13, 620000, tzinfo=UTC) + legacy_rows = LearningResourceViewEventFactory.create_batch( + 2, learning_resource=resource, event_date=event_date + ) + + for _ in range(runs): + posthog.load_posthog_lrd_view_events( + posthog.posthog_transform_lrd_view_events( + posthog.posthog_extract_lrd_view_events() + ) + ) + + stamped = [ + row + for row in LearningResourceViewEvent.objects.filter( + pk__in=[r.pk for r in legacy_rows] + ) + if row.event_uuid is not None + ] + assert len(stamped) == 1 + # The unstamped duplicate survives rather than being deleted + assert ( + LearningResourceViewEvent.objects.filter( + learning_resource=resource, event_date=event_date, event_uuid__isnull=True + ).count() + == 1 + ) + + +@pytest.mark.django_db +def test_load_posthog_lrd_view_event_adoption_lost_to_concurrent_run(mocker): + """Losing the adoption race to a concurrent run doesn't crash the loader""" + resource = LearningResourceFactory.create() + event_date = now_in_utc() + LearningResourceViewEventFactory.create( + learning_resource=resource, event_date=event_date + ) + event_uuid = "0198f143-cf8c-79b6-bab8-9c9063659a54" + # Simulates another worker stamping a different duplicate with this uuid + # between our existence check and the adoption update + mocker.patch( + "django.db.models.QuerySet.update", + side_effect=IntegrityError("duplicate key value violates unique constraint"), + ) + + lr_event = posthog.load_posthog_lrd_view_event( + posthog.PostHogLearningResourceViewEvent( + resource_id=resource.id, event_date=event_date, event_uuid=event_uuid + ) + ) + + assert lr_event is not None + assert LearningResourceViewEvent.objects.filter(event_uuid=event_uuid).count() == 1 diff --git a/learning_resources/etl/utils.py b/learning_resources/etl/utils.py index b32a8a4265..d06c1628ad 100644 --- a/learning_resources/etl/utils.py +++ b/learning_resources/etl/utils.py @@ -10,7 +10,6 @@ import re import tarfile import uuid -import zipfile from collections import Counter from collections.abc import Generator from datetime import UTC, datetime @@ -899,18 +898,13 @@ def get_bucket_by_name(bucket_name: str) -> object: def calc_checksum(filename) -> str: """ - Return the md5 checksum of the specified filepath + Return a checksum for the specified tar archive Args: - filename(str): The path to the file to checksum + filename(str): The path to the tar archive to checksum Returns: - str: The md5 checksum of the file + str: checksum derived from the archive members' header checksums """ - if zipfile.is_zipfile(filename): - with zipfile.ZipFile(filename, "r") as zip_file: - return str( - hash(tuple(f"{zp.filename}:{zp.file_size}" for zp in zip_file.filelist)) - ) with tarfile.open(filename, "r") as tgz_file: return str(hash(tuple(ti.chksum for ti in tgz_file.getmembers()))) diff --git a/learning_resources/migrations/0119_unique_resource_view_event.py b/learning_resources/migrations/0119_unique_resource_view_event.py new file mode 100644 index 0000000000..58c5fe3b96 --- /dev/null +++ b/learning_resources/migrations/0119_unique_resource_view_event.py @@ -0,0 +1,28 @@ +"""Add the PostHog event UUID column to LearningResourceViewEvent""" + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("learning_resources", "0118_delete_micromasters_resources"), + ] + + operations = [ + # Nullable column with no default: metadata-only, no table rewrite. + # The unique index is built CONCURRENTLY in the next migration. + # Pre-existing duplicate rows are left alone - they all have a NULL + # event_uuid, which never conflicts in a unique index. + migrations.AddField( + model_name="learningresourceviewevent", + name="event_uuid", + field=models.UUIDField( + editable=False, + help_text=( + "The PostHog event UUID. Null only for rows loaded" + " before this field existed." + ), + null=True, + ), + ), + ] diff --git a/learning_resources/migrations/0120_view_event_uuid_unique_index.py b/learning_resources/migrations/0120_view_event_uuid_unique_index.py new file mode 100644 index 0000000000..e555527d01 --- /dev/null +++ b/learning_resources/migrations/0120_view_event_uuid_unique_index.py @@ -0,0 +1,46 @@ +"""Build the unique index on event_uuid concurrently to avoid blocking reads""" + +from django.db import migrations, models +from django.db.models import Q + +INDEX_NAME = "learning_resources_lrviewevent_event_uuid_uniq" +TABLE_NAME = "learning_resources_learningresourceviewevent" + + +class Migration(migrations.Migration): + # CREATE INDEX CONCURRENTLY cannot run inside a transaction + atomic = False + + dependencies = [ + ("learning_resources", "0119_unique_resource_view_event"), + ] + + operations = [ + migrations.SeparateDatabaseAndState( + state_operations=[ + migrations.AddConstraint( + model_name="learningresourceviewevent", + constraint=models.UniqueConstraint( + condition=Q(event_uuid__isnull=False), + fields=("event_uuid",), + name=INDEX_NAME, + ), + ), + ], + database_operations=[ + migrations.RunSQL( + [ + # A failed CONCURRENTLY build leaves an INVALID index + # that IF NOT EXISTS would silently accept; drop any + # leftover so a rerun rebuilds and enforces uniqueness + f"DROP INDEX CONCURRENTLY IF EXISTS {INDEX_NAME}", + # Partial: the legacy NULL rows need no index entries + f"CREATE UNIQUE INDEX CONCURRENTLY {INDEX_NAME}" + f" ON {TABLE_NAME} (event_uuid)" + f" WHERE event_uuid IS NOT NULL", + ], + reverse_sql=f"DROP INDEX CONCURRENTLY IF EXISTS {INDEX_NAME}", + ), + ], + ), + ] diff --git a/learning_resources/migrations/0121_learningresourcerun_archive_key.py b/learning_resources/migrations/0121_learningresourcerun_archive_key.py new file mode 100644 index 0000000000..169cbf537c --- /dev/null +++ b/learning_resources/migrations/0121_learningresourcerun_archive_key.py @@ -0,0 +1,17 @@ +# Generated by Django 4.2.30 on 2026-07-23 14:14 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("learning_resources", "0120_view_event_uuid_unique_index"), + ] + + operations = [ + migrations.AddField( + model_name="learningresourcerun", + name="archive_key", + field=models.CharField(blank=True, max_length=512, null=True), + ), + ] diff --git a/learning_resources/models.py b/learning_resources/models.py index 488fbf812e..f7d54aa81a 100644 --- a/learning_resources/models.py +++ b/learning_resources/models.py @@ -828,6 +828,8 @@ class LearningResourceRun(TimestampedModel): ) resource_prices = models.ManyToManyField(LearningResourcePrice, blank=True) checksum = models.CharField(max_length=32, null=True, blank=True) # noqa: DJ001 + # S3 key of the last-processed course archive (content-addressed) + archive_key = models.CharField(max_length=512, null=True, blank=True) # noqa: DJ001 delivery = ArrayField( models.CharField( max_length=24, db_index=True, choices=LearningResourceDelivery.as_tuple() @@ -1566,6 +1568,25 @@ class LearningResourceViewEvent(TimestampedModel): editable=False, help_text="The date of the lrd_view event, as collected by PostHog.", ) + event_uuid = models.UUIDField( + null=True, + editable=False, + help_text=( + "The PostHog event UUID. Null only for rows loaded" + " before this field existed." + ), + ) + + class Meta: + constraints = [ + # Conditional so the index skips the legacy NULL rows, which would + # otherwise cost ~105MB of index for entries nothing ever probes. + models.UniqueConstraint( + fields=["event_uuid"], + condition=Q(event_uuid__isnull=False), + name="learning_resources_lrviewevent_event_uuid_uniq", + ) + ] def __str__(self): """Return a string representation of the event.""" diff --git a/learning_resources/serializers.py b/learning_resources/serializers.py index 836dbf55cc..53408e7aa4 100644 --- a/learning_resources/serializers.py +++ b/learning_resources/serializers.py @@ -390,7 +390,13 @@ class LearningResourceRunSerializer(serializers.ModelSerializer): class Meta: model = models.LearningResourceRun - exclude = ["learning_resource", "is_b2b", "is_variant", *COMMON_IGNORED_FIELDS] + exclude = [ + "learning_resource", + "is_b2b", + "is_variant", + "archive_key", + *COMMON_IGNORED_FIELDS, + ] class ResourceListMixin(serializers.Serializer): diff --git a/learning_resources_search/constants.py b/learning_resources_search/constants.py index dd6a1a535e..9603d0aa0e 100644 --- a/learning_resources_search/constants.py +++ b/learning_resources_search/constants.py @@ -56,6 +56,21 @@ class IndexestoUpdate(Enum): all_indexes = "all_indexes" +# TaskJob.task_name for recreate_index jobs +REINDEX_TASK_NAME = "recreate_index" + + +class ReindexBatchKind(Enum): + """ + Enum for the kinds of TaskBatch used by recreate_index jobs + """ + + learning_resources = "learning_resources" + content_files = "content_files" + percolate = "percolate" + dispatch_content_files = "dispatch_content_files" + + LEARNING_RESOURCE_TYPES = ( COURSE_TYPE, PROGRAM_TYPE, diff --git a/learning_resources_search/indexing_api.py b/learning_resources_search/indexing_api.py index 7e23248e5e..042b3ee982 100644 --- a/learning_resources_search/indexing_api.py +++ b/learning_resources_search/indexing_api.py @@ -640,6 +640,23 @@ def switch_indices(backing_index, object_type): log.warning("Reindex alias not found for %s", object_type) +def is_default_backing_index(backing_index, object_type): + """ + Check whether the default alias already points at the given backing index + + Args: + backing_index (str): The backing index to check + object_type (str): The object type for the index + + Returns: + bool: True if the default alias already points at backing_index + """ + conn = get_conn() + return conn.indices.exists_alias( + index=backing_index, name=get_default_alias_name(object_type) + ) + + def delete_orphaned_indexes(obj_types, delete_reindexing_tags): """ Delete any indices without aliases diff --git a/learning_resources_search/management/commands/recreate_index.py b/learning_resources_search/management/commands/recreate_index.py index 420ad3127e..9e22d47c60 100644 --- a/learning_resources_search/management/commands/recreate_index.py +++ b/learning_resources_search/management/commands/recreate_index.py @@ -1,11 +1,16 @@ """Management command to index content""" -from django.core.management.base import BaseCommand, CommandError +from django.core.management.base import BaseCommand +from django.db.models import Count -from learning_resources_search.constants import ALL_INDEX_TYPES, HYBRID_COMBINED_INDEX +from learning_resources_search.constants import ( + ALL_INDEX_TYPES, + HYBRID_COMBINED_INDEX, + REINDEX_TASK_NAME, +) from learning_resources_search.indexing_api import get_existing_reindexing_indexes from learning_resources_search.tasks import start_recreate_index -from main.utils import now_in_utc +from main.models import TaskJob class Command(BaseCommand): @@ -15,10 +20,13 @@ class Command(BaseCommand): def add_arguments(self, parser): parser.add_argument( - "--remove_existing_reindexing_tags", - dest="remove_existing_reindexing_tags", + "--restart", + dest="restart", action="store_true", - help="Overwrite any existing reindexing tags and remove those indexes", + help=( + "Discard any in-progress reindex (existing reindexing indexes and" + " active jobs) and start over" + ), ) parser.add_argument( @@ -32,6 +40,19 @@ def add_arguments(self, parser): help="Recreate combined index for hybrid search", ) + parser.add_argument( + "--status", + dest="status", + nargs="?", + type=int, + const=0, + default=None, + help=( + "Print the status of a reindex job instead of starting one " + "(defaults to the most recent job)" + ), + ) + for object_type in sorted(ALL_INDEX_TYPES): if object_type != HYBRID_COMBINED_INDEX: parser.add_argument( @@ -42,11 +63,40 @@ def add_arguments(self, parser): ) super().add_arguments(parser) + def print_job_status(self, job_id): + """Print the status of a reindex job""" + jobs = TaskJob.objects.filter(task_name=REINDEX_TASK_NAME) + job = jobs.filter(id=job_id).first() if job_id else jobs.order_by("-id").first() + if not job: + self.stdout.write("No reindex job found") + return + self.stdout.write(f"Reindex job {job.id}: {job.status}") + self.stdout.write(f" indexes: {job.params.get('indexes')}") + self.stdout.write(f" backing indexes: {job.params.get('backing_indexes')}") + self.stdout.write(" batches:") + batch_counts = job.batches.values("kind", "status").annotate(count=Count("id")) + kind_counts = {} + for row in batch_counts: + kind_counts.setdefault(row["kind"], {})[row["status"]] = row["count"] + for kind, counts in sorted(kind_counts.items()): + self.stdout.write(f" {kind}: {counts}") + if job.error: + self.stdout.write(f" error: {job.error}") + def handle(self, *args, **options): # noqa: ARG002 """Index all LEARNING_RESOURCE_TYPES""" - remove_existing_reindexing_tags = options["remove_existing_reindexing_tags"] + if options["status"] is not None: + self.print_job_status(options["status"]) + return + + restart = options["restart"] if options["all"]: - indexes_to_update = list(ALL_INDEX_TYPES) + # the combined hybrid index is still experimental and expensive to + # build; keep it out of --all and require the explicit + # --combined_hybrid flag to reindex it + indexes_to_update = [ + index for index in ALL_INDEX_TYPES if index != HYBRID_COMBINED_INDEX + ] else: indexes_to_update = list( filter( @@ -61,7 +111,15 @@ def handle(self, *args, **options): # noqa: ARG002 for object_type in sorted(ALL_INDEX_TYPES): self.stdout.write(f" --{object_type}s") return - if not remove_existing_reindexing_tags: + + active_jobs = [ + job + for job in TaskJob.objects.filter( + task_name=REINDEX_TASK_NAME, status__in=TaskJob.ACTIVE_STATUSES + ) + if set(job.params.get("indexes", [])) & set(indexes_to_update) + ] + if not restart: existing_reindexing_indexes = get_existing_reindexing_indexes( indexes_to_update ) @@ -69,24 +127,39 @@ def handle(self, *args, **options): # noqa: ARG002 self.stdout.write( f"Reindexing in progress. Reindexing indexes already exist:" f" {', '.join(existing_reindexing_indexes)}" - f"\nUse --remove_existing_reindexing_tags if you want to continue" + f"\nUse --restart if you want to continue" + ) + return + if active_jobs: + self.stdout.write( + f"Reindexing in progress. Active reindex jobs already exist:" + f" {', '.join(str(job.id) for job in active_jobs)}" + f"\nUse --restart if you want to continue" ) return - task = start_recreate_index.delay( - indexes_to_update, remove_existing_reindexing_tags - ) - self.stdout.write( - f"Started celery task {task} to index content for the following" - f" indexes: {indexes_to_update}" + job = TaskJob.objects.create( + task_name=REINDEX_TASK_NAME, + params={ + "indexes": indexes_to_update, + "restart": restart, + }, ) - self.stdout.write("Waiting on task...") - start = now_in_utc() - error = task.get() - if error: - msg = f"Recreate index errored: {error}" - raise CommandError(msg) + if restart: + for active_job in active_jobs: + TaskJob.objects.filter( + id=active_job.id, status__in=TaskJob.ACTIVE_STATUSES + ).update( + status=TaskJob.Status.FAILED, + error=f"superseded by reindex job {job.id}", + ) - total_seconds = (now_in_utc() - start).total_seconds() - self.stdout.write(f"Recreate index finished, took {total_seconds} seconds") + start_recreate_index.delay(job.id) + self.stdout.write( + f"Started reindex job {job.id} for the following indexes:" + f" {indexes_to_update}" + ) + self.stdout.write( + f"Check progress with: ./manage.py recreate_index --status {job.id}" + ) diff --git a/learning_resources_search/management/commands/recreate_index_test.py b/learning_resources_search/management/commands/recreate_index_test.py new file mode 100644 index 0000000000..58b3d4bd6b --- /dev/null +++ b/learning_resources_search/management/commands/recreate_index_test.py @@ -0,0 +1,195 @@ +"""Tests for the recreate_index management command""" + +from io import StringIO + +import pytest +from django.core.management import call_command + +from learning_resources_search.constants import ( + ALL_INDEX_TYPES, + HYBRID_COMBINED_INDEX, + REINDEX_TASK_NAME, +) +from main.factories import TaskBatchFactory, TaskJobFactory +from main.models import TaskBatch, TaskJob + +pytestmark = pytest.mark.django_db + + +@pytest.fixture +def start_recreate_index_mock(mocker): + """Mock the start_recreate_index task""" + return mocker.patch( + "learning_resources_search.management.commands.recreate_index.start_recreate_index", + autospec=True, + ) + + +@pytest.fixture +def _no_existing_reindexing_indexes(mocker): + """Mock get_existing_reindexing_indexes to return nothing""" + mocker.patch( + "learning_resources_search.management.commands.recreate_index.get_existing_reindexing_indexes", + autospec=True, + return_value=[], + ) + + +@pytest.mark.usefixtures("_no_existing_reindexing_indexes") +def test_recreate_index_starts_job(start_recreate_index_mock): + """The command should create a job, enqueue the start task and exit""" + stdout = StringIO() + call_command("recreate_index", "--programs", stdout=stdout) + + job = TaskJob.objects.get() + assert job.task_name == REINDEX_TASK_NAME + assert job.params == { + "indexes": ["program"], + "restart": False, + } + assert job.status == TaskJob.Status.QUEUED + start_recreate_index_mock.delay.assert_called_once_with(job.id) + output = stdout.getvalue() + assert f"Started reindex job {job.id}" in output + assert f"--status {job.id}" in output + + +@pytest.mark.usefixtures("_no_existing_reindexing_indexes") +def test_recreate_index_all_excludes_combined_hybrid(start_recreate_index_mock): + """--all should reindex every type except the experimental hybrid index""" + call_command("recreate_index", "--all", stdout=StringIO()) + + job = TaskJob.objects.get() + assert HYBRID_COMBINED_INDEX not in job.params["indexes"] + assert set(job.params["indexes"]) == { + index for index in ALL_INDEX_TYPES if index != HYBRID_COMBINED_INDEX + } + + +@pytest.mark.usefixtures("_no_existing_reindexing_indexes") +def test_recreate_index_combined_hybrid_flag(start_recreate_index_mock): + """--combined_hybrid should still reindex the hybrid index explicitly""" + call_command("recreate_index", "--combined_hybrid", stdout=StringIO()) + + job = TaskJob.objects.get() + assert job.params["indexes"] == [HYBRID_COMBINED_INDEX] + + +@pytest.mark.usefixtures("_no_existing_reindexing_indexes") +def test_recreate_index_blocks_on_active_job(start_recreate_index_mock): + """The command should not start a job when an overlapping job is active""" + active_job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, + params={"indexes": ["program"]}, + status=TaskJob.Status.RUNNING, + ) + + stdout = StringIO() + call_command("recreate_index", "--programs", stdout=stdout) + + assert TaskJob.objects.count() == 1 + start_recreate_index_mock.delay.assert_not_called() + assert str(active_job.id) in stdout.getvalue() + + +@pytest.mark.usefixtures("_no_existing_reindexing_indexes") +def test_recreate_index_supersedes_active_job(start_recreate_index_mock): + """--restart should fail overlapping active jobs""" + active_job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, + params={"indexes": ["program"]}, + status=TaskJob.Status.RUNNING, + ) + + stdout = StringIO() + call_command( + "recreate_index", + "--programs", + "--restart", + stdout=stdout, + ) + + active_job.refresh_from_db() + new_job = TaskJob.objects.exclude(id=active_job.id).get() + assert active_job.status == TaskJob.Status.FAILED + assert f"superseded by reindex job {new_job.id}" == active_job.error + assert new_job.params["restart"] is True + start_recreate_index_mock.delay.assert_called_once_with(new_job.id) + + +def test_recreate_index_blocks_on_existing_reindexing_indexes( + mocker, start_recreate_index_mock +): + """The command should not start a job when reindexing indexes exist""" + mocker.patch( + "learning_resources_search.management.commands.recreate_index.get_existing_reindexing_indexes", + autospec=True, + return_value=["some_reindexing_index"], + ) + + stdout = StringIO() + call_command("recreate_index", "--programs", stdout=stdout) + + assert TaskJob.objects.count() == 0 + start_recreate_index_mock.delay.assert_not_called() + assert "some_reindexing_index" in stdout.getvalue() + + +def test_recreate_index_requires_index_selection(start_recreate_index_mock): + """The command should print valid options when no index is selected""" + stdout = StringIO() + call_command("recreate_index", stdout=stdout) + + assert TaskJob.objects.count() == 0 + start_recreate_index_mock.delay.assert_not_called() + assert "Must select at least one index to update" in stdout.getvalue() + + +def test_recreate_index_status(start_recreate_index_mock): + """--status should print the job status without starting anything""" + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, + params={"indexes": ["course"], "backing_indexes": {"course": "backing"}}, + status=TaskJob.Status.RUNNING, + ) + TaskBatchFactory.create( + job=job, kind="learning_resources", status=TaskBatch.Status.SUCCEEDED + ) + TaskBatchFactory.create( + job=job, kind="learning_resources", status=TaskBatch.Status.QUEUED + ) + + stdout = StringIO() + call_command("recreate_index", "--status", str(job.id), stdout=stdout) + + output = stdout.getvalue() + assert f"Reindex job {job.id}: running" in output + assert "learning_resources" in output + assert "'succeeded': 1" in output + assert "'queued': 1" in output + start_recreate_index_mock.delay.assert_not_called() + + +def test_recreate_index_status_latest(start_recreate_index_mock): + """--status with no id should print the most recent job""" + TaskJobFactory.create(task_name=REINDEX_TASK_NAME, params={"indexes": ["course"]}) + latest_job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, + params={"indexes": ["program"]}, + status=TaskJob.Status.SUCCEEDED, + ) + + stdout = StringIO() + call_command("recreate_index", "--status", stdout=stdout) + + assert f"Reindex job {latest_job.id}: succeeded" in stdout.getvalue() + start_recreate_index_mock.delay.assert_not_called() + + +def test_recreate_index_status_no_jobs(start_recreate_index_mock): + """--status should handle there being no jobs at all""" + stdout = StringIO() + call_command("recreate_index", "--status", stdout=stdout) + + assert "No reindex job found" in stdout.getvalue() + start_recreate_index_mock.delay.assert_not_called() diff --git a/learning_resources_search/tasks.py b/learning_resources_search/tasks.py index eb817d2bdd..fa9cd2a2c2 100644 --- a/learning_resources_search/tasks.py +++ b/learning_resources_search/tasks.py @@ -5,11 +5,13 @@ import logging from collections import OrderedDict from contextlib import contextmanager +from http import HTTPStatus from itertools import groupby from random import random from urllib.parse import urlencode import celery +import requests from celery.exceptions import Ignore from django.conf import settings from django.contrib.auth import get_user_model @@ -41,8 +43,10 @@ LEARNING_RESOURCE_TYPES, PERCOLATE_INDEX_TYPE, PROGRAM_TYPE, + REINDEX_TASK_NAME, SEARCH_CONN_EXCEPTIONS, IndexestoUpdate, + ReindexBatchKind, ) from learning_resources_search.exceptions import ReindexError, RetryError from learning_resources_search.models import PercolateQuery @@ -53,11 +57,12 @@ serialize_percolate_query_for_update, ) from main.celery import app +from main.models import TaskBatch, TaskJob +from main.tasks import maybe_finish_task_job from main.utils import ( chunks, clear_views_cache, frontend_absolute_url, - merge_strings, now_in_utc, ) from profiles.utils import send_template_email @@ -65,6 +70,9 @@ User = get_user_model() log = logging.getLogger(__name__) +# Timeout for the digest email's image liveness check +IMAGE_CHECK_TIMEOUT_SECONDS = 5 + # For our tasks that attempt to partially update a document, there's a chance that # the document has not yet been created. When we get an error that indicates that the @@ -217,6 +225,46 @@ def key_func(x): return grouped_data +def _image_url_is_reachable(url): + """ + Check whether an image URL responds successfully. + + Uses a short timeout: this runs while building a digest email, so a slow + or hanging image host should not hold up the send. + """ + try: + response = requests.head( + url, timeout=IMAGE_CHECK_TIMEOUT_SECONDS, allow_redirects=True + ) + if response.status_code in ( + HTTPStatus.METHOD_NOT_ALLOWED, + HTTPStatus.NOT_IMPLEMENTED, + ): + # some servers reject HEAD; retry without downloading the body + response = requests.get( + url, timeout=IMAGE_CHECK_TIMEOUT_SECONDS, stream=True + ) + response.close() + except requests.RequestException: + return False + return HTTPStatus.OK <= response.status_code < HTTPStatus.MULTIPLE_CHOICES + + +def _validated_resource_image_url(resource): + """ + Return the resource's image URL if it is reachable, otherwise the default + resource image. Email clients can't fall back on their own, so a dead URL + would render as a broken image icon. + """ + if ( + resource.image + and resource.image.url + and _image_url_is_reachable(resource.image.url) + ): + return resource.image.url + return frontend_absolute_url("/images/default_resource.jpg") + + def _get_percolated_rows(resources, subscription_type): """ Get percolated rows for a list of learning resources and subscription type @@ -229,6 +277,7 @@ def _get_percolated_rows(resources, subscription_type): source_type=subscription_type ) if percolated.count() > 0: + resource_image_url = _validated_resource_image_url(resource) percolated_users = set(percolated.values_list("users", flat=True)) all_users.update(percolated_users) for user in percolated_users: @@ -248,9 +297,7 @@ def _get_percolated_rows(resources, subscription_type): { "resource_url": resource_url, "resource_title": resource.title, - "resource_image_url": resource.image.url - if resource.image - else frontend_absolute_url("/images/default_resource.jpg"), + "resource_image_url": resource_image_url, "resource_type": LearningResourceType[ resource.resource_type ].value, @@ -568,200 +615,419 @@ def wrap_retry_exception(*exception_classes): raise -@app.task(bind=True) -def start_recreate_index(self, indexes, remove_existing_reindexing_tags): # noqa: C901 +def _build_reindex_batches(job): # noqa: C901, PLR0912 """ - Wipe and recreate index and mapping, and index all items. - """ - try: - if not remove_existing_reindexing_tags: - existing_reindexing_indexes = api.get_existing_reindexing_indexes(indexes) + Build the TaskBatch rows for a reindex job using fast id-only queries. - if existing_reindexing_indexes: - error = ( - f"Reindexing in progress. Reindexing indexes already exist: " - f"{', '.join(existing_reindexing_indexes)}" - ) - log.exception(error) - return error + Content files are not enumerated here; dispatch batches defer the slow + per-resource ContentFile queries to run_reindex_batch workers. - api.delete_orphaned_indexes( - indexes, delete_reindexing_tags=remove_existing_reindexing_tags - ) + Args: + job (TaskJob): the reindex job - new_backing_indices = { - obj_type: api.create_backing_index(obj_type) for obj_type in indexes - } + Returns: + list of TaskBatch: unsaved batch rows + """ + indexes = job.params["indexes"] + batches = [] - # Do the indexing on the temp index - log.info("starting to index %s objects...", ", ".join(indexes)) + def add_batch(kind, batch_key, params): + batches.append( + TaskBatch(job=job, kind=kind.value, batch_key=batch_key, params=params) + ) - index_tasks = [] + if PERCOLATE_INDEX_TYPE in indexes: + for chunk, ids in enumerate( + chunks( + PercolateQuery.objects.order_by("id").values_list("id", flat=True), + chunk_size=settings.OPENSEARCH_INDEXING_CHUNK_SIZE, + ) + ): + add_batch(ReindexBatchKind.percolate, f"percolate:{chunk}", {"ids": ids}) - if PERCOLATE_INDEX_TYPE in indexes: - index_tasks = index_tasks + [ - bulk_index_percolate_queries.si( - percolate_ids, IndexestoUpdate.reindexing_index.value - ) - for percolate_ids in chunks( - PercolateQuery.objects.order_by("id").values_list("id", flat=True), - chunk_size=settings.OPENSEARCH_INDEXING_CHUNK_SIZE, - ) - ] + if COURSE_TYPE in indexes or HYBRID_COMBINED_INDEX in indexes: + blocklisted_ids = load_course_blocklist() - if COURSE_TYPE in indexes: - blocklisted_ids = load_course_blocklist() - index_tasks = index_tasks + [ - index_learning_resources.si( - ids, - COURSE_TYPE, - index_types=IndexestoUpdate.reindexing_index.value, - ) - for ids in chunks( - Course.objects.filter(learning_resource__published=True) - .exclude(learning_resource__readable_id__in=blocklisted_ids) - .order_by("learning_resource_id") - .values_list("learning_resource_id", flat=True), - chunk_size=settings.OPENSEARCH_INDEXING_CHUNK_SIZE, - ) - ] + if COURSE_TYPE in indexes: + for chunk, ids in enumerate( + chunks( + Course.objects.filter(learning_resource__published=True) + .exclude(learning_resource__readable_id__in=blocklisted_ids) + .order_by("learning_resource_id") + .values_list("learning_resource_id", flat=True), + chunk_size=settings.OPENSEARCH_INDEXING_CHUNK_SIZE, + ) + ): + add_batch( + ReindexBatchKind.learning_resources, + f"{COURSE_TYPE}:resources:{chunk}", + {"ids": ids, "index_name": COURSE_TYPE}, + ) - for course in ( + for chunk, resource_ids in enumerate( + chunks( Course.objects.filter(learning_resource__published=True) .filter(learning_resource__etl_source__in=RESOURCE_FILE_ETL_SOURCES) .exclude(learning_resource__readable_id__in=blocklisted_ids) .order_by("learning_resource_id") - ): - index_tasks = ( - index_tasks - + [ - index_content_files.si( - ids, - course.learning_resource_id, - index_types=IndexestoUpdate.reindexing_index.value, - ) - for ids in chunks( - ContentFile.objects.filter( - run__learning_resource_id=course.learning_resource_id, - published=True, - run__published=True, - ) - .order_by("id") - .values_list("id", flat=True), - chunk_size=settings.OPENSEARCH_DOCUMENT_INDEXING_CHUNK_SIZE, - ) - ] - + [ - index_content_files.si( - ids, - course.learning_resource_id, - index_types=IndexestoUpdate.reindexing_index.value, - ) - for ids in chunks( - ContentFile.objects.filter( - learning_resource_id=course.learning_resource_id, - published=True, - ) - .order_by("id") - .values_list("id", flat=True), - chunk_size=settings.OPENSEARCH_DOCUMENT_INDEXING_CHUNK_SIZE, - ) - ] - ) + .values_list("learning_resource_id", flat=True), + chunk_size=settings.OPENSEARCH_REINDEX_DISPATCH_CHUNK_SIZE, + ) + ): + add_batch( + ReindexBatchKind.dispatch_content_files, + f"dispatch:{COURSE_TYPE}:{chunk}", + { + "learning_resource_ids": resource_ids, + "resource_type": COURSE_TYPE, + }, + ) - if HYBRID_COMBINED_INDEX in indexes: - blocklisted_ids = load_course_blocklist() + if HYBRID_COMBINED_INDEX in indexes: + for chunk, ids in enumerate( + chunks( + LearningResource.objects.filter(published=True) + .exclude(readable_id__in=blocklisted_ids) + .order_by("id") + .values_list("id", flat=True), + chunk_size=settings.OPENSEARCH_INDEXING_CHUNK_SIZE, + ) + ): + add_batch( + ReindexBatchKind.learning_resources, + f"{HYBRID_COMBINED_INDEX}:resources:{chunk}", + {"ids": ids, "index_name": HYBRID_COMBINED_INDEX}, + ) - index_tasks = index_tasks + [ - index_learning_resources.si( - ids, - HYBRID_COMBINED_INDEX, - index_types=IndexestoUpdate.reindexing_index.value, - ) - for ids in chunks( + for resource_type in set(LEARNING_RESOURCE_TYPES) - {COURSE_TYPE}: + if resource_type in indexes: + for chunk, ids in enumerate( + chunks( LearningResource.objects.filter( published=True, + resource_type=resource_type, ) - .exclude(readable_id__in=blocklisted_ids) .order_by("id") .values_list("id", flat=True), chunk_size=settings.OPENSEARCH_INDEXING_CHUNK_SIZE, ) - ] + ): + add_batch( + ReindexBatchKind.learning_resources, + f"{resource_type}:resources:{chunk}", + {"ids": ids, "index_name": resource_type}, + ) - for resource_type in set(LEARNING_RESOURCE_TYPES) - {COURSE_TYPE}: - if resource_type in indexes: - index_tasks = index_tasks + [ - index_learning_resources.si( - ids, - resource_type, - index_types=IndexestoUpdate.reindexing_index.value, - ) - for ids in chunks( - LearningResource.objects.filter( - published=True, - resource_type=resource_type, - ) - .order_by("id") - .values_list("id", flat=True), - chunk_size=settings.OPENSEARCH_INDEXING_CHUNK_SIZE, - ) - ] + if PROGRAM_TYPE in indexes: + for chunk, resource_ids in enumerate( + chunks( + LearningResource.objects.filter( + published=True, resource_type=PROGRAM_TYPE + ) + .order_by("id") + .values_list("id", flat=True), + chunk_size=settings.OPENSEARCH_REINDEX_DISPATCH_CHUNK_SIZE, + ) + ): + add_batch( + ReindexBatchKind.dispatch_content_files, + f"dispatch:{PROGRAM_TYPE}:{chunk}", + { + "learning_resource_ids": resource_ids, + "resource_type": PROGRAM_TYPE, + }, + ) - if PROGRAM_TYPE in indexes: - for program_resource in LearningResource.objects.filter( - published=True, resource_type=PROGRAM_TYPE - ).order_by("id"): - index_tasks = ( - index_tasks - + [ - index_content_files.si( - ids, - program_resource.id, - index_types=IndexestoUpdate.reindexing_index.value, - resource_type=PROGRAM_TYPE, - ) - for ids in chunks( - ContentFile.objects.filter( - run__learning_resource_id=program_resource.id, - published=True, - run__published=True, - ) - .order_by("id") - .values_list("id", flat=True), - chunk_size=settings.OPENSEARCH_DOCUMENT_INDEXING_CHUNK_SIZE, - ) + return batches + + +def _dispatch_content_file_batches(batch): + """ + Create and enqueue the content file batches for a dispatch batch. + + Child rows are created (idempotently, via the unique batch_key) before the + dispatch batch itself is marked complete, so the job can never appear + finished while content file fan-out is still pending. + + Args: + batch (TaskBatch): a dispatch_content_files batch + """ + resource_type = batch.params["resource_type"] + children = [] + for resource_id in batch.params["learning_resource_ids"]: + for chunk, ids in enumerate( + chunks( + ContentFile.objects.filter( + run__learning_resource_id=resource_id, + published=True, + run__published=True, + ) + .order_by("id") + .values_list("id", flat=True), + chunk_size=settings.OPENSEARCH_DOCUMENT_INDEXING_CHUNK_SIZE, + ) + ): + children.append( + TaskBatch( + job=batch.job, + kind=ReindexBatchKind.content_files.value, + batch_key=f"content_files:{resource_id}:run:{chunk}", + params={ + "ids": ids, + "learning_resource_id": resource_id, + "resource_type": resource_type, + }, + ) + ) + for chunk, ids in enumerate( + chunks( + ContentFile.objects.filter( + learning_resource_id=resource_id, + published=True, + ) + .order_by("id") + .values_list("id", flat=True), + chunk_size=settings.OPENSEARCH_DOCUMENT_INDEXING_CHUNK_SIZE, + ) + ): + children.append( + TaskBatch( + job=batch.job, + kind=ReindexBatchKind.content_files.value, + batch_key=f"content_files:{resource_id}:direct:{chunk}", + params={ + "ids": ids, + "learning_resource_id": resource_id, + "resource_type": resource_type, + }, + ) + ) + TaskBatch.objects.bulk_create(children, ignore_conflicts=True) + child_ids = batch.job.batches.filter( + batch_key__in=[child.batch_key for child in children], + status=TaskBatch.Status.QUEUED, + ).values_list("id", flat=True) + for child_id in child_ids: + run_reindex_batch.delay(child_id) + + +def _execute_reindex_batch(batch): + """ + Run the indexing work for a single reindex batch + + Args: + batch (TaskBatch): the batch to execute + """ + params = batch.params + if batch.kind == ReindexBatchKind.learning_resources.value: + api.index_learning_resources( + params["ids"], + params["index_name"], + IndexestoUpdate.reindexing_index.value, + ) + elif batch.kind == ReindexBatchKind.content_files.value: + api.index_content_files( + params["ids"], + params["learning_resource_id"], + index_types=IndexestoUpdate.reindexing_index.value, + resource_type=params["resource_type"], + ) + elif batch.kind == ReindexBatchKind.percolate.value: + api.index_items( + serialize_bulk_percolators(params["ids"]), + PERCOLATE_INDEX_TYPE, + IndexestoUpdate.reindexing_index.value, + ) + elif batch.kind == ReindexBatchKind.dispatch_content_files.value: + _dispatch_content_file_batches(batch) + + +def _maybe_finish_reindex_job(job_id): + """ + Claim and enqueue finish_reindex_job if every batch of the job is done + + Args: + job_id (int): TaskJob id + """ + maybe_finish_task_job(job_id, finish_reindex_job) + + +class _RunReindexBatchTask(app.Task): + """ + Base task that fails the batch if run_reindex_batch gives up. + + When autoretries are exhausted the task raises without having marked the + batch terminal, which would leave it RUNNING and hang the job. on_failure + fires once, on the final give-up, so we mark the batch FAILED and nudge + completion. (A worker killed mid-run does not trigger this — that message + is redelivered by acks_late instead.) + """ + + def on_failure(self, exc, task_id, args, kwargs, einfo): # noqa: ARG002 + batch_id = args[0] if args else kwargs.get("batch_id") + batch = TaskBatch.objects.filter(id=batch_id).first() + if batch is None: + return + TaskBatch.objects.filter( + id=batch_id, status__in=TaskBatch.NON_TERMINAL_STATUSES + ).update( + status=TaskBatch.Status.FAILED, + error=f"run_reindex_batch gave up: {exc}", + ) + _maybe_finish_reindex_job(batch.job_id) + + +@app.task( + base=_RunReindexBatchTask, + acks_late=True, + reject_on_worker_lost=True, + autoretry_for=(RetryError,), + retry_backoff=True, + rate_limit=settings.CELERY_SEARCH_RATE_LIMIT, +) +def run_reindex_batch(batch_id): + """ + Execute one reindex batch and record its completion in the database + + Args: + batch_id (int): TaskBatch id + """ + batch = TaskBatch.objects.select_related("job").get(id=batch_id) + if ( + batch.status not in TaskBatch.NON_TERMINAL_STATUSES + or batch.job.status not in TaskJob.ACTIVE_STATUSES + ): + log.info( + "Skipping reindex batch %s (batch status=%s, job status=%s)", + batch.batch_key, + batch.status, + batch.job.status, + ) + # a redelivery of an already-finished batch still nudges completion, so + # the job can't hang if the last batch's worker was culled right after + # committing its status but before the finish step was enqueued + _maybe_finish_reindex_job(batch.job_id) + return + # mark the batch running; a redelivered message may find the batch already + # running, which is fine — execution is idempotent + TaskBatch.objects.filter( + id=batch_id, status__in=TaskBatch.NON_TERMINAL_STATUSES + ).update(status=TaskBatch.Status.RUNNING) + try: + with wrap_retry_exception(*SEARCH_CONN_EXCEPTIONS): + _execute_reindex_batch(batch) + except (RetryError, Ignore): + raise + except SystemExit as err: + raise RetryError(SystemExit.__name__) from err + except Exception as ex: + error = f"run_reindex_batch threw an error: {type(ex).__name__}: {ex}" + log.exception("Reindex batch %s failed", batch.batch_key) + TaskBatch.objects.filter( + id=batch_id, status__in=TaskBatch.NON_TERMINAL_STATUSES + ).update(status=TaskBatch.Status.FAILED, error=error) + else: + TaskBatch.objects.filter( + id=batch_id, status__in=TaskBatch.NON_TERMINAL_STATUSES + ).update(status=TaskBatch.Status.SUCCEEDED) + _maybe_finish_reindex_job(batch.job_id) + + +@app.task(acks_late=True, reject_on_worker_lost=True) +def start_recreate_index(job_id): + """ + Create backing indexes for a reindex job and fan out indexing batches. + + All indexing writes go only to the new (reindexing) backing indexes; + search keeps using the current default indexes until finish_reindex_job + switches the aliases after every batch has succeeded. + + Args: + job_id (int): TaskJob id + """ + job = TaskJob.objects.get(id=job_id) + # QUEUED: fresh job, do one-time setup. RUNNING: a redelivery (the worker + # died partway through the enqueue loop) — skip setup and just re-enqueue + # whatever batches are still waiting. Anything else is already done. + if job.status not in (TaskJob.Status.QUEUED, TaskJob.Status.RUNNING): + log.info("Reindex job %s not startable (status=%s)", job_id, job.status) + return + indexes = job.params["indexes"] + restart = job.params.get("restart", False) + + if job.status == TaskJob.Status.QUEUED: + try: + error = None + if not restart: + existing_reindexing_indexes = api.get_existing_reindexing_indexes( + indexes + ) + if existing_reindexing_indexes: + error = ( + f"Reindexing in progress. Reindexing indexes already exist: " + f"{', '.join(existing_reindexing_indexes)}" + ) + else: + other_active_jobs = [ + other_job + for other_job in TaskJob.objects.filter( + task_name=REINDEX_TASK_NAME, + status__in=TaskJob.ACTIVE_STATUSES, + ).exclude(id=job_id) + if set(other_job.params.get("indexes", [])) & set(indexes) ] - + [ - index_content_files.si( - ids, - program_resource.id, - index_types=IndexestoUpdate.reindexing_index.value, - resource_type=PROGRAM_TYPE, - ) - for ids in chunks( - ContentFile.objects.filter( - learning_resource_id=program_resource.id, - published=True, - ) - .order_by("id") - .values_list("id", flat=True), - chunk_size=settings.OPENSEARCH_DOCUMENT_INDEXING_CHUNK_SIZE, + if other_active_jobs: + error = ( + f"Reindexing in progress. Active reindex jobs already" + f" exist: " + f"{', '.join(str(other.id) for other in other_active_jobs)}" ) - ] + if error: + log.error(error) + TaskJob.objects.filter(id=job_id).update( + status=TaskJob.Status.FAILED, error=error ) + return - index_tasks = celery.group(index_tasks) - except: # noqa: E722 - error = "start_recreate_index threw an error" - log.exception(error) - return error + api.delete_orphaned_indexes(indexes, delete_reindexing_tags=restart) - # Use self.replace so that code waiting on this task will also wait on the indexing - # and finish tasks - return self.replace( - celery.chain(index_tasks, finish_recreate_index.s(new_backing_indices)) - ) + job.params["backing_indexes"] = { + obj_type: api.create_backing_index(obj_type) for obj_type in indexes + } + job.save() + + log.info("starting to index %s objects...", ", ".join(indexes)) + + TaskBatch.objects.bulk_create( + _build_reindex_batches(job), ignore_conflicts=True + ) + # flip to RUNNING before enqueuing so a redelivery after this point + # resumes the enqueue loop rather than redoing setup + TaskJob.objects.filter(id=job_id, status=TaskJob.Status.QUEUED).update( + status=TaskJob.Status.RUNNING + ) + except Exception: + error = "start_recreate_index threw an error" + log.exception(error) + TaskJob.objects.filter(id=job_id).update( + status=TaskJob.Status.FAILED, error=error + ) + try: + api.delete_orphaned_indexes(indexes, delete_reindexing_tags=True) + except Exception: + log.exception( + "Failed to clean up reindexing indexes for job %s", job_id + ) + return + + # (re)enqueue every batch still waiting; idempotent under redelivery, so a + # worker death mid-loop can't strand batches in QUEUED + for batch_id in job.batches.filter(status=TaskBatch.Status.QUEUED).values_list( + "id", flat=True + ): + run_reindex_batch.delay(batch_id) + # handles the edge case of a job with no batches at all + _maybe_finish_reindex_job(job_id) @app.task( @@ -1111,40 +1377,81 @@ def get_update_learning_resource_tasks(resource_type): ] +class _FinishReindexJobTask(app.Task): + """ + Base task that fails the job if finish_reindex_job gives up. + + If the alias switch keeps erroring until autoretries are exhausted, the job + would otherwise hang in FINISHING. on_failure marks it FAILED so it doesn't + stay active forever; the old index keeps serving and an operator can re-run. + """ + + def on_failure(self, exc, task_id, args, kwargs, einfo): # noqa: ARG002 + job_id = args[0] if args else kwargs.get("job_id") + TaskJob.objects.filter(id=job_id, status=TaskJob.Status.FINISHING).update( + status=TaskJob.Status.FAILED, + error=f"finish_reindex_job gave up: {exc}", + ) + + @app.task( + base=_FinishReindexJobTask, acks_late=True, reject_on_worker_lost=True, autoretry_for=(RetryError, SystemExit), retry_backoff=True, rate_limit=settings.CELERY_SEARCH_RATE_LIMIT, ) -def finish_recreate_index(results, backing_indices): +def finish_reindex_job(job_id): """ - Swap reindex backing index with default backing index + Swap the reindex backing indexes with the default backing indexes once + every batch of the job has succeeded, or clean up if any batch failed. + + Safe to re-run: already-switched object types are skipped, so a redelivery + can never delete the newly promoted backing index. Args: - results (list or bool): Results saying whether the error exists - backing_indices (dict): The backing OpenSearch indices keyed by object type + job_id (int): TaskJob id """ - errors = merge_strings(results) + job = TaskJob.objects.get(id=job_id) + if job.status != TaskJob.Status.FINISHING: + log.info("Skipping finish for reindex job %s (status=%s)", job_id, job.status) + return + + backing_indexes = job.params.get("backing_indexes", {}) + errors = [ + f"{batch_key}: {error}" + for batch_key, error in job.batches.filter( + status=TaskBatch.Status.FAILED + ).values_list("batch_key", "error") + ] if errors: try: api.delete_orphaned_indexes( - list(backing_indices.keys()), delete_reindexing_tags=True + list(backing_indexes.keys()), delete_reindexing_tags=True ) except RequestError as ex: raise RetryError(str(ex)) from ex msg = f"Errors occurred during recreate_index: {errors}" + TaskJob.objects.filter(id=job_id, status=TaskJob.Status.FINISHING).update( + status=TaskJob.Status.FAILED, error=msg + ) raise ReindexError(msg) log.info( "Done with temporary index. Pointing default aliases to newly created backing indexes..." # noqa: E501 ) - for obj_type, backing_index in backing_indices.items(): + for obj_type, backing_index in backing_indexes.items(): try: + if api.is_default_backing_index(backing_index, obj_type): + # already switched by a previous delivery of this task + continue api.switch_indices(backing_index, obj_type) except RequestError as ex: raise RetryError(str(ex)) from ex + TaskJob.objects.filter(id=job_id, status=TaskJob.Status.FINISHING).update( + status=TaskJob.Status.SUCCEEDED + ) log.info("recreate_index has finished successfully!") clear_views_cache() diff --git a/learning_resources_search/tasks_test.py b/learning_resources_search/tasks_test.py index e8f3a67730..b63575de7a 100644 --- a/learning_resources_search/tasks_test.py +++ b/learning_resources_search/tasks_test.py @@ -3,6 +3,8 @@ from collections import OrderedDict import pytest +import requests +import responses from celery.exceptions import Ignore, Retry from django.conf import settings from django.contrib.auth import get_user_model @@ -27,8 +29,11 @@ COURSE_TYPE, HYBRID_COMBINED_INDEX, LEARNING_RESOURCE_TYPES, + PERCOLATE_INDEX_TYPE, PROGRAM_TYPE, + REINDEX_TASK_NAME, IndexestoUpdate, + ReindexBatchKind, ) from learning_resources_search.exceptions import ReindexError, RetryError from learning_resources_search.factories import PercolateQueryFactory @@ -41,13 +46,17 @@ _generate_subscription_digest_subject, _get_percolated_rows, _group_percolated_rows, + _image_url_is_reachable, _infer_percolate_group, + _maybe_finish_reindex_job, + _validated_resource_image_url, bulk_deindex_learning_resources, deindex_document, deindex_run_content_files, - finish_recreate_index, + finish_reindex_job, index_learning_resources, index_run_content_files, + run_reindex_batch, send_subscription_emails, start_recreate_index, start_update_index, @@ -56,8 +65,10 @@ upsert_learning_resource, wrap_retry_exception, ) -from main.factories import UserFactory +from main.factories import TaskBatchFactory, TaskJobFactory, UserFactory +from main.models import TaskBatch, TaskJob from main.test_utils import assert_not_raises +from main.utils import frontend_absolute_url pytestmark = pytest.mark.django_db User = get_user_model() @@ -80,6 +91,19 @@ def mocked_api(mocker): return mocker.patch("learning_resources_search.tasks.api") +@pytest.fixture(autouse=True) +def mock_image_url_is_reachable(mocker, request): + """ + Stub the digest email's image reachability check so tests don't make real + requests. Tests that use mocked_responses exercise the real function. + """ + if "mocked_responses" in request.fixturenames: + return None + return mocker.patch( + "learning_resources_search.tasks._image_url_is_reachable", return_value=True + ) + + def test_upsert_learning_resource(mocked_api): """Test that upsert_learning_resourc will serialize the learning resource data and upsert it to the OS index""" resource = LearningResourceFactory.create() @@ -140,12 +164,13 @@ def test_system_exit_retry(mocker): ["combined_hybrid"], ], ) -def test_start_recreate_index(mocker, mocked_celery, user, indexes): # noqa: C901, PLR0915 +def test_start_recreate_index(mocker, indexes): # noqa: C901, PLR0912, PLR0915 """ - recreate_index should recreate the OpenSearch index and reindex all data with it + recreate_index should create backing indexes and batch rows for all data """ settings.OPENSEARCH_INDEXING_CHUNK_SIZE = 2 settings.OPENSEARCH_DOCUMENT_INDEXING_CHUNK_SIZE = 2 + settings.OPENSEARCH_REINDEX_DISPATCH_CHUNK_SIZE = 2 mock_blocklist = mocker.patch( "learning_resources_search.tasks.load_course_blocklist", return_value=[] @@ -159,12 +184,6 @@ def test_start_recreate_index(mocker, mocked_celery, user, indexes): # noqa: C9 for course in ocw_courses: ContentFileFactory.create_batch(3, run=course.learning_resource.runs.first()) - # A resource-level (marketing page) content file attached directly to the - # learning resource rather than a run. - course_marketing_file = ContentFileFactory.create( - learning_resource=ocw_courses[0].learning_resource - ) - oll_courses = CourseFactory.create_batch(2, etl_source=ETLSource.ocw.value) courses = sorted( @@ -180,21 +199,8 @@ def test_start_recreate_index(mocker, mocked_celery, user, indexes): # noqa: C9 key=lambda program: program.learning_resource_id, ) - # Attach both a run-level and a resource-level (marketing page) content file - # to one program to exercise program content file indexing. - program_with_files = programs[0] - program_run_file = ContentFileFactory.create( - run=program_with_files.learning_resource.runs.first() - ) - program_marketing_file = ContentFileFactory.create( - learning_resource=program_with_files.learning_resource - ) - - index_learning_resources_mock = mocker.patch( - "learning_resources_search.tasks.index_learning_resources", autospec=True - ) - index_files_mock = mocker.patch( - "learning_resources_search.tasks.index_content_files", autospec=True + run_reindex_batch_mock = mocker.patch( + "learning_resources_search.tasks.run_reindex_batch", autospec=True ) backing_index = "backing" @@ -211,123 +217,98 @@ def test_start_recreate_index(mocker, mocked_celery, user, indexes): # noqa: C9 delete_orphaned_indexes_mock = mocker.patch( "learning_resources_search.indexing_api.delete_orphaned_indexes", autospec=True ) - finish_recreate_index_mock = mocker.patch( - "learning_resources_search.tasks.finish_recreate_index", autospec=True - ) - finish_recreate_index_dict = {} + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, params={"indexes": indexes} + ) + start_recreate_index.delay(job.id) - with pytest.raises(mocked_celery.replace_exception_class): - start_recreate_index.delay(indexes, remove_existing_reindexing_tags=False) + job.refresh_from_db() + assert job.status == TaskJob.Status.RUNNING + assert job.error == "" for doctype in [COURSE_TYPE, PROGRAM_TYPE, HYBRID_COMBINED_INDEX]: if doctype in indexes: - finish_recreate_index_dict[doctype] = backing_index + assert job.params["backing_indexes"][doctype] == backing_index create_backing_index_mock.assert_any_call(doctype) - finish_recreate_index_mock.s.assert_called_once_with(finish_recreate_index_dict) - delete_orphaned_indexes_mock.assert_called_once_with( indexes, delete_reindexing_tags=False ) - assert mocked_celery.group.call_count == 1 + resource_batches = job.batches.filter( + kind=ReindexBatchKind.learning_resources.value + ).order_by("id") + dispatch_batches = job.batches.filter( + kind=ReindexBatchKind.dispatch_content_files.value + ).order_by("id") - # Celery's 'group' function takes a generator as an argument. In order to make assertions about the items - # in that generator, 'list' is being called to force iteration through all of those items. - list(mocked_celery.group.call_args[0][0]) + # no content file batches until dispatch batches run + assert not job.batches.filter(kind=ReindexBatchKind.content_files.value).exists() if COURSE_TYPE in indexes: - assert index_learning_resources_mock.si.call_count == 3 + assert resource_batches.count() == 3 if PROGRAM_TYPE in indexes: - assert index_learning_resources_mock.si.call_count == 2 + assert resource_batches.count() == 2 if HYBRID_COMBINED_INDEX in indexes: - assert index_learning_resources_mock.si.call_count == 5 + assert resource_batches.count() == 5 + + resource_id_chunks = [batch.params["ids"] for batch in resource_batches] + for batch in resource_batches: + assert batch.params["index_name"] == indexes[0] if COURSE_TYPE in indexes or HYBRID_COMBINED_INDEX in indexes: mock_blocklist.assert_called_once() - index_type = indexes[0] - index_learning_resources_mock.si.assert_any_call( + for chunk in ( [courses[0].learning_resource_id, courses[1].learning_resource_id], - index_type, - index_types=IndexestoUpdate.reindexing_index.value, - ) - index_learning_resources_mock.si.assert_any_call( [courses[2].learning_resource_id, courses[3].learning_resource_id], - index_type, - index_types=IndexestoUpdate.reindexing_index.value, - ) - index_learning_resources_mock.si.assert_any_call( [courses[4].learning_resource_id, courses[5].learning_resource_id], - index_type, - index_types=IndexestoUpdate.reindexing_index.value, - ) - - if COURSE_TYPE in indexes: - for course in ocw_courses: - content_file_ids = ( - course.learning_resource.runs.first() - .content_files.order_by("id") - .values_list("id", flat=True) - ) - index_files_mock.si.assert_any_call( - [content_file_ids[0], content_file_ids[1]], - course.learning_resource_id, - index_types=IndexestoUpdate.reindexing_index.value, - ) - - index_files_mock.si.assert_any_call( - [content_file_ids[2]], - course.learning_resource_id, - index_types=IndexestoUpdate.reindexing_index.value, - ) - - # resource-level (marketing page) content file attached directly to the - # learning resource - index_files_mock.si.assert_any_call( - [course_marketing_file.id], - ocw_courses[0].learning_resource_id, - index_types=IndexestoUpdate.reindexing_index.value, - ) - - if PROGRAM_TYPE in indexes: - # Program content files are indexed with resource_type=PROGRAM_TYPE, for - # both run-level and resource-level (marketing page) content files. - index_files_mock.si.assert_any_call( - [program_run_file.id], - program_with_files.learning_resource_id, - index_types=IndexestoUpdate.reindexing_index.value, - resource_type=PROGRAM_TYPE, - ) - index_files_mock.si.assert_any_call( - [program_marketing_file.id], - program_with_files.learning_resource_id, - index_types=IndexestoUpdate.reindexing_index.value, - resource_type=PROGRAM_TYPE, - ) + ): + assert chunk in resource_id_chunks if PROGRAM_TYPE in indexes or HYBRID_COMBINED_INDEX in indexes: - index_type = indexes[0] - index_learning_resources_mock.si.assert_any_call( + for chunk in ( [programs[0].learning_resource_id, programs[1].learning_resource_id], - index_type, - index_types=IndexestoUpdate.reindexing_index.value, - ) - index_learning_resources_mock.si.assert_any_call( [programs[2].learning_resource_id, programs[3].learning_resource_id], - index_type, - index_types=IndexestoUpdate.reindexing_index.value, - ) - assert mocked_celery.replace.call_count == 1 - assert mocked_celery.replace.call_args[0][1] == mocked_celery.chain.return_value + ): + assert chunk in resource_id_chunks + + if COURSE_TYPE in indexes: + # all 6 courses are resource-file (ocw) courses, dispatched in chunks of 2 + assert dispatch_batches.count() == 3 + dispatched_ids = [ + resource_id + for batch in dispatch_batches + for resource_id in batch.params["learning_resource_ids"] + ] + assert dispatched_ids == [course.learning_resource_id for course in courses] + for batch in dispatch_batches: + assert batch.params["resource_type"] == COURSE_TYPE + elif PROGRAM_TYPE in indexes: + assert dispatch_batches.count() == 2 + dispatched_ids = [ + resource_id + for batch in dispatch_batches + for resource_id in batch.params["learning_resource_ids"] + ] + assert dispatched_ids == [program.learning_resource_id for program in programs] + for batch in dispatch_batches: + assert batch.params["resource_type"] == PROGRAM_TYPE + else: + assert dispatch_batches.count() == 0 + + # every batch was enqueued + assert run_reindex_batch_mock.delay.call_count == job.batches.count() + enqueued_ids = { + call.args[0] for call in run_reindex_batch_mock.delay.call_args_list + } + assert enqueued_ids == set(job.batches.values_list("id", flat=True)) @pytest.mark.parametrize("indexes", [["course"], ["combined_hybrid"]]) -def test_start_recreate_index_excludes_blocklisted_courses( - mocker, mocked_celery, indexes -): +def test_start_recreate_index_excludes_blocklisted_courses(mocker, indexes): """start_recreate_index should not index courses whose readable_id is blocklisted""" courses = CourseFactory.create_batch(3, etl_source=ETLSource.ocw.value) blocked = courses[0] @@ -337,54 +318,115 @@ def test_start_recreate_index_excludes_blocklisted_courses( "learning_resources_search.tasks.load_course_blocklist", return_value=[blocked.learning_resource.readable_id], ) - index_learning_resources_mock = mocker.patch( - "learning_resources_search.tasks.index_learning_resources", autospec=True - ) - index_files_mock = mocker.patch( - "learning_resources_search.tasks.index_content_files", autospec=True - ) + mocker.patch("learning_resources_search.tasks.run_reindex_batch", autospec=True) mocker.patch( "learning_resources_search.indexing_api.get_existing_reindexing_indexes", autospec=True, return_value=[], ) # ponytail: no assertions on these; they just keep the task off OpenSearch - for target in ( + # (create_backing_index must return a string so it can be stored in the + # job's JSON params) + mocker.patch( "learning_resources_search.indexing_api.create_backing_index", + autospec=True, + return_value="backing", + ) + mocker.patch( "learning_resources_search.indexing_api.delete_orphaned_indexes", - "learning_resources_search.tasks.finish_recreate_index", - ): - mocker.patch(target, autospec=True) + autospec=True, + ) - with pytest.raises(mocked_celery.replace_exception_class): - start_recreate_index.delay(indexes, remove_existing_reindexing_tags=False) + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, params={"indexes": indexes} + ) + start_recreate_index.delay(job.id) + + job.refresh_from_db() + assert job.status == TaskJob.Status.RUNNING indexed_resource_ids = { resource_id - for call in index_learning_resources_mock.si.call_args_list - for resource_id in call.args[0] + for batch in job.batches.filter(kind=ReindexBatchKind.learning_resources.value) + for resource_id in batch.params["ids"] } assert indexed_resource_ids == { course.learning_resource_id for course in courses[1:] } if COURSE_TYPE in indexes: - file_course_ids = {call.args[1] for call in index_files_mock.si.call_args_list} - assert file_course_ids == { - course.learning_resource_id for course in courses[1:] + dispatched_ids = { + resource_id + for batch in job.batches.filter( + kind=ReindexBatchKind.dispatch_content_files.value + ) + for resource_id in batch.params["learning_resource_ids"] } + assert dispatched_ids == {course.learning_resource_id for course in courses[1:]} + + +def test_start_recreate_index_percolate(mocker): + """start_recreate_index should chunk and enqueue percolate query batches""" + settings.OPENSEARCH_INDEXING_CHUNK_SIZE = 2 + + PercolateQuery.objects.bulk_create(PercolateQueryFactory.build_batch(5)) + percolate_queries = list(PercolateQuery.objects.order_by("id")) + + run_reindex_batch_mock = mocker.patch( + "learning_resources_search.tasks.run_reindex_batch", autospec=True + ) + mocker.patch( + "learning_resources_search.indexing_api.create_backing_index", + autospec=True, + return_value="backing", + ) + mocker.patch( + "learning_resources_search.indexing_api.get_existing_reindexing_indexes", + autospec=True, + return_value=[], + ) + mocker.patch( + "learning_resources_search.indexing_api.delete_orphaned_indexes", autospec=True + ) + + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, params={"indexes": [PERCOLATE_INDEX_TYPE]} + ) + start_recreate_index.delay(job.id) + + job.refresh_from_db() + assert job.status == TaskJob.Status.RUNNING + + percolate_batches = job.batches.filter( + kind=ReindexBatchKind.percolate.value + ).order_by("batch_key") + # 5 queries in chunks of 2 -> 3 batches, and only percolate batches + assert percolate_batches.count() == 3 + assert job.batches.count() == 3 + + id_chunks = [batch.params["ids"] for batch in percolate_batches] + assert id_chunks == [ + [percolate_queries[0].id, percolate_queries[1].id], + [percolate_queries[2].id, percolate_queries[3].id], + [percolate_queries[4].id], + ] + + # every percolate batch was enqueued + enqueued_ids = { + call.args[0] for call in run_reindex_batch_mock.delay.call_args_list + } + assert enqueued_ids == set(percolate_batches.values_list("id", flat=True)) @pytest.mark.parametrize( - "remove_existing_reindexing_tags", + "restart", [True, False], ) -def test_start_recreate_index_existing_reindexing_index( - mocker, mocked_celery, user, remove_existing_reindexing_tags -): +def test_start_recreate_index_existing_reindexing_index(mocker, restart): """start_recreate_index should stop when reindexing indexes already exist.""" settings.OPENSEARCH_INDEXING_CHUNK_SIZE = 2 settings.OPENSEARCH_DOCUMENT_INDEXING_CHUNK_SIZE = 2 + settings.OPENSEARCH_REINDEX_DISPATCH_CHUNK_SIZE = 2 indexes = ["program"] programs = sorted( @@ -392,8 +434,8 @@ def test_start_recreate_index_existing_reindexing_index( key=lambda program: program.learning_resource_id, ) - index_learning_resources_mock = mocker.patch( - "learning_resources_search.tasks.index_learning_resources", autospec=True + run_reindex_batch_mock = mocker.patch( + "learning_resources_search.tasks.run_reindex_batch", autospec=True ) backing_index = "backing" @@ -405,9 +447,6 @@ def test_start_recreate_index_existing_reindexing_index( delete_orphaned_indexes_mock = mocker.patch( "learning_resources_search.indexing_api.delete_orphaned_indexes", autospec=True ) - finish_recreate_index_mock = mocker.patch( - "learning_resources_search.tasks.finish_recreate_index", autospec=True - ) mocker.patch( "learning_resources_search.indexing_api.get_existing_reindexing_indexes", @@ -415,81 +454,222 @@ def test_start_recreate_index_existing_reindexing_index( return_value=["another_reindexing_index"], ) - if remove_existing_reindexing_tags: - with pytest.raises(mocked_celery.replace_exception_class): - start_recreate_index.delay( - indexes, remove_existing_reindexing_tags=remove_existing_reindexing_tags - ) - else: - start_recreate_index.delay( - indexes, remove_existing_reindexing_tags=remove_existing_reindexing_tags - ) + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, + params={ + "indexes": indexes, + "restart": restart, + }, + ) + start_recreate_index.delay(job.id) - finish_recreate_index_dict = {"program": "backing"} + job.refresh_from_db() - if remove_existing_reindexing_tags: + if restart: + assert job.status == TaskJob.Status.RUNNING delete_orphaned_indexes_mock.assert_called_once_with( indexes, delete_reindexing_tags=True ) + assert job.params["backing_indexes"] == {"program": "backing"} - finish_recreate_index_mock.s.assert_called_once_with(finish_recreate_index_dict) - assert mocked_celery.group.call_count == 1 - - # Celery's 'group' function takes a generator as an argument. In order to make assertions about the items - # in that generator, 'list' is being called to force iteration through all of those items. - list(mocked_celery.group.call_args[0][0]) - assert index_learning_resources_mock.si.call_count == 2 - index_learning_resources_mock.si.assert_any_call( - [programs[0].learning_resource_id, programs[1].learning_resource_id], - PROGRAM_TYPE, - index_types=IndexestoUpdate.reindexing_index.value, + resource_batches = job.batches.filter( + kind=ReindexBatchKind.learning_resources.value ) - index_learning_resources_mock.si.assert_any_call( + assert resource_batches.count() == 2 + resource_id_chunks = [batch.params["ids"] for batch in resource_batches] + for chunk in ( + [programs[0].learning_resource_id, programs[1].learning_resource_id], [programs[2].learning_resource_id, programs[3].learning_resource_id], - PROGRAM_TYPE, - index_types=IndexestoUpdate.reindexing_index.value, - ) - - assert mocked_celery.replace.call_count == 1 - assert mocked_celery.replace.call_args[0][1] == mocked_celery.chain.return_value + ): + assert chunk in resource_id_chunks + assert run_reindex_batch_mock.delay.call_count == job.batches.count() else: - assert index_learning_resources_mock.si.call_count == 0 - assert mocked_celery.replace.call_count == 0 + assert job.status == TaskJob.Status.FAILED + assert "another_reindexing_index" in job.error + assert job.batches.count() == 0 + assert run_reindex_batch_mock.delay.call_count == 0 + delete_orphaned_indexes_mock.assert_not_called() + + +def test_start_recreate_index_existing_active_job(mocker): + """start_recreate_index should stop when another active reindex job overlaps""" + mocker.patch( + "learning_resources_search.indexing_api.get_existing_reindexing_indexes", + autospec=True, + return_value=[], + ) + run_reindex_batch_mock = mocker.patch( + "learning_resources_search.tasks.run_reindex_batch", autospec=True + ) + other_job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, + params={"indexes": ["program"]}, + status=TaskJob.Status.RUNNING, + ) + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, params={"indexes": ["program"]} + ) + + start_recreate_index.delay(job.id) + + job.refresh_from_db() + assert job.status == TaskJob.Status.FAILED + assert str(other_job.id) in job.error + assert run_reindex_batch_mock.delay.call_count == 0 + + +def test_start_recreate_index_resumes_running_job(mocker): + """ + A redelivery of start_recreate_index for a RUNNING job (worker died mid + enqueue loop) should re-enqueue still-queued batches without redoing setup + """ + create_backing_index_mock = mocker.patch( + "learning_resources_search.indexing_api.create_backing_index", autospec=True + ) + delete_orphaned_indexes_mock = mocker.patch( + "learning_resources_search.indexing_api.delete_orphaned_indexes", autospec=True + ) + run_reindex_batch_mock = mocker.patch( + "learning_resources_search.tasks.run_reindex_batch", autospec=True + ) + finish_mock = mocker.patch( + "learning_resources_search.tasks.finish_reindex_job", autospec=True + ) + + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, + params={"indexes": ["course"], "backing_indexes": {"course": "backing"}}, + status=TaskJob.Status.RUNNING, + ) + # one batch already finished in the first attempt, one never got enqueued + done_batch = TaskBatchFactory.create( + job=job, + kind=ReindexBatchKind.learning_resources.value, + status=TaskBatch.Status.SUCCEEDED, + ) + stranded_batch = TaskBatchFactory.create( + job=job, + kind=ReindexBatchKind.learning_resources.value, + status=TaskBatch.Status.QUEUED, + ) + + start_recreate_index.delay(job.id) + + # setup must NOT be redone (recreating backing indexes would orphan the + # documents the finished batch already wrote) + create_backing_index_mock.assert_not_called() + delete_orphaned_indexes_mock.assert_not_called() + # only the still-queued batch is (re)enqueued + run_reindex_batch_mock.delay.assert_called_once_with(stranded_batch.id) + # no new batch rows created + assert job.batches.count() == 2 + assert done_batch.job_id == job.id + # job is not complete yet (a queued batch remains), so finish isn't claimed + finish_mock.delay.assert_not_called() @pytest.mark.parametrize("with_error", [True, False]) -def test_finish_recreate_index(mocker, with_error): +def test_finish_reindex_job(mocker, with_error): """ - finish_recreate_index should attach the backing index to the default alias + finish_reindex_job should attach the backing index to the default alias """ - backing_indices = {"course": "backing", "program": "backing"} - results = ["error"] if with_error else [] + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, + params={ + "indexes": ["course", "program"], + "backing_indexes": {"course": "backing", "program": "backing"}, + }, + status=TaskJob.Status.FINISHING, + ) + TaskBatchFactory.create(job=job, status=TaskBatch.Status.SUCCEEDED) + if with_error: + TaskBatchFactory.create(job=job, status=TaskBatch.Status.FAILED, error="error") switch_indices_mock = mocker.patch( "learning_resources_search.indexing_api.switch_indices", autospec=True ) mock_delete_orphans = mocker.patch( "learning_resources_search.indexing_api.delete_orphaned_indexes" ) + mocker.patch( + "learning_resources_search.indexing_api.is_default_backing_index", + autospec=True, + return_value=False, + ) if with_error: with pytest.raises(ReindexError): - finish_recreate_index.delay(results, backing_indices) + finish_reindex_job.delay(job.id) switch_indices_mock.assert_not_called() mock_delete_orphans.assert_called_once() + job.refresh_from_db() + assert job.status == TaskJob.Status.FAILED + assert "error" in job.error else: - finish_recreate_index.delay(results, backing_indices) + finish_reindex_job.delay(job.id) switch_indices_mock.assert_any_call("backing", COURSE_TYPE) switch_indices_mock.assert_any_call("backing", PROGRAM_TYPE) mock_delete_orphans.assert_not_called() + job.refresh_from_db() + assert job.status == TaskJob.Status.SUCCEEDED + + +def test_finish_reindex_job_skips_already_switched(mocker): + """ + A redelivered finish_reindex_job should not re-switch (and thereby delete) + a backing index that is already the default + """ + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, + params={"indexes": ["course"], "backing_indexes": {"course": "backing"}}, + status=TaskJob.Status.FINISHING, + ) + switch_indices_mock = mocker.patch( + "learning_resources_search.indexing_api.switch_indices", autospec=True + ) + mocker.patch( + "learning_resources_search.indexing_api.is_default_backing_index", + autospec=True, + return_value=True, + ) + + finish_reindex_job.delay(job.id) + + switch_indices_mock.assert_not_called() + job.refresh_from_db() + assert job.status == TaskJob.Status.SUCCEEDED + + +def test_finish_reindex_job_noop_when_not_finishing(mocker): + """finish_reindex_job should do nothing unless the job is in finishing state""" + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, + params={"indexes": ["course"], "backing_indexes": {"course": "backing"}}, + status=TaskJob.Status.SUCCEEDED, + ) + switch_indices_mock = mocker.patch( + "learning_resources_search.indexing_api.switch_indices", autospec=True + ) + + finish_reindex_job.delay(job.id) + + switch_indices_mock.assert_not_called() @pytest.mark.parametrize("with_error", [True, False]) -def test_finish_recreate_index_retry_exceptions(mocker, with_error): +def test_finish_reindex_job_retry_exceptions(mocker, with_error): """ - finish_recreate_index should be retried on RequestErrors + finish_reindex_job should be retried on RequestErrors """ - backing_indices = {"course": "backing", "program": "backing"} - results = ["error"] if with_error else [] + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, + params={ + "indexes": ["course", "program"], + "backing_indexes": {"course": "backing", "program": "backing"}, + }, + status=TaskJob.Status.FINISHING, + ) + if with_error: + TaskBatchFactory.create(job=job, status=TaskBatch.Status.FAILED, error="error") mock_error = RequestError(429, "oops", {}) switch_indices_mock = mocker.patch( "learning_resources_search.indexing_api.switch_indices", @@ -500,9 +680,17 @@ def test_finish_recreate_index_retry_exceptions(mocker, with_error): "learning_resources_search.indexing_api.delete_orphaned_indexes", side_effect=[mock_error, None], ) + mocker.patch( + "learning_resources_search.indexing_api.is_default_backing_index", + autospec=True, + return_value=False, + ) with pytest.raises(Retry): - finish_recreate_index.delay(results, backing_indices) + finish_reindex_job.delay(job.id) + job.refresh_from_db() + # the job stays in finishing state so the retry can pick it back up + assert job.status == TaskJob.Status.FINISHING if with_error: switch_indices_mock.assert_not_called() mock_delete_orphans.assert_called_once() @@ -511,6 +699,315 @@ def test_finish_recreate_index_retry_exceptions(mocker, with_error): switch_indices_mock.assert_called_once() +def test_run_reindex_batch_learning_resources(mocker, mocked_api): + """run_reindex_batch should index learning resources for that batch kind""" + finish_mock = mocker.patch( + "learning_resources_search.tasks.finish_reindex_job", autospec=True + ) + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, status=TaskJob.Status.RUNNING + ) + batch = TaskBatchFactory.create( + job=job, + kind=ReindexBatchKind.learning_resources.value, + params={"ids": [1, 2], "index_name": COURSE_TYPE}, + ) + + run_reindex_batch.delay(batch.id) + + mocked_api.index_learning_resources.assert_called_once_with( + [1, 2], COURSE_TYPE, IndexestoUpdate.reindexing_index.value + ) + batch.refresh_from_db() + assert batch.status == TaskBatch.Status.SUCCEEDED + # the last batch of the job claims the finish step + job.refresh_from_db() + assert job.status == TaskJob.Status.FINISHING + finish_mock.delay.assert_called_once_with(job.id) + + +def test_run_reindex_batch_content_files(mocker, mocked_api): + """run_reindex_batch should index content files for that batch kind""" + mocker.patch("learning_resources_search.tasks.finish_reindex_job", autospec=True) + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, status=TaskJob.Status.RUNNING + ) + batch = TaskBatchFactory.create( + job=job, + kind=ReindexBatchKind.content_files.value, + params={ + "ids": [3, 4], + "learning_resource_id": 7, + "resource_type": PROGRAM_TYPE, + }, + ) + + run_reindex_batch.delay(batch.id) + + mocked_api.index_content_files.assert_called_once_with( + [3, 4], + 7, + index_types=IndexestoUpdate.reindexing_index.value, + resource_type=PROGRAM_TYPE, + ) + batch.refresh_from_db() + assert batch.status == TaskBatch.Status.SUCCEEDED + + +def test_run_reindex_batch_percolate(mocker, mocked_api): + """run_reindex_batch should index percolate queries for that batch kind""" + mocker.patch("learning_resources_search.tasks.finish_reindex_job", autospec=True) + serialize_mock = mocker.patch( + "learning_resources_search.tasks.serialize_bulk_percolators", + return_value=["serialized"], + ) + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, status=TaskJob.Status.RUNNING + ) + batch = TaskBatchFactory.create( + job=job, + kind=ReindexBatchKind.percolate.value, + params={"ids": [5, 6]}, + ) + + run_reindex_batch.delay(batch.id) + + serialize_mock.assert_called_once_with([5, 6]) + mocked_api.index_items.assert_called_once_with( + ["serialized"], + PERCOLATE_INDEX_TYPE, + IndexestoUpdate.reindexing_index.value, + ) + batch.refresh_from_db() + assert batch.status == TaskBatch.Status.SUCCEEDED + + +def test_run_reindex_batch_dispatch_content_files(mocker, mocked_api): + """ + A dispatch batch should create and enqueue content file batches, idempotently + """ + settings.OPENSEARCH_DOCUMENT_INDEXING_CHUNK_SIZE = 2 + course = CourseFactory.create(etl_source=ETLSource.ocw.value) + run = course.learning_resource.runs.first() + run_files = sorted( + ContentFileFactory.create_batch(3, run=run), key=lambda file: file.id + ) + marketing_file = ContentFileFactory.create( + learning_resource=course.learning_resource + ) + delay_mock = mocker.patch.object(run_reindex_batch, "delay") + + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, status=TaskJob.Status.RUNNING + ) + batch = TaskBatchFactory.create( + job=job, + kind=ReindexBatchKind.dispatch_content_files.value, + params={ + "learning_resource_ids": [course.learning_resource_id], + "resource_type": COURSE_TYPE, + }, + ) + + run_reindex_batch(batch.id) + + children = job.batches.filter(kind=ReindexBatchKind.content_files.value).order_by( + "batch_key" + ) + assert children.count() == 3 + child_params = [child.params for child in children] + assert { + "ids": [run_files[0].id, run_files[1].id], + "learning_resource_id": course.learning_resource_id, + "resource_type": COURSE_TYPE, + } in child_params + assert { + "ids": [run_files[2].id], + "learning_resource_id": course.learning_resource_id, + "resource_type": COURSE_TYPE, + } in child_params + assert { + "ids": [marketing_file.id], + "learning_resource_id": course.learning_resource_id, + "resource_type": COURSE_TYPE, + } in child_params + + batch.refresh_from_db() + assert batch.status == TaskBatch.Status.SUCCEEDED + assert delay_mock.call_count == 3 + + # re-running the dispatch (e.g. on redelivery) must not duplicate children + TaskBatch.objects.filter(id=batch.id).update(status=TaskBatch.Status.QUEUED) + run_reindex_batch(batch.id) + assert job.batches.filter(kind=ReindexBatchKind.content_files.value).count() == 3 + + +def test_run_reindex_batch_error(mocker, mocked_api): + """run_reindex_batch should mark the batch failed on a non-retryable error""" + finish_mock = mocker.patch( + "learning_resources_search.tasks.finish_reindex_job", autospec=True + ) + mocked_api.index_learning_resources.side_effect = ValueError("boom") + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, status=TaskJob.Status.RUNNING + ) + batch = TaskBatchFactory.create( + job=job, + kind=ReindexBatchKind.learning_resources.value, + params={"ids": [1], "index_name": COURSE_TYPE}, + ) + + run_reindex_batch.delay(batch.id) + + batch.refresh_from_db() + assert batch.status == TaskBatch.Status.FAILED + assert "boom" in batch.error + # failed batches still count toward completion so the job can finish/clean up + job.refresh_from_db() + assert job.status == TaskJob.Status.FINISHING + finish_mock.delay.assert_called_once_with(job.id) + + +def test_run_reindex_batch_retry(mocker, mocked_api): + """run_reindex_batch should retry on search connection errors""" + mocker.patch("learning_resources_search.tasks.finish_reindex_job", autospec=True) + mocked_api.index_learning_resources.side_effect = ESConnectionError( + "err", "err", "err" + ) + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, status=TaskJob.Status.RUNNING + ) + batch = TaskBatchFactory.create( + job=job, + kind=ReindexBatchKind.learning_resources.value, + params={"ids": [1], "index_name": COURSE_TYPE}, + ) + + with pytest.raises(Retry): + run_reindex_batch.delay(batch.id) + + batch.refresh_from_db() + # the batch is left non-terminal so the celery retry re-runs it + assert batch.status == TaskBatch.Status.RUNNING + + +def test_run_reindex_batch_on_failure_fails_batch(mocker): + """ + When run_reindex_batch gives up (retries exhausted) its on_failure handler + should mark the batch FAILED and nudge the job toward completion + """ + finish_mock = mocker.patch( + "learning_resources_search.tasks.finish_reindex_job", autospec=True + ) + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, status=TaskJob.Status.RUNNING + ) + batch = TaskBatchFactory.create( + job=job, + kind=ReindexBatchKind.learning_resources.value, + status=TaskBatch.Status.RUNNING, + ) + + run_reindex_batch.on_failure(RetryError("boom"), "task-id", [batch.id], {}, None) + + batch.refresh_from_db() + assert batch.status == TaskBatch.Status.FAILED + assert "boom" in batch.error + # it was the only batch, so the job is now claimable for finishing + job.refresh_from_db() + assert job.status == TaskJob.Status.FINISHING + finish_mock.delay.assert_called_once_with(job.id) + + +def test_finish_reindex_job_on_failure_fails_job(): + """ + When finish_reindex_job gives up its on_failure handler should mark the + FINISHING job FAILED so it doesn't hang active forever + """ + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, + params={"indexes": ["course"], "backing_indexes": {"course": "backing"}}, + status=TaskJob.Status.FINISHING, + ) + + finish_reindex_job.on_failure( + RetryError("switch failed"), "task-id", [job.id], {}, None + ) + + job.refresh_from_db() + assert job.status == TaskJob.Status.FAILED + assert "switch failed" in job.error + + +@pytest.mark.parametrize( + ("batch_status", "job_status"), + [ + (TaskBatch.Status.SUCCEEDED, TaskJob.Status.RUNNING), + (TaskBatch.Status.QUEUED, TaskJob.Status.FAILED), + ], +) +def test_run_reindex_batch_noop(mocker, mocked_api, batch_status, job_status): + """run_reindex_batch should do nothing if the batch or job is terminal""" + mocker.patch("learning_resources_search.tasks.finish_reindex_job", autospec=True) + job = TaskJobFactory.create(task_name=REINDEX_TASK_NAME, status=job_status) + batch = TaskBatchFactory.create( + job=job, + kind=ReindexBatchKind.learning_resources.value, + status=batch_status, + params={"ids": [1], "index_name": COURSE_TYPE}, + ) + + run_reindex_batch.delay(batch.id) + + mocked_api.index_learning_resources.assert_not_called() + batch.refresh_from_db() + assert batch.status == batch_status + + +def test_maybe_finish_reindex_job(mocker): + """_maybe_finish_reindex_job should claim the finish exactly once""" + finish_mock = mocker.patch( + "learning_resources_search.tasks.finish_reindex_job", autospec=True + ) + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, status=TaskJob.Status.RUNNING + ) + done_batch = TaskBatchFactory.create(job=job, status=TaskBatch.Status.SUCCEEDED) + pending_batch = TaskBatchFactory.create(job=job, status=TaskBatch.Status.QUEUED) + + _maybe_finish_reindex_job(job.id) + job.refresh_from_db() + assert job.status == TaskJob.Status.RUNNING + finish_mock.delay.assert_not_called() + + TaskBatch.objects.filter(id=pending_batch.id).update(status=TaskBatch.Status.FAILED) + _maybe_finish_reindex_job(job.id) + job.refresh_from_db() + assert job.status == TaskJob.Status.FINISHING + finish_mock.delay.assert_called_once_with(job.id) + + # a second caller cannot claim the finish again + _maybe_finish_reindex_job(job.id) + assert finish_mock.delay.call_count == 1 + assert done_batch.job_id == job.id + + +def test_maybe_finish_reindex_job_no_batches(mocker): + """A job with no batches at all should finish immediately""" + finish_mock = mocker.patch( + "learning_resources_search.tasks.finish_reindex_job", autospec=True + ) + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, status=TaskJob.Status.RUNNING + ) + + _maybe_finish_reindex_job(job.id) + + job.refresh_from_db() + assert job.status == TaskJob.Status.FINISHING + finish_mock.delay.assert_called_once_with(job.id) + + @pytest.mark.usefixtures("_wrap_retry_mock") @pytest.mark.parametrize("with_error", [True, False]) @pytest.mark.parametrize( @@ -1245,6 +1742,73 @@ def get_percolator(res): assert topic in template_data +@pytest.mark.parametrize( + ("status", "expected"), + [(200, True), (404, False), (403, False), (500, False)], +) +def test_image_url_is_reachable(mocked_responses, status, expected): + """_image_url_is_reachable should be True only for successful responses""" + url = "http://example.com/image.jpg" + mocked_responses.add(responses.HEAD, url, status=status) + assert _image_url_is_reachable(url) is expected + + +@pytest.mark.parametrize(("final_status", "expected"), [(200, True), (404, False)]) +def test_image_url_is_reachable_follows_redirect( + mocked_responses, final_status, expected +): + """A redirect is followed through to the destination's status""" + url = "http://example.com/image.jpg" + redirected_to = "http://example.com/moved.jpg" + mocked_responses.add( + responses.HEAD, url, status=301, headers={"Location": redirected_to} + ) + mocked_responses.add(responses.HEAD, redirected_to, status=final_status) + assert _image_url_is_reachable(url) is expected + + +def test_image_url_is_reachable_head_not_allowed(mocked_responses): + """Servers that reject HEAD should be retried with GET""" + url = "http://example.com/image.jpg" + mocked_responses.add(responses.HEAD, url, status=405) + mocked_responses.add(responses.GET, url, status=200) + assert _image_url_is_reachable(url) is True + + +def test_image_url_is_reachable_connection_error(mocked_responses): + """A request that errors out counts as unreachable""" + url = "http://example.com/image.jpg" + mocked_responses.add( + responses.HEAD, url, body=requests.exceptions.ConnectionError() + ) + assert _image_url_is_reachable(url) is False + + +@pytest.mark.parametrize("reachable", [True, False]) +def test_validated_resource_image_url(mock_image_url_is_reachable, reachable): + """ + The digest email should use the resource image only if its URL is + reachable, and the default image otherwise + """ + mock_image_url_is_reachable.return_value = reachable + resource = LearningResourceFactory.create(is_course=True) + validated_url = _validated_resource_image_url(resource) + if reachable: + assert validated_url == resource.image.url + else: + assert validated_url == frontend_absolute_url("/images/default_resource.jpg") + mock_image_url_is_reachable.assert_called_once_with(resource.image.url) + + +def test_validated_resource_image_url_no_image(mock_image_url_is_reachable): + """The digest email should use the default image if the resource has none""" + resource = LearningResourceFactory.create(is_course=True, no_image=True) + assert _validated_resource_image_url(resource) == frontend_absolute_url( + "/images/default_resource.jpg" + ) + mock_image_url_is_reachable.assert_not_called() + + def test_subscription_digest_subject(): """ Test that email generates a dynamic subject based @@ -1379,11 +1943,22 @@ def test_cache_is_cleared_after_reindex(mocker): "learning_resources_search.tasks.clear_views_cache" ) - backing_indices = {"course": "backing", "program": "backing"} - results = [] + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, + params={ + "indexes": ["course", "program"], + "backing_indexes": {"course": "backing", "program": "backing"}, + }, + status=TaskJob.Status.FINISHING, + ) mocker.patch("learning_resources_search.indexing_api.switch_indices", autospec=True) mocker.patch("learning_resources_search.indexing_api.delete_orphaned_indexes") - finish_recreate_index.delay(results, backing_indices) + mocker.patch( + "learning_resources_search.indexing_api.is_default_backing_index", + autospec=True, + return_value=False, + ) + finish_reindex_job.delay(job.id) assert mocked_clear_views_cache.call_count == 1 diff --git a/main/admin.py b/main/admin.py new file mode 100644 index 0000000000..81c7460d42 --- /dev/null +++ b/main/admin.py @@ -0,0 +1,37 @@ +"""admin for main""" + +from django.contrib import admin + +from main import models + + +class TaskJobAdmin(admin.ModelAdmin): + """TaskJob Admin""" + + model = models.TaskJob + list_display = ("id", "task_name", "status", "created_on", "updated_on") + list_filter = ("status", "task_name") + readonly_fields = ("created_on", "updated_on") + + +admin.site.register(models.TaskJob, TaskJobAdmin) + + +class TaskBatchAdmin(admin.ModelAdmin): + """TaskBatch Admin""" + + model = models.TaskBatch + list_display = ( + "id", + "job", + "batch_key", + "kind", + "status", + "updated_on", + ) + list_filter = ("status", "kind") + search_fields = ("batch_key",) + readonly_fields = ("created_on", "updated_on") + + +admin.site.register(models.TaskBatch, TaskBatchAdmin) diff --git a/main/factories.py b/main/factories.py index 02ab8b93a2..e9a8a7552f 100644 --- a/main/factories.py +++ b/main/factories.py @@ -11,6 +11,7 @@ LazyFunction, RelatedFactory, SelfAttribute, + Sequence, SubFactory, Trait, ) @@ -18,6 +19,8 @@ from factory.fuzzy import FuzzyText from social_django.models import UserSocialAuth +from main.models import TaskBatch, TaskJob + class UserFactory(DjangoModelFactory): """Factory for Users""" @@ -62,3 +65,25 @@ class UserSocialAuthFactory(DjangoModelFactory): class Meta: model = UserSocialAuth + + +class TaskJobFactory(DjangoModelFactory): + """Factory for TaskJobs""" + + task_name = FuzzyText() + params = LazyFunction(dict) + + class Meta: + model = TaskJob + + +class TaskBatchFactory(DjangoModelFactory): + """Factory for TaskBatches""" + + job = SubFactory(TaskJobFactory) + batch_key = Sequence(lambda n: f"batch:{n}") + kind = FuzzyText() + params = LazyFunction(dict) + + class Meta: + model = TaskBatch diff --git a/main/migrations/0001_taskjob_taskbatch.py b/main/migrations/0001_taskjob_taskbatch.py new file mode 100644 index 0000000000..d17f4f38e9 --- /dev/null +++ b/main/migrations/0001_taskjob_taskbatch.py @@ -0,0 +1,102 @@ +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + initial = True + + dependencies = [] + + operations = [ + migrations.CreateModel( + name="TaskJob", + fields=[ + ( + "id", + models.BigAutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ("created_on", models.DateTimeField(auto_now_add=True, db_index=True)), + ("updated_on", models.DateTimeField(auto_now=True)), + ("task_name", models.CharField(db_index=True, max_length=255)), + ("params", models.JSONField(default=dict)), + ( + "status", + models.CharField( + choices=[ + ("queued", "Queued"), + ("running", "Running"), + ("finishing", "Finishing"), + ("succeeded", "Succeeded"), + ("failed", "Failed"), + ], + db_index=True, + default="queued", + max_length=20, + ), + ), + ("error", models.TextField(blank=True)), + ], + options={ + "abstract": False, + }, + ), + migrations.CreateModel( + name="TaskBatch", + fields=[ + ( + "id", + models.BigAutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ("created_on", models.DateTimeField(auto_now_add=True, db_index=True)), + ("updated_on", models.DateTimeField(auto_now=True)), + ("batch_key", models.CharField(max_length=255)), + ("kind", models.CharField(max_length=64)), + ("params", models.JSONField(default=dict)), + ( + "status", + models.CharField( + choices=[ + ("queued", "Queued"), + ("running", "Running"), + ("succeeded", "Succeeded"), + ("failed", "Failed"), + ], + db_index=True, + default="queued", + max_length=20, + ), + ), + ("error", models.TextField(blank=True)), + ( + "job", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + related_name="batches", + to="main.taskjob", + ), + ), + ], + ), + migrations.AddIndex( + model_name="taskbatch", + index=models.Index( + fields=["job", "status"], name="taskbatch_job_status_idx" + ), + ), + migrations.AddConstraint( + model_name="taskbatch", + constraint=models.UniqueConstraint( + fields=("job", "batch_key"), name="unique_task_batch_key" + ), + ), + ] diff --git a/main/migrations/__init__.py b/main/migrations/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/main/models.py b/main/models.py index 4bafc925ff..431fe9ee5f 100644 --- a/main/models.py +++ b/main/models.py @@ -2,6 +2,7 @@ Classes related to models for main """ +from django.db import models from django.db.models import DateTimeField, Model from django.db.models.query import QuerySet @@ -52,3 +53,70 @@ class NoDefaultTimestampedModel(TimestampedModel): class Meta: abstract = True + + +class TaskJob(TimestampedModel): + """ + Tracks a long-running celery task that has been decomposed into batches + (TaskBatch rows), so its progress and completion live in the database + rather than in any single worker process + """ + + class Status(models.TextChoices): + QUEUED = "queued" # created; the start task has not completed yet + RUNNING = "running" # batches created and executing + FINISHING = "finishing" # all batches done; finish step claimed + SUCCEEDED = "succeeded" + FAILED = "failed" + + ACTIVE_STATUSES = (Status.QUEUED, Status.RUNNING, Status.FINISHING) + + task_name = models.CharField(max_length=255, db_index=True) + params = models.JSONField(default=dict) + status = models.CharField( + max_length=20, + choices=Status.choices, + default=Status.QUEUED, + db_index=True, + ) + error = models.TextField(blank=True) + + def __str__(self): + return f"Task job {self.id} {self.task_name} ({self.status})" + + +class TaskBatch(TimestampedModel): + """A unit of work for a TaskJob""" + + class Status(models.TextChoices): + QUEUED = "queued" # waiting for a worker to pick it up + RUNNING = "running" # a worker has started executing it + SUCCEEDED = "succeeded" + FAILED = "failed" + + NON_TERMINAL_STATUSES = (Status.QUEUED, Status.RUNNING) + + job = models.ForeignKey(TaskJob, on_delete=models.CASCADE, related_name="batches") + batch_key = models.CharField(max_length=255) + kind = models.CharField(max_length=64) + params = models.JSONField(default=dict) + status = models.CharField( + max_length=20, + choices=Status.choices, + default=Status.QUEUED, + db_index=True, + ) + error = models.TextField(blank=True) + + class Meta: + constraints = [ + models.UniqueConstraint( + fields=["job", "batch_key"], name="unique_task_batch_key" + ) + ] + indexes = [ + models.Index(fields=["job", "status"], name="taskbatch_job_status_idx") + ] + + def __str__(self): + return f"Task batch {self.batch_key} ({self.status}) for job {self.job_id}" diff --git a/main/settings.py b/main/settings.py index 8ffe4ab3e4..42db269aa2 100644 --- a/main/settings.py +++ b/main/settings.py @@ -36,7 +36,7 @@ from main.settings_pluggy import * # noqa: F403 from openapi.settings_spectacular import open_spectacular_settings -VERSION = "0.77.2" +VERSION = "0.77.3" log = logging.getLogger() @@ -545,6 +545,12 @@ "OPENSEARCH_DOCUMENT_INDEXING_CHUNK_SIZE", get_int("OPENSEARCH_INDEXING_CHUNK_SIZE", 100), ) +# learning resources per dispatch_content_files reindex batch +OPENSEARCH_REINDEX_DISPATCH_CHUNK_SIZE = get_int( + "OPENSEARCH_REINDEX_DISPATCH_CHUNK_SIZE", 100 +) +# how long finished TaskJobs (and their batches) are kept before cleanup +TASK_JOB_RETENTION_DAYS = get_int("TASK_JOB_RETENTION_DAYS", 7) OPENSEARCH_MIN_QUERY_SIZE = get_int("OPENSEARCH_MIN_QUERY_SIZE", 2) OPENSEARCH_MAX_SUGGEST_HITS = get_int("OPENSEARCH_MAX_SUGGEST_HITS", 1) OPENSEARCH_MAX_SUGGEST_RESULTS = get_int("OPENSEARCH_MAX_SUGGEST_RESULTS", 1) diff --git a/main/settings_celery.py b/main/settings_celery.py index 62f11fce73..70452ff21e 100644 --- a/main/settings_celery.py +++ b/main/settings_celery.py @@ -164,6 +164,10 @@ "task": "learning_resources_search.tasks.update_featured_rank", "schedule": crontab(minute=30, hour=7), # 3:30am EST }, + "delete-old-task-jobs-every-1-days": { + "task": "main.tasks.delete_old_task_jobs", + "schedule": crontab(minute=0, hour=8), # 4:00am EST + }, "scrape-marketing-pages-every-1-days": { "task": "learning_resources.tasks.scrape_marketing_pages", "schedule": get_int( diff --git a/main/settings_course_etl.py b/main/settings_course_etl.py index c4211b5661..fe27d826ce 100644 --- a/main/settings_course_etl.py +++ b/main/settings_course_etl.py @@ -14,9 +14,6 @@ "EDX_COURSE_BUCKET_PREFIX", "edxorg-raw-data/edxorg/raw_data/course_xml/" ) -# Authentication for the github api -GITHUB_ACCESS_TOKEN = get_string("GITHUB_ACCESS_TOKEN", None) - # OCW settings OCW_LIVE_BUCKET = get_string("OCW_LIVE_BUCKET", None) OCW_ITERATOR_CHUNK_SIZE = get_int("OCW_ITERATOR_CHUNK_SIZE", 1000) diff --git a/main/tasks.py b/main/tasks.py new file mode 100644 index 0000000000..90183524fc --- /dev/null +++ b/main/tasks.py @@ -0,0 +1,45 @@ +"""Helpers for batched task jobs (TaskJob / TaskBatch)""" + +import datetime +import logging + +from django.conf import settings + +from main.celery import app +from main.models import TaskBatch, TaskJob +from main.utils import now_in_utc + +log = logging.getLogger(__name__) + + +def maybe_finish_task_job(job_id, finish_task): + """ + Claim and enqueue the finish step if every batch of the job is done. + + The conditional UPDATE guarantees exactly one caller claims the finish; + each batch commits its terminal status before calling this, so the last + batch to finish is guaranteed to see all batches terminal. + + Args: + job_id (int): TaskJob id + finish_task (celery task): task taking a TaskJob id that finalizes it + """ + claimed = ( + TaskJob.objects.filter(id=job_id, status=TaskJob.Status.RUNNING) + .exclude(batches__status__in=TaskBatch.NON_TERMINAL_STATUSES) + .update(status=TaskJob.Status.FINISHING) + ) + if claimed: + finish_task.delay(job_id) + + +@app.task +def delete_old_task_jobs(): + """ + Delete TaskJobs (and, by cascade, their batches) with no activity in the + retention window. A job untouched for that long is treated as inactive, + whatever its status. + """ + threshold = now_in_utc() - datetime.timedelta(days=settings.TASK_JOB_RETENTION_DAYS) + deleted, _ = TaskJob.objects.filter(updated_on__lt=threshold).delete() + log.info("Deleted %d old task job/batch rows", deleted) diff --git a/main/tasks_test.py b/main/tasks_test.py new file mode 100644 index 0000000000..340d75cc8b --- /dev/null +++ b/main/tasks_test.py @@ -0,0 +1,55 @@ +"""Tests for batched task job helpers""" + +import datetime + +import pytest +from django.conf import settings + +from main.factories import TaskBatchFactory, TaskJobFactory +from main.models import TaskBatch, TaskJob +from main.tasks import delete_old_task_jobs, maybe_finish_task_job +from main.utils import now_in_utc + +pytestmark = pytest.mark.django_db + + +def test_maybe_finish_task_job(mocker): + """maybe_finish_task_job should claim the finish once all batches terminal""" + finish_task = mocker.Mock() + job = TaskJobFactory.create(status=TaskJob.Status.RUNNING) + TaskBatchFactory.create(job=job, status=TaskBatch.Status.SUCCEEDED) + running_batch = TaskBatchFactory.create(job=job, status=TaskBatch.Status.RUNNING) + + # a running (non-terminal) batch must block completion + maybe_finish_task_job(job.id, finish_task) + job.refresh_from_db() + assert job.status == TaskJob.Status.RUNNING + finish_task.delay.assert_not_called() + + TaskBatch.objects.filter(id=running_batch.id).update(status=TaskBatch.Status.FAILED) + maybe_finish_task_job(job.id, finish_task) + job.refresh_from_db() + assert job.status == TaskJob.Status.FINISHING + finish_task.delay.assert_called_once_with(job.id) + + # a second caller cannot claim the finish again + maybe_finish_task_job(job.id, finish_task) + assert finish_task.delay.call_count == 1 + + +def test_delete_old_task_jobs(): + """Jobs untouched past the retention window are deleted with their batches""" + old_job = TaskJobFactory.create(status=TaskJob.Status.SUCCEEDED) + old_batch = TaskBatchFactory.create(job=old_job) + recent_job = TaskJobFactory.create(status=TaskJob.Status.RUNNING) + recent_batch = TaskBatchFactory.create(job=recent_job) + + stale = now_in_utc() - datetime.timedelta(days=settings.TASK_JOB_RETENTION_DAYS + 1) + TaskJob.objects.filter(id=old_job.id).update(updated_on=stale) + + delete_old_task_jobs.delay() + + assert not TaskJob.objects.filter(id=old_job.id).exists() + assert not TaskBatch.objects.filter(id=old_batch.id).exists() # cascaded + assert TaskJob.objects.filter(id=recent_job.id).exists() + assert TaskBatch.objects.filter(id=recent_batch.id).exists() diff --git a/openapi/specs/v0.yaml b/openapi/specs/v0.yaml index dfba96cec4..384a27716b 100644 --- a/openapi/specs/v0.yaml +++ b/openapi/specs/v0.yaml @@ -147,6 +147,8 @@ paths: schema: $ref: '#/components/schemas/ContentFeedbackRequest' required: true + security: + - {} responses: '201': content: diff --git a/pyproject.toml b/pyproject.toml index bbf3d7178b..49991ffd08 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -163,6 +163,14 @@ dev = [ package = false default-groups = "all" override-dependencies = ["setuptools<80"] +exclude-newer = "7d" +required-version = ">=0.9.17" + +[tool.uv.exclude-newer-package] +mitol-django-common = "0d" +mitol-django-observability = "0d" +mitol-django-scim = "0d" +mitol-drf-lint = "0d" [tool.uv.build-backend] module-root = "" diff --git a/uv.lock b/uv.lock index 8f99f9d1a9..c1b80213dc 100644 --- a/uv.lock +++ b/uv.lock @@ -2,6 +2,16 @@ version = 1 revision = 3 requires-python = "==3.12.*" +[options] +exclude-newer = "0001-01-01T00:00:00Z" # This has no effect and is included for backwards compatibility when using relative exclude-newer values. +exclude-newer-span = "P7D" + +[options.exclude-newer-package] +mitol-drf-lint = { timestamp = "0001-01-01T00:00:00Z", span = "PT0S" } +mitol-django-observability = { timestamp = "0001-01-01T00:00:00Z", span = "PT0S" } +mitol-django-scim = { timestamp = "0001-01-01T00:00:00Z", span = "PT0S" } +mitol-django-common = { timestamp = "0001-01-01T00:00:00Z", span = "PT0S" } + [manifest] overrides = [{ name = "setuptools", specifier = "<80" }] @@ -1861,7 +1871,7 @@ name = "jinxed" version = "1.3.0" source = { registry = "https://pypi.org/simple" } dependencies = [ - { name = "ansicon", marker = "sys_platform == 'win32'" }, + { name = "ansicon" }, ] sdist = { url = "https://files.pythonhosted.org/packages/20/d0/59b2b80e7a52d255f9e0ad040d2e826342d05580c4b1d7d7747cfb8db731/jinxed-1.3.0.tar.gz", hash = "sha256:1593124b18a41b7a3da3b078471442e51dbad3d77b4d4f2b0c26ab6f7d660dbf", size = 80981, upload-time = "2024-07-31T22:39:18.854Z" } wheels = [