diff --git a/.github/workflows/README.md b/.github/workflows/README.md index 5d63d29fdef..03f8b46800e 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -33,6 +33,19 @@ How we write GitHub Actions workflows safely. Follow these when adding or editin ## Secrets - Expose a secret only to the job that uses it. Do not make secrets available to jobs that run untrusted code. +- **Never pass a secret to a local action (`uses: ./...`) on a `pull_request` run.** A `pull_request` run executes the merge commit (`GITHUB_REF` is `refs/pull/N/merge`), so the workflow file and every local action it calls are PR-authored — an input you pass is an input the PR reads. A guard _inside_ the action is too late; the secret is already an input. Gate it at the caller: + + ```yaml + # Bad — the PR controls .github/actions/foo/action.yml + github-token: ${{ secrets.SOME_PAT }} + + # Good — push runs publish, PR runs get an empty string + github-token: ${{ github.event_name != 'pull_request' && secrets.SOME_PAT || '' }} + ``` + + Fork PRs get no secrets, so this bites on same-repo branches — and there it is damage control rather than a boundary, since anyone who can push a branch can also edit the workflow. What it does buy is that a compromised dependency or third-party action in that job never sees the secret. + +- **A secret is exposed to its whole job, not just its step.** Steps share a runner, so PR-authored code running earlier in the job — a repo script, a build, a lifecycle hook — can shadow a binary on `$GITHUB_PATH` or write `$GITHUB_ENV` and capture the secret from a later step. If a job checks out PR code, keep secrets out of it entirely and do the privileged work in a separate job with no checkout. - Prefer OIDC (`id-token: write`) over long-lived stored secrets where the provider supports it. ## Supply chain @@ -54,4 +67,5 @@ How we write GitHub Actions workflows safely. Follow these when adding or editin 3. Trigger is `pull_request` unless `pull_request_target` is genuinely required and safe. 4. No `${{ github.event.* }}` inside `run:`. 5. Third-party actions pinned to SHAs. -6. Secrets scoped to the jobs that use them. +6. Secrets scoped to the jobs that use them, and never passed to a local action on a `pull_request` run. +7. No job both checks out PR code and holds a secret. diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 831d6393257..3175df20e8c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -103,23 +103,6 @@ jobs: ON_MISSING: ${{ (env.IS_MAIN == 'true' && github.repository == 'TryGhost/Ghost') && 'error' || 'previous-commit' }} run: node scripts/nx-set-shas.js --branch "$BRANCH" --head "$HEAD_COMMIT" --on-missing "$ON_MISSING" - - name: Check user org membership - id: check_user_org_membership - if: github.event_name == 'pull_request' - run: | - echo "Looking up: ${{ github.triggering_actor }}" - ENCODED_USERNAME=$(printf '%s' '${{ github.triggering_actor }}' | jq -sRr @uri) - - LOOKUP_USER=$(curl --write-out "%{http_code}" --silent --output /dev/null --location "https://api.github.com/orgs/tryghost/members/$ENCODED_USERNAME" --header "Authorization: Bearer ${{ secrets.CANARY_DOCKER_BUILD }}") - - if [ "$LOOKUP_USER" == "204" ]; then - echo "User is in the org" - echo "is_member=true" >> $GITHUB_OUTPUT - else - echo "User is not in the org" - echo "is_member=false" >> $GITHUB_OUTPUT - fi - - name: Determine changed packages if: env.IS_TAG != 'true' uses: AurorNZ/paths-filter@c9dd42e99db87803313ff6f4b1150cc9f6c836af # v5.0.0 @@ -325,7 +308,6 @@ jobs: is_development: ${{ env.IS_DEVELOPMENT }} is_six: ${{ env.IS_SIX }} is_six_pr: ${{ env.IS_SIX_PR }} - member_is_in_org: ${{ steps.check_user_org_membership.outputs.is_member }} has_perf_tests_label: ${{ github.event_name == 'pull_request' && contains(github.event.pull_request.labels.*.name, 'perf-tests') }} node_version: ${{ env.NODE_VERSION }} node_test_matrix: ${{ steps.node_matrix.outputs.matrix }} @@ -593,7 +575,9 @@ jobs: series: Benchmark metric: Boot time title: Boot time (dev tree) - github-token: ${{ secrets.CANARY_DOCKER_BUILD }} + # Only push runs publish, and only they get the token: a pull_request run + # executes PR-authored code, including this local action's own definition. + github-token: ${{ github.event_name != 'pull_request' && secrets.CANARY_DOCKER_BUILD || '' }} # Boot time of the production image, as a series separate from job_perf-tests: # that one tracks the code, this one tracks what ships (NODE_ENV=production, no @@ -685,7 +669,9 @@ jobs: series: Production image metric: Boot time title: Boot time (production image) - github-token: ${{ secrets.CANARY_DOCKER_BUILD }} + # Only push runs publish, and only they get the token: a pull_request run + # executes PR-authored code, including this local action's own definition. + github-token: ${{ github.event_name != 'pull_request' && secrets.CANARY_DOCKER_BUILD || '' }} job_unit-tests: runs-on: ubuntu-latest @@ -795,8 +781,6 @@ jobs: --health-timeout=5s --health-retries=60 env: - DB: mysql8 - NODE_ENV: testing-mysql COVERAGE_ENABLED: ${{ needs.job_setup.outputs.coverage_enabled }} name: Acceptance tests (Node ${{ needs.job_setup.outputs.node_version }}, mysql8) steps: @@ -902,9 +886,6 @@ jobs: --health-interval=10s --health-timeout=5s --health-retries=12 - env: - DB: mysql8 - NODE_ENV: testing-mysql name: Legacy tests (Node ${{ needs.job_setup.outputs.node_version }}, mysql8) steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 @@ -2344,6 +2325,7 @@ jobs: # Serialize per-app publishes so two quick main merges can't both compute the # same next-patch number and collide on npm. Different apps still publish in # parallel; cancel-in-progress stays false so a queued publish isn't dropped. + environment: npm-release concurrency: group: publish-public-app-${{ matrix.package_name }} cancel-in-progress: false diff --git a/.github/workflows/publish-packages.yml b/.github/workflows/publish-packages.yml index 32aa3f4ec7e..f48ee9b885b 100644 --- a/.github/workflows/publish-packages.yml +++ b/.github/workflows/publish-packages.yml @@ -55,6 +55,7 @@ jobs: github.event_name == 'workflow_dispatch' || (startsWith(github.ref, 'refs/tags/v') && !contains(github.ref_name, '-')) ) + environment: npm-release permissions: contents: read id-token: write diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index ceb725bff12..14a24979e84 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -1,16 +1,11 @@ name: Release -run-name: "Release — ${{ inputs.bump-type || 'auto' }} from ${{ inputs.branch || 'main' }}${{ inputs.dry-run && ' (dry run)' || '' }}" +run-name: "Release — ${{ inputs.bump-type || 'auto' }} from main${{ inputs.dry-run && ' (dry run)' || '' }}" on: schedule: - cron: '0 15 * * 2' # Tuesday 3pm UTC workflow_dispatch: inputs: - branch: - description: 'Git branch to release from' - type: string - default: 'main' - required: false bump-type: description: 'Version bump type (auto, patch, minor)' type: string @@ -55,7 +50,11 @@ jobs: with: # Deploy key (via ssh-agent) is used for git push — it bypasses # branch protection and triggers downstream workflows (unlike GITHUB_TOKEN) - ref: ${{ inputs.branch || 'main' }} + # + # Pinned to main, not a dispatch input: this job runs the checked-out + # scripts/release.js with the deploy key and PAT loaded, so a + # releasable ref must be one that went through review. + ref: main fetch-depth: 0 ssh-key: ${{ secrets.DEPLOY_KEY }} @@ -84,8 +83,8 @@ jobs: env: # release.js reads these RELEASE_* vars as its arg defaults. inputs.* # are empty on the schedule trigger, so fall back to the - # scheduled-release defaults (main / auto / no flags). - RELEASE_BRANCH: ${{ inputs.branch || 'main' }} + # scheduled-release defaults (auto / no flags). + RELEASE_BRANCH: main RELEASE_BUMP_TYPE: ${{ inputs.bump-type || 'auto' }} RELEASE_DRY_RUN: ${{ inputs.dry-run || 'false' }} RELEASE_SKIP_CHECKS: ${{ inputs.skip-checks || 'false' }} diff --git a/apps/admin/src/automations/components/automations-list.test.tsx b/apps/admin/src/automations/components/automations-list.test.tsx index 96ca8c8b95c..66872d607d1 100644 --- a/apps/admin/src/automations/components/automations-list.test.tsx +++ b/apps/admin/src/automations/components/automations-list.test.tsx @@ -74,7 +74,7 @@ describe('AutomationsList', () => { expect(screen.getByRole('columnheader', { name: 'In progress' })).toBeInTheDocument(); expect(screen.getByText('1,432')).toBeInTheDocument(); expect(screen.getByText('118')).toBeInTheDocument(); - expect(screen.getByText('14 days ago')).toHaveAttribute('datetime', '2026-07-21T07:12:00.000Z'); + expect(screen.getByText('Jul 21')).toHaveAttribute('datetime', '2026-07-21T07:12:00.000Z'); }); it('renders Never when an automation has no last entry', () => { diff --git a/apps/admin/src/automations/components/automations-list.tsx b/apps/admin/src/automations/components/automations-list.tsx index 772ee00a037..f9d3cbaede6 100644 --- a/apps/admin/src/automations/components/automations-list.tsx +++ b/apps/admin/src/automations/components/automations-list.tsx @@ -11,8 +11,7 @@ import { TableHeader, TableRow, } from '@tryghost/shade/components'; -import { cn, formatNumber } from '@tryghost/shade/utils'; -import moment from 'moment'; +import { cn, formatNumber, formatTimestamp } from '@tryghost/shade/utils'; const AUTOMATION_DESCRIPTIONS: Record = { 'member-welcome-email-free': 'Welcome new free members after they sign up.', @@ -145,7 +144,7 @@ const AutomationsList: React.FC = ({ const statCells = { lastEntry: { content: lastEntry ? ( - + ) : ( 'Never' ), diff --git a/apps/admin/src/render-admin-app-labs.acceptance.test.tsx b/apps/admin/src/render-admin-app-labs.acceptance.test.tsx new file mode 100644 index 00000000000..103c6ce8ae8 --- /dev/null +++ b/apps/admin/src/render-admin-app-labs.acceptance.test.tsx @@ -0,0 +1,126 @@ +import { describe, expect, it } from 'vitest'; + +import { + configResponse, + fakePages, + fakePosts, + fakePostsListScreen, + renderAdminApp, + settingsResponse, +} from '@test-utils/acceptance'; +import { composeLabsBootOverrides, type BootOverrides } from '@test-utils/acceptance/boot'; +import { postsListScreen } from '@/posts/list/posts-list.screen'; + +/** Resolve an override's response the way the boot table serves it. */ +async function bodyOf(override: BootOverrides[keyof BootOverrides]): Promise { + const response = override?.response; + return typeof response === 'function' + ? await (response as (request: Request) => unknown)(new Request('https://ghost.test')) + : response; +} + +function labsOf(configBody: unknown): Record { + return (configBody as { config: { labs: Record } }).config.labs; +} + +function labsSettingOf(settingsBody: unknown): Record { + const { settings } = settingsBody as { settings: Array<{ key: string; value: string }> }; + const entry = settings.find(({ key }) => key === 'labs'); + expect(entry).toBeDefined(); + return JSON.parse(entry!.value) as Record; +} + +describe('labs/boot composition', () => { + it('merges labs into a browseConfig object override without mutating it', async () => { + const config = configResponse(); + config.config.hostSettings = { marker: true }; + const snapshot = JSON.stringify(config); + + const composed = composeLabsBootOverrides( + { automations: true }, + { browseConfig: { response: config } }, + ); + const body = await bodyOf(composed.browseConfig); + + expect(labsOf(body).automations).toBe(true); + expect((body as { config: { hostSettings: unknown } }).config.hostSettings).toEqual({ + marker: true, + }); + expect(JSON.stringify(config)).toBe(snapshot); + }); + + it('wins over the same flag inside the override response', async () => { + const composed = composeLabsBootOverrides( + { automations: true }, + { + browseConfig: { response: configResponse({ labs: { automations: false } }) }, + browseSettings: { response: settingsResponse({ labs: { automations: false } }) }, + }, + ); + + expect(labsOf(await bodyOf(composed.browseConfig)).automations).toBe(true); + expect(labsSettingOf(await bodyOf(composed.browseSettings)).automations).toBe(true); + }); + + it('merges labs into a browseSettings override, adding the labs entry when missing', async () => { + const settings = settingsResponse(); + settings.settings = settings.settings.filter(({ key }) => key !== 'labs'); + + const composed = composeLabsBootOverrides( + { automations: true }, + { browseSettings: { response: settings } }, + ); + + expect(labsSettingOf(await bodyOf(composed.browseSettings)).automations).toBe(true); + }); + + it('merges labs into a function response', async () => { + const composed = composeLabsBootOverrides( + { automations: true }, + { browseSettings: { response: () => Promise.resolve(settingsResponse()) } }, + ); + + expect(labsSettingOf(await bodyOf(composed.browseSettings)).automations).toBe(true); + }); + + it('leaves unrecognized bodies untouched and keeps responseStatus', async () => { + const errors = { errors: [{ message: 'nope' }] }; + const composed = composeLabsBootOverrides( + { automations: true }, + { browseConfig: { response: errors, responseStatus: 422 } }, + ); + + expect(await bodyOf(composed.browseConfig)).toBe(errors); + expect(composed.browseConfig?.responseStatus).toBe(422); + }); + + it('compiles the canned responses when boot has no override for the entry', async () => { + const composed = composeLabsBootOverrides({ automations: true }); + + expect(await bodyOf(composed.browseConfig)).toEqual( + configResponse({ labs: { automations: true } }), + ); + expect(await bodyOf(composed.browseSettings)).toEqual( + settingsResponse({ labs: { automations: true } }), + ); + }); +}); + +describe('renderAdminApp labs + boot', () => { + // postsListReact gates which implementation serves /posts, so the React + // screen appearing proves the flag survived the boot overrides. + it('applies labs flags alongside browseConfig and browseSettings overrides', async () => { + fakePostsListScreen(); + fakePosts([]); + fakePages([]); + await renderAdminApp('/posts', { + labs: { postsListReact: true }, + boot: { + browseConfig: { response: configResponse() }, + browseSettings: { response: settingsResponse() }, + }, + }); + + await expect.element(postsListScreen.page('posts')).toBeVisible(); + }); +}); diff --git a/apps/admin/src/settings/advanced/labs/private-features.tsx b/apps/admin/src/settings/advanced/labs/private-features.tsx index ba19a8eb5d1..979ec31736a 100644 --- a/apps/admin/src/settings/advanced/labs/private-features.tsx +++ b/apps/admin/src/settings/advanced/labs/private-features.tsx @@ -72,12 +72,6 @@ const features: Feature[] = [ 'Deduplicate identical {{#get}} helper queries within a single request to avoid redundant database calls', flag: 'getHelperDeduplication', }, - { - title: 'React tag details', - description: - 'Renders the tag detail screen (/tags/:slug) from the React app instead of the Ember screen. Gates the migration behind a runtime toggle so we can compare both implementations.', - flag: 'tagDetailsReact', - }, { title: 'Member custom fields', description: 'Let admins create and manage custom field definitions for members', diff --git a/apps/admin/src/settings/advanced/migration-tools-export.acceptance.test.tsx b/apps/admin/src/settings/advanced/migration-tools-export.acceptance.test.tsx index 63bf87223d8..69e85e53939 100644 --- a/apps/admin/src/settings/advanced/migration-tools-export.acceptance.test.tsx +++ b/apps/admin/src/settings/advanced/migration-tools-export.acceptance.test.tsx @@ -29,9 +29,8 @@ function fakeExportDownload() { } async function renderWithArchiveHost() { - const config = configResponse({ labs: { selfServeArchives: true } }); - (config.config as { hostSettings?: object }).hostSettings = { - ...(config.config as { hostSettings?: object }).hostSettings, + const config = configResponse(); + config.config.hostSettings = { export: { webhookUrl: 'https://archives.example.com/generate' }, }; await renderAdminApp('/settings/advanced', { diff --git a/apps/admin/src/settings/advanced/migration-tools/content-import/mapping.test.ts b/apps/admin/src/settings/advanced/migration-tools/content-import/mapping.test.ts index 69ad2dd7396..817464d5035 100644 --- a/apps/admin/src/settings/advanced/migration-tools/content-import/mapping.test.ts +++ b/apps/admin/src/settings/advanced/migration-tools/content-import/mapping.test.ts @@ -33,6 +33,7 @@ describe('ContentFieldMapping', () => { 'authors', 'author_emails', 'tags', + 'frontmatter', 'Something else', ]); @@ -45,6 +46,7 @@ describe('ContentFieldMapping', () => { authors: 'authors', author_emails: 'author_emails', tags: 'tags', + frontmatter: '', 'Something else': '', }); }); @@ -106,10 +108,9 @@ describe('ContentFieldMapping', () => { 'custom_template', 'codeinjection_head', 'codeinjection_foot', - 'frontmatter', ]); expect(CONTENT_FIELD_MAPPINGS.map((field) => field.value)).not.toEqual( - expect.arrayContaining(['newsletter_id', 'email', 'tiers', 'id', 'lexical']), + expect.arrayContaining(['frontmatter', 'newsletter_id', 'email', 'tiers', 'id', 'lexical']), ); }); }); diff --git a/apps/admin/src/settings/advanced/migration-tools/content-import/mapping.ts b/apps/admin/src/settings/advanced/migration-tools/content-import/mapping.ts index 092e5fa8c0d..7af4a765871 100644 --- a/apps/admin/src/settings/advanced/migration-tools/content-import/mapping.ts +++ b/apps/admin/src/settings/advanced/migration-tools/content-import/mapping.ts @@ -79,7 +79,6 @@ export const CONTENT_FIELD_GROUPS: readonly ContentFieldGroup[] = [ { label: 'Custom template', value: 'custom_template', required: false }, { label: 'Code injection head', value: 'codeinjection_head', required: false }, { label: 'Code injection foot', value: 'codeinjection_foot', required: false }, - { label: 'Frontmatter', value: 'frontmatter', required: false }, ], }, ] as const; diff --git a/apps/admin/src/settings/membership/custom-fields.acceptance.test.tsx b/apps/admin/src/settings/membership/custom-fields.acceptance.test.tsx index 328dc617608..7bd179807ac 100644 --- a/apps/admin/src/settings/membership/custom-fields.acceptance.test.tsx +++ b/apps/admin/src/settings/membership/custom-fields.acceptance.test.tsx @@ -2,12 +2,10 @@ import { describe, expect, it } from 'vitest'; import { page, userEvent } from 'vitest/browser'; import { - configResponse, fakeAdminEndpoint, fakeMemberCustomFields, fakeSettingsScreens, renderAdminApp, - settingsResponse, } from '@test-utils/acceptance'; import { settingsScreen } from '@/settings/settings.screen'; @@ -29,13 +27,7 @@ const archivedField = { updated_at: '2026-07-13T00:00:00.000Z', }; -function customFieldsBoot() { - const labs = { membersCustomFields: true }; - return { - browseConfig: { response: configResponse({ labs }) }, - browseSettings: { response: settingsResponse({ labs }) }, - }; -} +const flagOn = { labs: { membersCustomFields: true } }; type CustomField = typeof companyField; @@ -73,7 +65,7 @@ describe('Custom fields', () => { it('lists each field with its user-facing type, opting into archived fields', async () => { fakeSettingsScreens(); const customFieldsApi = fakeCustomFields(); - await renderAdminApp('/settings', { boot: customFieldsBoot() }); + await renderAdminApp('/settings', flagOn); // Browse hides archived by default; Settings asks for both statuses. await expect @@ -92,7 +84,7 @@ describe('Custom fields', () => { const createApi = fakeAdminEndpoint('POST', '/members/custom_fields/', { members_custom_fields: [{ ...companyField, key: 'job_title', name: 'Job Title' }], }); - await renderAdminApp('/settings', { boot: customFieldsBoot() }); + await renderAdminApp('/settings', flagOn); await settingsScreen.customFields().getByRole('button', { name: 'Add custom field' }).click(); const modal = settingsScreen.customFieldModal(); @@ -114,7 +106,7 @@ describe('Custom fields', () => { const createApi = fakeAdminEndpoint('POST', '/members/custom_fields/', { members_custom_fields: [{ ...companyField, key: 'bio', name: 'Bio', type: 'long_text' }], }); - await renderAdminApp('/settings', { boot: customFieldsBoot() }); + await renderAdminApp('/settings', flagOn); await settingsScreen.customFields().getByRole('button', { name: 'Add custom field' }).click(); const modal = settingsScreen.customFieldModal(); @@ -147,7 +139,7 @@ describe('Custom fields', () => { }, { status: 422 }, ); - await renderAdminApp('/settings', { boot: customFieldsBoot() }); + await renderAdminApp('/settings', flagOn); await settingsScreen.customFields().getByRole('button', { name: 'Add custom field' }).click(); const modal = settingsScreen.customFieldModal(); @@ -167,7 +159,7 @@ describe('Custom fields', () => { const editApi = fakeAdminEndpoint('PUT', '/members/custom_fields/company/', { members_custom_fields: [{ ...companyField, name: 'Employer' }], }); - await renderAdminApp('/settings', { boot: customFieldsBoot() }); + await renderAdminApp('/settings', flagOn); await settingsScreen.customFields().getByTestId('custom-field-list-item').click(); const modal = settingsScreen.customFieldModal(); @@ -186,7 +178,7 @@ describe('Custom fields', () => { const editApi = fakeAdminEndpoint('PUT', '/members/custom_fields/company/', { members_custom_fields: [{ ...companyField, status: 'archived' }], }); - await renderAdminApp('/settings', { boot: customFieldsBoot() }); + await renderAdminApp('/settings', flagOn); await settingsScreen.customFields().getByTestId('custom-field-list-item').click(); await settingsScreen.customFieldModal().getByRole('button', { name: 'Archive' }).click(); @@ -209,7 +201,7 @@ describe('Custom fields', () => { it('splits fields into Active and Archived tabs', async () => { fakeSettingsScreens(); fakeCustomFields([companyField, archivedField]); - await renderAdminApp('/settings', { boot: customFieldsBoot() }); + await renderAdminApp('/settings', flagOn); // Active tab is the default and shows only active fields. const rows = settingsScreen.customFields().getByTestId('custom-field-list-item'); @@ -229,7 +221,7 @@ describe('Custom fields', () => { name: `Field ${index}`, })); fakeCustomFields(manyFields); - await renderAdminApp('/settings', { boot: customFieldsBoot() }); + await renderAdminApp('/settings', flagOn); const rows = settingsScreen.customFields().getByTestId('custom-field-list-item'); await expect(rows).toHaveCount(5); @@ -255,7 +247,7 @@ describe('Custom fields', () => { name: `Field ${index}`, })); fakeCustomFieldsWithCreate(initialFields, { ...companyField, key: 'newest', name: 'Newest' }); - await renderAdminApp('/settings', { boot: customFieldsBoot() }); + await renderAdminApp('/settings', flagOn); const rows = settingsScreen.customFields().getByTestId('custom-field-list-item'); await expect(rows).toHaveCount(5); @@ -287,7 +279,7 @@ describe('Custom fields', () => { currentFields = order.map(({ key }) => currentFields.find((field) => field.key === key)!); return { members_custom_fields: currentFields }; }); - await renderAdminApp('/settings', { boot: customFieldsBoot() }); + await renderAdminApp('/settings', flagOn); const rows = settingsScreen.customFields().getByTestId('custom-field-list-item'); await expect(rows).toHaveCount(3); @@ -340,7 +332,7 @@ describe('Custom fields', () => { currentFields = order.map(({ key }) => currentFields.find((field) => field.key === key)!); return { members_custom_fields: currentFields }; }); - await renderAdminApp('/settings', { boot: customFieldsBoot() }); + await renderAdminApp('/settings', flagOn); const rows = settingsScreen.customFields().getByTestId('custom-field-list-item'); await expect(rows).toHaveCount(2); @@ -388,7 +380,7 @@ describe('Custom fields', () => { }, { status: 422 }, ); - await renderAdminApp('/settings', { boot: customFieldsBoot() }); + await renderAdminApp('/settings', flagOn); const rows = settingsScreen.customFields().getByTestId('custom-field-list-item'); await expect(rows).toHaveCount(2); @@ -418,7 +410,7 @@ describe('Custom fields', () => { currentFields = order.map(({ key }) => currentFields.find((field) => field.key === key)!); return { members_custom_fields: currentFields }; }); - await renderAdminApp('/settings', { boot: customFieldsBoot() }); + await renderAdminApp('/settings', flagOn); const rows = settingsScreen.customFields().getByTestId('custom-field-list-item'); await expect(rows).toHaveCount(2); @@ -451,7 +443,7 @@ describe('Custom fields', () => { currentFields = order.map(({ key }) => currentFields.find((field) => field.key === key)!); return { members_custom_fields: currentFields }; }); - await renderAdminApp('/settings', { boot: customFieldsBoot() }); + await renderAdminApp('/settings', flagOn); const rows = settingsScreen.customFields().getByTestId('custom-field-list-item'); await expect(rows).toHaveCount(5); @@ -480,7 +472,7 @@ describe('Custom fields', () => { it('does not offer dragging on the archived tab', async () => { fakeSettingsScreens(); fakeCustomFields([companyField, archivedField]); - await renderAdminApp('/settings', { boot: customFieldsBoot() }); + await renderAdminApp('/settings', flagOn); // An archived field holds its place in the order, but there is nowhere to see // it, so there is nothing to drag it through. @@ -496,7 +488,7 @@ describe('Custom fields', () => { fakeSettingsScreens(); const customFieldsApi = fakeCustomFields([companyField, archivedField]); const deleteApi = fakeAdminEndpoint('DELETE', '/members/custom_fields/old_hobby/', {}); - await renderAdminApp('/settings', { boot: customFieldsBoot() }); + await renderAdminApp('/settings', flagOn); await settingsScreen.customFields().getByRole('tab', { name: 'Archived' }).click(); await settingsScreen.customFields().getByTestId('custom-field-list-item').click(); @@ -526,7 +518,7 @@ describe('Custom fields', () => { it('does not expose permanent deletion for an active field', async () => { fakeSettingsScreens(); fakeCustomFields([companyField]); - await renderAdminApp('/settings', { boot: customFieldsBoot() }); + await renderAdminApp('/settings', flagOn); // Deletion lives behind the header menu, and an active field has none — // the UI can't reach delete, matching the API's archived-only rule. @@ -538,7 +530,7 @@ describe('Custom fields', () => { it('shows no tabs at all while no fields exist', async () => { fakeSettingsScreens(); fakeCustomFields([]); - await renderAdminApp('/settings', { boot: customFieldsBoot() }); + await renderAdminApp('/settings', flagOn); await expect.element(settingsScreen.customFields()).toBeVisible(); await expect(settingsScreen.customFields().getByRole('tab')).toHaveCount(0); @@ -550,7 +542,7 @@ describe('Custom fields', () => { const editApi = fakeAdminEndpoint('PUT', '/members/custom_fields/old_hobby/', { members_custom_fields: [{ ...archivedField, status: 'active' }], }); - await renderAdminApp('/settings', { boot: customFieldsBoot() }); + await renderAdminApp('/settings', flagOn); await settingsScreen.customFields().getByRole('tab', { name: 'Archived' }).click(); await settingsScreen.customFields().getByTestId('custom-field-list-item').click(); diff --git a/apps/admin/src/settings/membership/portal.acceptance.test.tsx b/apps/admin/src/settings/membership/portal.acceptance.test.tsx index 384d44a373c..a576a9a1e7f 100644 --- a/apps/admin/src/settings/membership/portal.acceptance.test.tsx +++ b/apps/admin/src/settings/membership/portal.acceptance.test.tsx @@ -108,7 +108,7 @@ describe('Portal settings', () => { it('hides gift promotion settings when the backend does not provide them', async () => { fakeSettingsScreens(); fakeTiers([freeTier]); - const settings = settingsResponse({ labs: { giftSubCustomization: true } }); + const settings = settingsResponse(); settings.settings = settings.settings.filter( ({ key }) => !['portal_signup_gift_promotion', 'portal_account_gift_promotion'].includes(key), ); diff --git a/apps/admin/src/settings/membership/tiers-checkout.acceptance.test.tsx b/apps/admin/src/settings/membership/tiers-checkout.acceptance.test.tsx index 19b7f9e34eb..4957360f34d 100644 --- a/apps/admin/src/settings/membership/tiers-checkout.acceptance.test.tsx +++ b/apps/admin/src/settings/membership/tiers-checkout.acceptance.test.tsx @@ -2,7 +2,6 @@ import { describe, expect, it } from 'vitest'; import { page, userEvent } from 'vitest/browser'; import { - configResponse, fakeAdminEndpoint, fakeMemberCustomFields, fakeSettingsScreens, @@ -36,24 +35,22 @@ const nameField = { type: 'short_text', }; -function stripeSettings(overrides: Parameters[0] = {}) { +function stripeSettings() { return settingsResponse({ - ...overrides, settings: { stripe_connect_display_name: 'Dummy', stripe_connect_livemode: false, stripe_connect_account_id: 'acct_123', stripe_connect_publishable_key: 'pk_test_123', stripe_connect_secret_key: 'sk_test_123', - ...overrides.settings, }, }); } -// The flag lives in settings and config in lockstep, and Stripe rides along in settings. -const flagOnBoot = { - browseConfig: { response: configResponse({ labs: { membersCustomFields: true } }) }, - browseSettings: { response: stripeSettings({ labs: { membersCustomFields: true } }) }, +// The harness composes the flag into settings and config; Stripe rides along in settings. +const flagOn = { + labs: { membersCustomFields: true }, + boot: { browseSettings: { response: stripeSettings() } }, }; const supporterConfig = { @@ -114,7 +111,7 @@ describe('Tier checkout collection', () => { { errors: [{ type: 'NotFoundError', message: 'Resource not found error.' }] }, { status: 404 }, ); - await renderAdminApp('/settings', { boot: flagOnBoot }); + await renderAdminApp('/settings', flagOn); const modal = await openSupporterModal(); await expect(modal.getByText('Checkout', { exact: true })).toHaveCount(0); @@ -131,7 +128,7 @@ describe('Tier checkout collection', () => { { errors: [{ type: 'InternalServerError', message: 'Something went wrong.' }] }, { status: 500 }, ); - await renderAdminApp('/settings', { boot: flagOnBoot }); + await renderAdminApp('/settings', flagOn); const modal = await openSupporterModal(); await expect.element(modal.getByText(/could not be loaded/)).toBeVisible(); @@ -140,7 +137,7 @@ describe('Tier checkout collection', () => { it('shows no checkout section on the free tier', async () => { checkoutWorld(); - await renderAdminApp('/settings', { boot: flagOnBoot }); + await renderAdminApp('/settings', flagOn); await settingsScreen.tiers().getByText(freeTier.name, { exact: true }).click(); const modal = settingsScreen.tierDetailModal(); @@ -150,7 +147,7 @@ describe('Tier checkout collection', () => { it('reflects the saved configuration and writes nothing when untouched', async () => { const putApi = checkoutWorld([supporterConfig]); - await renderAdminApp('/settings', { boot: flagOnBoot }); + await renderAdminApp('/settings', flagOn); const modal = await openSupporterModal(); await expect.element(modal.getByLabelText('Collect shipping address')).toBeChecked(); @@ -182,7 +179,7 @@ describe('Tier checkout collection', () => { const everywhere = { ...supporterConfig.shipping }; delete (everywhere as { allowed_countries?: string[] }).allowed_countries; checkoutWorld([{ ...supporterConfig, shipping: everywhere }]); - await renderAdminApp('/settings', { boot: flagOnBoot }); + await renderAdminApp('/settings', flagOn); const modal = await openSupporterModal(); await expect.element(modal.getByLabelText('Collect shipping address')).toBeChecked(); @@ -192,7 +189,7 @@ describe('Tier checkout collection', () => { it('validates destinations inline before anything is written', async () => { const putApi = checkoutWorld(); - await renderAdminApp('/settings', { boot: flagOnBoot }); + await renderAdminApp('/settings', flagOn); const modal = await openSupporterModal(); await modal.getByLabelText('Collect shipping address').click(); @@ -204,7 +201,7 @@ describe('Tier checkout collection', () => { it('saves the chosen collections, stating every block explicitly', async () => { const putApi = checkoutWorld(); - await renderAdminApp('/settings', { boot: flagOnBoot }); + await renderAdminApp('/settings', flagOn); const modal = await openSupporterModal(); await modal.getByLabelText('Collect shipping address').click(); @@ -259,7 +256,7 @@ describe('Tier checkout collection', () => { }, { status: 422 }, ); - await renderAdminApp('/settings', { boot: flagOnBoot }); + await renderAdminApp('/settings', flagOn); const modal = await openSupporterModal(); await modal.getByLabelText('Collect shipping address').click(); @@ -290,7 +287,7 @@ describe('Tier checkout collection', () => { fields = [...fields, created]; return { members_custom_fields: [created] }; }); - await renderAdminApp('/settings', { boot: flagOnBoot }); + await renderAdminApp('/settings', flagOn); const modal = await openSupporterModal(); await modal.getByLabelText('Collect shipping address').click(); @@ -335,7 +332,7 @@ describe('Tier checkout collection', () => { ], }), ); - await renderAdminApp('/settings', { boot: flagOnBoot }); + await renderAdminApp('/settings', flagOn); await settingsScreen.tiers().getByRole('button', { name: 'Add tier' }).click(); const modal = settingsScreen.tierDetailModal(); @@ -377,7 +374,7 @@ describe('Tier checkout collection', () => { it('closes without confirmation after saving checkout edits', async () => { const putApi = checkoutWorld(); - await renderAdminApp('/settings', { boot: flagOnBoot }); + await renderAdminApp('/settings', flagOn); const modal = await openSupporterModal(); await modal.getByLabelText('Collect business tax ID').click(); @@ -392,7 +389,7 @@ describe('Tier checkout collection', () => { it('asks before discarding unsaved checkout edits', async () => { checkoutWorld(); - await renderAdminApp('/settings', { boot: flagOnBoot }); + await renderAdminApp('/settings', flagOn); const modal = await openSupporterModal(); await modal.getByLabelText('Collect phone number').click(); diff --git a/apps/admin/src/settings/membership/tiers.acceptance.test.tsx b/apps/admin/src/settings/membership/tiers.acceptance.test.tsx index b7ca813dd3f..7da134f5d7d 100644 --- a/apps/admin/src/settings/membership/tiers.acceptance.test.tsx +++ b/apps/admin/src/settings/membership/tiers.acceptance.test.tsx @@ -21,22 +21,20 @@ const supporterTier = tier({ benefits: ['Simple benefit'], }); -function stripeSettings(overrides: Parameters[0] = {}) { +function stripeSettings() { return settingsResponse({ - ...overrides, settings: { stripe_connect_display_name: 'Dummy', stripe_connect_livemode: false, stripe_connect_account_id: 'acct_123', stripe_connect_publishable_key: 'pk_test_123', stripe_connect_secret_key: 'sk_test_123', - ...overrides.settings, }, }); } function withoutSettings(keys: string[]) { - const response = stripeSettings({ labs: { machinePayments: true } }); + const response = stripeSettings(); response.settings = response.settings.filter(({ key }) => !keys.includes(key)); return response; } @@ -341,7 +339,7 @@ describe('Tier settings', () => { fakeTiers([freeTier, supporterTier]); await renderAdminApp('/settings', { labs: { machinePayments: true }, - boot: { browseSettings: { response: stripeSettings({ labs: { machinePayments: true } }) } }, + boot: { browseSettings: { response: stripeSettings() } }, }); await expect diff --git a/apps/admin/test-utils/acceptance/README.md b/apps/admin/test-utils/acceptance/README.md index 45b51f9cda7..921a227c242 100644 --- a/apps/admin/test-utils/acceptance/README.md +++ b/apps/admin/test-utils/acceptance/README.md @@ -46,7 +46,8 @@ When your area calls a new external origin, add it to `EXTERNAL_URL_BLOCKLIST` i The shell requests handled by default (`boot.ts`): `browseSettings`, `browseConfig`, `browseSite`, `browseMe`, `browseMembersCount`, `browseActiveTheme`, `editUserPreferences`. A **boot override** replaces the response of one named entry for one test (the entry's method/path stay fixed): ```ts -// Labs flags (sugar for lockstep settings + config overrides): +// Labs flags (sugar for lockstep settings + config overrides; merges into +// any browseSettings/browseConfig boot override, named flags winning): await renderAdminApp("/tags", {labs: {someFlag: true}}); // Persisted user state, e.g. what's-new preferences: diff --git a/apps/admin/test-utils/acceptance/boot.ts b/apps/admin/test-utils/acceptance/boot.ts index 810dc2d67f7..a307ecf8f4c 100644 --- a/apps/admin/test-utils/acceptance/boot.ts +++ b/apps/admin/test-utils/acceptance/boot.ts @@ -85,6 +85,94 @@ export function defaultBootRoutes(): string[] { return Object.values(defaultBootRequests()).map(({ method, path }) => `${method} ${path}`); } +type LabsFlags = Record; +type BootOverride = NonNullable; + +function isRecord(value: unknown): value is Record { + return typeof value === 'object' && value !== null && !Array.isArray(value); +} + +function parseLabsSettingValue(value: unknown): Record { + if (typeof value !== 'string') { + return {}; + } + try { + const parsed: unknown = JSON.parse(value); + return isRecord(parsed) ? parsed : {}; + } catch { + return {}; + } +} + +// Unrecognized bodies (error envelopes, non-objects) pass through untouched; +// recognized ones are clone-and-merged — never mutate a test-owned object. +function mergeLabsIntoConfigBody(body: unknown, labs: LabsFlags): unknown { + if (!isRecord(body) || !isRecord(body.config)) { + return body; + } + const existing = isRecord(body.config.labs) ? body.config.labs : {}; + return { ...body, config: { ...body.config, labs: { ...existing, ...labs } } }; +} + +function mergeLabsIntoSettingsBody(body: unknown, labs: LabsFlags): unknown { + if (!isRecord(body) || !Array.isArray(body.settings)) { + return body; + } + let found = false; + const settings = body.settings.map((entry: unknown) => { + if (!isRecord(entry) || entry.key !== 'labs') { + return entry; + } + found = true; + return { ...entry, value: JSON.stringify({ ...parseLabsSettingValue(entry.value), ...labs }) }; + }); + if (!found) { + settings.push(...settingsResponse({ labs }).settings.filter(({ key }) => key === 'labs')); + } + return { ...body, settings }; +} + +function withMergedLabs( + override: BootOverride, + merge: (body: unknown) => unknown, + fallback: () => unknown, +): BootOverride { + const { response } = override; + if (response === undefined) { + return { ...override, response: fallback() }; + } + if (typeof response === 'function') { + return { + ...override, + response: async (request: Request) => + merge(await (response as (request: Request) => unknown)(request)), + }; + } + return { ...override, response: merge(response) }; +} + +/** + * The `labs` render option compiled onto the boot overrides: `browseConfig`/ + * `browseSettings` overrides get the flags merged into their responses + * (flags named in `labs` win); absent entries get the canned test-data + * responses with the flags applied. + */ +export function composeLabsBootOverrides(labs: LabsFlags, boot: BootOverrides = {}): BootOverrides { + return { + ...boot, + browseConfig: withMergedLabs( + boot.browseConfig ?? {}, + (body) => mergeLabsIntoConfigBody(body, labs), + () => configResponse({ labs }), + ), + browseSettings: withMergedLabs( + boot.browseSettings ?? {}, + (body) => mergeLabsIntoSettingsBody(body, labs), + () => settingsResponse({ labs }), + ), + }; +} + function matches(config: BootRequestConfig, method: string, apiPath: string): boolean { if (config.method !== method) { return false; diff --git a/apps/admin/test-utils/acceptance/render-admin-app.tsx b/apps/admin/test-utils/acceptance/render-admin-app.tsx index ec80dbe5e63..39da1e62f8c 100644 --- a/apps/admin/test-utils/acceptance/render-admin-app.tsx +++ b/apps/admin/test-utils/acceptance/render-admin-app.tsx @@ -1,20 +1,23 @@ import { QueryClient } from '@tanstack/react-query'; import { render } from 'vitest-browser-react'; -import { configResponse, settingsResponse } from '@tryghost/test-data'; import { defaultUnsplashConfig, type TopLevelFrameworkProps } from '@tryghost/admin-x-framework'; import '@/index.css'; import { AdminAppRoot } from '@/app-root'; -import { installBootOverrides, type BootOverrides } from './boot'; +import { composeLabsBootOverrides, installBootOverrides, type BootOverrides } from './boot'; export interface RenderAdminAppOptions { /** * Labs flags for this test; compiles to lockstep settings + config boot - * overrides (the admin client reads labs from both). + * overrides (the admin client reads labs from both). Merges into any + * `boot` override for those entries; flags named here win. */ labs?: Record; - /** Boot-table overrides keyed by entry name (see boot.ts); wins over `labs`. */ + /** + * Boot-table overrides keyed by entry name (see boot.ts); `labs` flags + * merge into `browseConfig`/`browseSettings` override responses. + */ boot?: BootOverrides; } @@ -36,13 +39,7 @@ export async function renderAdminApp( route: string = '/', { labs, boot }: RenderAdminAppOptions = {}, ): Promise>> { - const overrides: BootOverrides = { - ...(labs && { - browseSettings: { response: settingsResponse({ labs }) }, - browseConfig: { response: configResponse({ labs }) }, - }), - ...boot, - }; + const overrides: BootOverrides = labs ? composeLabsBootOverrides(labs, boot) : { ...boot }; if (Object.keys(overrides).length > 0) { installBootOverrides(overrides); diff --git a/docs/contributing/testing.md b/docs/contributing/testing.md index 3cdaceab4bd..7a2b6d141ca 100644 --- a/docs/contributing/testing.md +++ b/docs/contributing/testing.md @@ -99,7 +99,8 @@ cd ghost/core pnpm exec vitest -c vitest.config.db.ts test/integration/path/to/test.test.js ``` -Ghost Core's database-backed suites use SQLite by default locally. Tests for +Ghost Core's database-backed suites use MySQL locally and in CI. Start the +development services with `pnpm dev` before running them locally. Tests for optional Redis and object-storage adapters skip when their services are not available; start the relevant development services when you need to exercise those adapters. diff --git a/docs/practices/database-migrations.md b/docs/practices/database-migrations.md index c27356a16f8..a7a008d6750 100644 --- a/docs/practices/database-migrations.md +++ b/docs/practices/database-migrations.md @@ -86,8 +86,9 @@ state that existed before `up()`. ### Testing -Test migrations against both MySQL and SQLite because Knex can behave -differently between database clients. +The database-backed migration suites run against MySQL, Ghost's supported +production database. Changes to retained SQLite compatibility paths should add +focused unit coverage for that dialect. Run the schema integrity test after changing `schema.js`, fixtures, default settings, or default routes. Update only the expected hash for the change you @@ -107,8 +108,7 @@ pnpm test:single test/integration/migrations/migration.test.js ``` Add focused tests for any non-trivial transformation, then run the affected Core -tests. CI runs the database-backed Core suites with both MySQL and SQLite -because Knex and the database engines do not always behave identically. +tests. CI runs the database-backed Core suites against MySQL. ### Reviewing diff --git a/e2e/helpers/pages/admin/tags/tag-details-page.ts b/e2e/helpers/pages/admin/tags/tag-details-page.ts index e8a9c2b7f35..df1a5e80892 100644 --- a/e2e/helpers/pages/admin/tags/tag-details-page.ts +++ b/e2e/helpers/pages/admin/tags/tag-details-page.ts @@ -1,7 +1,6 @@ import { AdminPage } from '@/admin-pages'; import { Locator, Page } from '@playwright/test'; import { - deleteTagMenuItem, descriptionFieldLabel, nameFieldLabel, slugFieldLabel, @@ -14,7 +13,6 @@ export class TagDetailsPage extends AdminPage { readonly descriptionInput: Locator; readonly saveButton: Locator; readonly saveButtonSuccess: Locator; - readonly deleteButton: Locator; readonly backLink: Locator; constructor(page: Page) { @@ -25,7 +23,6 @@ export class TagDetailsPage extends AdminPage { this.descriptionInput = page.getByRole('textbox', { name: descriptionFieldLabel }); this.saveButton = page.getByRole('button', { name: 'Save' }); this.saveButtonSuccess = page.getByRole('button', { name: 'Saved' }); - this.deleteButton = page.getByRole('button', { name: deleteTagMenuItem }); this.backLink = page.locator(`[data-test-link="${tagsBackLink}"]`); } diff --git a/e2e/helpers/pages/admin/tags/tag-editor-page.ts b/e2e/helpers/pages/admin/tags/tag-editor-page.ts index cbd0f9c404c..68dddcd5a7d 100644 --- a/e2e/helpers/pages/admin/tags/tag-editor-page.ts +++ b/e2e/helpers/pages/admin/tags/tag-editor-page.ts @@ -46,13 +46,8 @@ export class TagEditorPage extends TagDetailsPage { } async deleteTag() { - if (await this.tagActionsButton.isVisible()) { - await this.tagActionsButton.click(); - await this.deleteMenuItem.click(); - return; - } - - await this.deleteButton.click(); + await this.tagActionsButton.click(); + await this.deleteMenuItem.click(); } async confirmDelete() { diff --git a/e2e/tests/admin/tags/tag-detail.test.ts b/e2e/tests/admin/tags/tag-detail.test.ts index 8aa74e6d208..d79865dc231 100644 --- a/e2e/tests/admin/tags/tag-detail.test.ts +++ b/e2e/tests/admin/tags/tag-detail.test.ts @@ -4,85 +4,78 @@ import { expect, test } from '@/helpers/playwright'; import { usePerTestIsolation } from '@/helpers/playwright/isolation'; /** - * The tag detail screen exists twice while the Ember-to-React migration is in - * flight, behind the `tagDetailsReact` Labs flag. The same journeys run - * against both implementations — a test may not branch on the implementation; - * a failure under only one flag state is a real user-facing difference. + * Behaviour contract for `/tags/:slug` and `/tags/new`. The React screen is + * generally available (`tagDetailsReact` sits in GA_FEATURES), so every site + * serves it; the assertions describe what the screen does rather than how it + * is built. */ usePerTestIsolation(); -for (const { implementation, tagDetailsReact } of [ - { implementation: 'Ember', tagDetailsReact: false }, - { implementation: 'React', tagDetailsReact: true }, -] as const) { - test.describe(`Ghost Admin - Tag Detail (${implementation})`, () => { - test.use({ labs: { tagDetailsReact } }); +test.describe('Ghost Admin - Tag Detail', () => { + let tagFactory: TagFactory; - let tagFactory: TagFactory; + test.beforeEach(({ page }) => { + tagFactory = createTagFactory(page.request); + }); - test.beforeEach(({ page }) => { - tagFactory = createTagFactory(page.request); + test('opening a tag from the list and editing it - changes are saved', async ({ page }) => { + const tag = await tagFactory.create({ + name: 'Getting Started', + slug: 'getting-started', + feature_image: null, }); + const tagsPage = new TagsPage(page); + const tagEditor = new TagEditorPage(page); + + await tagsPage.goto(); + await tagsPage.waitForPageToFullyLoad(); + await tagsPage.getTagLinkByName(tag.name).click(); + await expect(tagEditor.nameInput).toHaveValue('Getting Started'); + await expect(tagEditor.slugInput).toHaveValue('getting-started'); + + await tagEditor.updateTag('Getting Started Updated', 'getting-started-updated'); + await tagEditor.goBackToTagsList(); + await tagsPage.waitForPageToFullyLoad(); + + await expect(tagsPage.getTagLinkByName('Getting Started Updated')).toBeVisible(); + await expect(tagsPage.getTagLinkByName('Getting Started Updated')).toContainText( + 'getting-started-updated', + ); + }); - test('opening a tag from the list and editing it - changes are saved', async ({ page }) => { - const tag = await tagFactory.create({ - name: 'Getting Started', - slug: 'getting-started', - feature_image: null, - }); - const tagsPage = new TagsPage(page); - const tagEditor = new TagEditorPage(page); - - await tagsPage.goto(); - await tagsPage.waitForPageToFullyLoad(); - await tagsPage.getTagLinkByName(tag.name).click(); - await expect(tagEditor.nameInput).toHaveValue('Getting Started'); - await expect(tagEditor.slugInput).toHaveValue('getting-started'); - - await tagEditor.updateTag('Getting Started Updated', 'getting-started-updated'); - await tagEditor.goBackToTagsList(); - await tagsPage.waitForPageToFullyLoad(); + test('creating a tag via the new tag screen - appears in the tags list', async ({ page }) => { + const newTagsPage = new NewTagsPage(page); + const tagsPage = new TagsPage(page); - await expect(tagsPage.getTagLinkByName('Getting Started Updated')).toBeVisible(); - await expect(tagsPage.getTagLinkByName('Getting Started Updated')).toContainText( - 'getting-started-updated', - ); - }); + await newTagsPage.goto(); + await newTagsPage.createTag('Fresh Tag', 'fresh-tag'); + await newTagsPage.goBackToTagsList(); + await tagsPage.waitForPageToFullyLoad(); - test('creating a tag via the new tag screen - appears in the tags list', async ({ page }) => { - const newTagsPage = new NewTagsPage(page); - const tagsPage = new TagsPage(page); - - await newTagsPage.goto(); - await newTagsPage.createTag('Fresh Tag', 'fresh-tag'); - await newTagsPage.goBackToTagsList(); - await tagsPage.waitForPageToFullyLoad(); + await expect(tagsPage.getTagLinkByName('Fresh Tag')).toBeVisible(); + await expect(tagsPage.getTagLinkByName('Fresh Tag')).toContainText('fresh-tag'); + }); - await expect(tagsPage.getTagLinkByName('Fresh Tag')).toBeVisible(); - await expect(tagsPage.getTagLinkByName('Fresh Tag')).toContainText('fresh-tag'); + test('deleting a tag from the detail screen - removed from the tags list', async ({ page }) => { + const tag = await tagFactory.create({ + name: 'Disposable', + slug: 'disposable', + feature_image: null, }); + const tagsPage = new TagsPage(page); + const tagEditor = new TagEditorPage(page); - test('deleting a tag from the detail screen - removed from the tags list', async ({ page }) => { - const tag = await tagFactory.create({ - name: 'Disposable', - slug: 'disposable', - feature_image: null, - }); - const tagsPage = new TagsPage(page); - const tagEditor = new TagEditorPage(page); + await tagEditor.gotoTagBySlug(tag.slug); + await expect(tagEditor.nameInput).toHaveValue('Disposable'); - await tagEditor.gotoTagBySlug(tag.slug); - await expect(tagEditor.nameInput).toHaveValue('Disposable'); + await tagEditor.deleteTag(); + await expect(tagEditor.deleteModal).toBeVisible(); + await tagEditor.confirmDelete(); - await tagEditor.deleteTag(); - await expect(tagEditor.deleteModal).toBeVisible(); - await tagEditor.confirmDelete(); - - await expect(tagEditor.deleteModal).toBeHidden(); - await expect(page).toHaveURL(tagsPage.pageUrl); - await tagsPage.waitForPageToFullyLoad(); - await expect(tagsPage.getTagLinkByName('Disposable')).toBeHidden(); - }); + await expect(tagEditor.deleteModal).toBeHidden(); + await expect(page).toHaveURL(tagsPage.pageUrl); + await tagsPage.waitForPageToFullyLoad(); + await expect(tagsPage.getTagLinkByName('Disposable')).toBeHidden(); }); -} +}); diff --git a/ghost/core/core/boot.js b/ghost/core/core/boot.js index 8e184506ba6..a5f753104a5 100644 --- a/ghost/core/core/boot.js +++ b/ghost/core/core/boot.js @@ -327,7 +327,7 @@ async function initAppService() { * These services should all be part of core, frontend services should be loaded with the frontend * We are working towards this being a service loader, with the ability to make certain services optional */ -async function initServices({ ghostServer, config, prometheusClient }) { +async function initServices({ ghostServer, config, prometheusClient, jobsService }) { debug('Begin: initServices'); debug('Begin: Services'); @@ -393,7 +393,7 @@ async function initServices({ ghostServer, config, prometheusClient }) { await Promise.all([ identityTokens.init(), memberAttribution.init(), - mentionsService.init(), + mentionsService.init({ jobsService }), staffService.init(), members.init(), tiers.init(), @@ -502,7 +502,7 @@ async function initBackgroundServices({ config }) { // service fails. try { const memberJobs = require('./server/services/members/jobs'); - await memberJobs.scheduleTokenCleanupJob(); + await memberJobs.scheduleTokenCleanupJob(jobsService); } catch (err) { const logging = require('@tryghost/logging'); logging.error(err); @@ -510,7 +510,7 @@ async function initBackgroundServices({ config }) { try { const memberJobs = require('./server/services/members/jobs'); - await memberJobs.scheduleExpiredCompCleanupJob(); + await memberJobs.scheduleExpiredCompCleanupJob(jobsService); } catch (err) { const logging = require('@tryghost/logging'); logging.error(err); @@ -670,25 +670,27 @@ async function bootGhost({ backend = true, frontend = true, server = true } = {} await initAppService(); } - await initServices({ ghostServer, config, prometheusClient }); + const jobsService = require('./server/services/jobs-service').init(); + + await initServices({ ghostServer, config, prometheusClient, jobsService }); debug('Begin: Register job handlers'); - const jobsServiceWrapper = require('./server/services/jobs-service'); + const assert = require('node:assert/strict'); const registerJobHandlers = require('./server/services/jobs-service/register-job-handlers').default; const mediaInliner = require('./server/services/media-inliner'); - const db = require('./server/data/db'); - const models = require('./server/models'); - const events = require('./server/lib/common/events'); - const jobsService = jobsServiceWrapper.init(); + const gifts = require('./server/services/gifts'); + const memberJobs = require('./server/services/members/jobs'); + const mentionsService = require('./server/services/mentions'); + memberJobs.init(); + assert(gifts.service, 'Gift service should be initialized'); + assert(mentionsService.controller, 'Mentions controller should be initialized'); registerJobHandlers({ jobsService, - db, - logging, - models, - events, - sentry, + memberJobs, + giftService: gifts.service, mediaInliner: mediaInliner.getInstance(), + mentionsController: mentionsService.controller, }); await jobsService.start(); debug('End: Register job handlers'); diff --git a/ghost/core/core/server/lib/image/image-size.js b/ghost/core/core/server/lib/image/image-size.js index b0ab6291103..bcdae3bd647 100644 --- a/ghost/core/core/server/lib/image/image-size.js +++ b/ghost/core/core/server/lib/image/image-size.js @@ -31,6 +31,12 @@ class ImageSize { }, response_timeout: this.config.get('times:getImageSizeTimeoutInMS') || 10000, }; + + // Coalesces concurrent getImageSizeFromUrl() calls for the same URL so we only + // ever issue one underlying request/read per URL at a time. A single post's + // coverImage/ogImage/twitterImage often resolve to the same feature_image URL + // and are looked up concurrently, which otherwise fans out into duplicate requests. + this.inFlightImageSizeLookups = new Map(); } // processes the Buffer result of an image file using image-size @@ -153,6 +159,24 @@ class ImageSize { * @returns {Promise} imageObject or error */ getImageSizeFromUrl(imagePath) { + if (this.inFlightImageSizeLookups.has(imagePath)) { + return this.inFlightImageSizeLookups.get(imagePath); + } + + const promise = this._getImageSizeFromUrl(imagePath).finally(() => { + this.inFlightImageSizeLookups.delete(imagePath); + }); + this.inFlightImageSizeLookups.set(imagePath, promise); + + return promise; + } + + /** + * @description read image dimensions from URL, without in-flight deduplication + * @param {string} imagePath as URL + * @returns {Promise} imageObject or error + */ + _getImageSizeFromUrl(imagePath) { if (this.storageUtils.isLocalImage(imagePath)) { // don't make a request for a locally stored image return this.getImageSizeFromStoragePath(imagePath); diff --git a/ghost/core/core/server/services/content-import/import/completion-email.ts b/ghost/core/core/server/services/content-import/import/completion-email.ts index 95de987e550..1f560492da7 100644 --- a/ghost/core/core/server/services/content-import/import/completion-email.ts +++ b/ghost/core/core/server/services/content-import/import/completion-email.ts @@ -63,8 +63,8 @@ function formatNumber(value: number): string { return value.toLocaleString(); } -function summaryItems(counts: OutcomeCounts): string { - const items = [ +function summaryItems(counts: OutcomeCounts, hasErrorsFile: boolean): string { + const items: Array<[string, number]> = [ ['Created', counts.created], ['Updated', counts.updated], ['Skipped', counts.skipped], @@ -72,10 +72,11 @@ function summaryItems(counts: OutcomeCounts): string { ]; return items - .map( - ([label, count]) => - `
  • ${label}: ${formatNumber(count as number)}
    • `, - ) + .map(([label, count]) => { + const attachmentCopy = + label === 'Failed' && hasErrorsFile ? ' (see attached errors.csv)' : ''; + return `
  • ${label}: ${formatNumber(count)}${attachmentCopy}
    • `; + }) .join(''); } @@ -130,7 +131,12 @@ function importedPostLinks(run: ImportRun, adminUrl: string): string {

    ${filteredLinks}

    `; } -function renderCompletionEmail(run: ImportRun, recipient: string, adminUrl: string): string { +function renderCompletionEmail( + run: ImportRun, + recipient: string, + adminUrl: string, + hasErrorsFile: boolean, +): string { const counts = countsFor(run); const heading = headingFor(run, counts); const warningCopy = counts.warningRows @@ -155,7 +161,7 @@ function renderCompletionEmail(run: ImportRun, recipient: string, adminUrl: stri

    ${heading}

    ${failureCopy}

    The import processed ${formatNumber(run.total)} ${run.total === 1 ? 'row' : 'rows'}:

    -
      ${summaryItems(counts)}
    +
      ${summaryItems(counts, hasErrorsFile)}
    ${warningCopy} ${importedPostLinks(run, adminUrl)}

    This email was sent to ${escapedRecipient}.

    @@ -192,7 +198,7 @@ export default function buildCompletionEmail( return { to: recipient, subject: headingFor(run, counts), - html: renderCompletionEmail(run, recipient, adminUrl), + html: renderCompletionEmail(run, recipient, adminUrl, Boolean(errorsFile)), forceTextContent: true, attachments, }; diff --git a/ghost/core/core/server/services/content-import/import/errors-file.ts b/ghost/core/core/server/services/content-import/import/errors-file.ts index 7d727d96b3e..3e90ddd4d8f 100644 --- a/ghost/core/core/server/services/content-import/import/errors-file.ts +++ b/ghost/core/core/server/services/content-import/import/errors-file.ts @@ -28,7 +28,7 @@ export default function buildErrorsFile(run: ImportRun): string | undefined { const [outcomeColumn, reasonColumn, mediaFailuresColumn] = ANNOTATION_NAMES.map((name) => uniqueColumnName(name, usedColumns), ); - const columns = [outcomeColumn, ...run.sourceColumns, reasonColumn, mediaFailuresColumn]; + const columns = [outcomeColumn, reasonColumn, mediaFailuresColumn, ...run.sourceColumns]; return serialize( rows.map((row) => { diff --git a/ghost/core/core/server/services/content-import/import/post-data.ts b/ghost/core/core/server/services/content-import/import/post-data.ts index 4265676185d..b4a242bf37d 100644 --- a/ghost/core/core/server/services/content-import/import/post-data.ts +++ b/ghost/core/core/server/services/content-import/import/post-data.ts @@ -26,7 +26,6 @@ export interface PostsMetaData { twitter_image?: string; twitter_title?: string; twitter_description?: string; - frontmatter?: string; } // The values handed to models.Post.add. Content is lexical only: under @@ -78,7 +77,6 @@ const META_FIELDS = [ 'twitter_image', 'twitter_title', 'twitter_description', - 'frontmatter', ] as const; export default function buildPostData( diff --git a/ghost/core/core/server/services/content-import/import/row.ts b/ghost/core/core/server/services/content-import/import/row.ts index 6eabd975872..208ead72805 100644 --- a/ghost/core/core/server/services/content-import/import/row.ts +++ b/ghost/core/core/server/services/content-import/import/row.ts @@ -34,7 +34,6 @@ export const EDITORIAL_POST_FIELDS = [ 'custom_template', 'codeinjection_head', 'codeinjection_foot', - 'frontmatter', ] as const; // An empty cell (or the literal 'undefined') reads as absent, not as a value. @@ -82,7 +81,6 @@ export const postImportRowSchema = z custom_template: optionalCell, codeinjection_head: optionalCell, codeinjection_foot: optionalCell, - frontmatter: optionalCell, }) .loose(); diff --git a/ghost/core/core/server/services/gifts/jobs/index.js b/ghost/core/core/server/services/gifts/jobs/index.js index 31aaddd3d2c..b29e09230d0 100644 --- a/ghost/core/core/server/services/gifts/jobs/index.js +++ b/ghost/core/core/server/services/gifts/jobs/index.js @@ -1,6 +1,6 @@ const path = require('path'); const logging = require('@tryghost/logging'); -const jobsService = require('../../jobs'); +const jobManager = require('../../jobs'); const CleanGiftsJob = require('./clean-gifts-job').default; let hasScheduled = { @@ -31,7 +31,7 @@ function scheduleJob(key, name, jobFile) { const at = randomOffPeakDailyCron(); logging.info(`[Background Job] ${name} scheduled at ${at}`); - jobsService.addJob({ + jobManager.addJob({ at, job: path.resolve(__dirname, jobFile), name, @@ -43,14 +43,14 @@ function scheduleJob(key, name, jobFile) { } module.exports = { - async scheduleGiftCleanupJob(classBasedJobs) { + async scheduleGiftCleanupJob(jobsService) { if (alreadyScheduledOrTest('cleanup')) { return; } const cron = randomOffPeakDailyCron(); logging.info(`[Background Job] clean-gifts scheduled at ${cron}`); - await classBasedJobs.scheduleRecurring(new CleanGiftsJob(), { cron }); + await jobsService.scheduleRecurring(new CleanGiftsJob(), { cron }); hasScheduled.cleanup = true; }, diff --git a/ghost/core/core/server/services/jobs-service/register-job-handlers.ts b/ghost/core/core/server/services/jobs-service/register-job-handlers.ts index eab6405e1c3..19640814866 100644 --- a/ghost/core/core/server/services/jobs-service/register-job-handlers.ts +++ b/ghost/core/core/server/services/jobs-service/register-job-handlers.ts @@ -1,62 +1,46 @@ -import errors from '@tryghost/errors'; import { JobsService } from './jobs-service'; +import type { GiftService } from '../gifts/gift-service'; import CleanTokensJob from '../members/jobs/clean-tokens-job'; -import cleanTokens from '../members/jobs/clean-tokens-task'; import CleanExpiredCompedJob from '../members/jobs/clean-expired-comped-job'; -import cleanExpiredComped from '../members/jobs/clean-expired-comped-task'; -import * as gifts from '../gifts'; import CleanGiftsJob from '../gifts/jobs/clean-gifts-job'; import ExternalMediaInliner from '../media-inliner/external-media-inliner'; import ExternalMediaInlinerJob from '../media-inliner/external-media-inliner-job'; import ContentCSVImportJob from '../content-import/jobs/content-csv-import-job'; import * as contentImport from '../content-import'; import UpdateCheckJob from '../update-check/jobs/update-check-job'; +import type MentionController from '../mentions/mention-controller'; +import ProcessWebmentionJob from '../mentions/process-webmention-job'; const updateCheck = require('../update-check'); interface RegisterJobHandlersDependencies { jobsService: JobsService; - db: typeof import('../../data/db'); - logging: typeof import('@tryghost/logging'); - // Structural types: models, lib/common/events and shared/sentry are - // untyped CommonJS modules, so name just the surface the handlers consume. - models: { - Member: { - findOne( - data: Record, - options: Record, - ): Promise<{ attributes: Record }>; - }; + memberJobs: { + cleanTokens(): Promise; + cleanExpiredComped(): Promise; }; - events: { emit(name: string, model: unknown, options: Record): void }; - sentry: { captureException(err: unknown): void }; + giftService: GiftService; mediaInliner: ExternalMediaInliner; + mentionsController: MentionController; } export default function registerJobHandlers({ jobsService, - db, - logging, - models, - events, - sentry, + memberJobs, + giftService, mediaInliner, + mentionsController, }: RegisterJobHandlersDependencies): void { jobsService.handle(CleanTokensJob, async () => { - await cleanTokens({ db, logging }); + await memberJobs.cleanTokens(); }); jobsService.handle(CleanExpiredCompedJob, async () => { - await cleanExpiredComped({ db, models, events, logging, sentry }); + await memberJobs.cleanExpiredComped(); }); jobsService.handle(CleanGiftsJob, async () => { - if (!gifts.service) { - throw new errors.IncorrectUsageError({ - message: 'clean-gifts ran before the gifts service was initialised', - }); - } - await gifts.service.cleanup(); + await giftService.cleanup(); }); jobsService.handle(ExternalMediaInlinerJob, async (job) => { @@ -70,4 +54,8 @@ export default function registerJobHandlers({ jobsService.handle(UpdateCheckJob, async () => { await updateCheck({ rethrowErrors: true }); }); + + jobsService.handle(ProcessWebmentionJob, async (job) => { + await mentionsController.processWebmention(job); + }); } diff --git a/ghost/core/core/server/services/members/jobs/index.js b/ghost/core/core/server/services/members/jobs/index.js index 617a0ffd5d1..33b89911518 100644 --- a/ghost/core/core/server/services/members/jobs/index.js +++ b/ghost/core/core/server/services/members/jobs/index.js @@ -1,12 +1,17 @@ const logging = require('@tryghost/logging'); +const errors = require('@tryghost/errors'); const CleanTokensJob = require('./clean-tokens-job').default; const CleanExpiredCompedJob = require('./clean-expired-comped-job').default; +const cleanTokensTask = require('./clean-tokens-task').default; +const cleanExpiredCompedTask = require('./clean-expired-comped-task').default; let hasScheduled = { expiredComped: false, tokens: false, }; +let tasks; + function alreadyScheduledOrTest(key) { return hasScheduled[key] || process.env.NODE_ENV.startsWith('test'); } @@ -18,30 +23,63 @@ function randomDailyCron(maxHour = 24) { return `${s} ${m} ${h} * * *`; } +function getTasks() { + if (!tasks) { + throw new errors.IncorrectUsageError({ + message: 'Member jobs used before init(). Call init() from boot first.', + }); + } + return tasks; +} + module.exports = { - async scheduleExpiredCompCleanupJob() { + // Composition root for the member cleanup tasks. Idempotent because tests + // may boot more than once per process. + init() { + if (tasks) { + return; + } + + const db = require('../../../data/db'); + const models = require('../../../models'); + const events = require('../../../lib/common/events'); + const sentry = require('../../../../shared/sentry'); + + tasks = { + cleanTokens: () => cleanTokensTask({ db, logging }), + cleanExpiredComped: () => cleanExpiredCompedTask({ db, models, events, logging, sentry }), + }; + }, + + cleanTokens() { + return getTasks().cleanTokens(); + }, + + cleanExpiredComped() { + return getTasks().cleanExpiredComped(); + }, + + async scheduleExpiredCompCleanupJob(jobsService) { if (alreadyScheduledOrTest('expiredComped')) { return; } - const classBasedJobs = require('../../jobs-service').getInstance(); // Keep the legacy off-peak window: a random time between 00:00 and 05:59 const cron = randomDailyCron(6); logging.info(`[Background Job] clean-expired-comped scheduled at ${cron}`); - await classBasedJobs.scheduleRecurring(new CleanExpiredCompedJob(), { cron }); + await jobsService.scheduleRecurring(new CleanExpiredCompedJob(), { cron }); hasScheduled.expiredComped = true; }, - async scheduleTokenCleanupJob() { + async scheduleTokenCleanupJob(jobsService) { if (alreadyScheduledOrTest('tokens')) { return; } - const classBasedJobs = require('../../jobs-service').getInstance(); const cron = randomDailyCron(); logging.info(`[Background Job] clean-tokens scheduled at ${cron}`); - await classBasedJobs.scheduleRecurring(new CleanTokensJob(), { cron }); + await jobsService.scheduleRecurring(new CleanTokensJob(), { cron }); hasScheduled.tokens = true; }, diff --git a/ghost/core/core/server/services/mentions/mention-controller.d.ts b/ghost/core/core/server/services/mentions/mention-controller.d.ts new file mode 100644 index 00000000000..1df8b77caf8 --- /dev/null +++ b/ghost/core/core/server/services/mentions/mention-controller.d.ts @@ -0,0 +1,7 @@ +import type ProcessWebmentionJob from './process-webmention-job'; + +declare class MentionController { + processWebmention(job: ProcessWebmentionJob): Promise; +} + +export = MentionController; diff --git a/ghost/core/core/server/services/mentions/mention-controller.js b/ghost/core/core/server/services/mentions/mention-controller.js index a7fd3fcaef3..00d68752578 100644 --- a/ghost/core/core/server/services/mentions/mention-controller.js +++ b/ghost/core/core/server/services/mentions/mention-controller.js @@ -1,4 +1,5 @@ const logging = require('@tryghost/logging'); +const ProcessWebmentionJob = require('./process-webmention-job').default; /** * @typedef {import('./mentions-api')} MentionsAPI @@ -22,11 +23,6 @@ const logging = require('@tryghost/logging'); * @prop {Resource} resource */ -/** - * @typedef {object} IJobService - * @prop {(name: string, fn: Function) => void} addJob - */ - /** * @typedef {object} IMentionResourceService * @prop {(id: import('bson-objectid').default) => Promise} getByID @@ -36,8 +32,8 @@ module.exports = class MentionController { /** @type {import('./mentions-api')} */ #api; - /** @type {IJobService} */ - #jobService; + /** @type {import('../jobs-service/jobs-service').JobsService} */ + #jobsService; /** @type {IMentionResourceService} */ #mentionResourceService; @@ -45,12 +41,12 @@ module.exports = class MentionController { /** * @param {object} deps * @param {import('./mentions-api')} deps.api - * @param {IJobService} deps.jobService + * @param {import('../jobs-service/jobs-service').JobsService} deps.jobsService * @param {IMentionResourceService} deps.mentionResourceService */ async init(deps) { this.#api = deps.api; - this.#jobService = deps.jobService; + this.#jobsService = deps.jobsService; this.#mentionResourceService = deps.mentionResourceService; } @@ -127,17 +123,23 @@ module.exports = class MentionController { */ async receive(frame) { logging.info('[Webmention] ' + JSON.stringify(frame.data)); - this.#jobService.addJob('processWebmention', async () => { - const { source, target, ...payload } = frame.data; - try { - await this.#api.processWebmention({ - source: new URL(source), - target: new URL(target), - payload, - }); - } catch (err) { - logging.error(err, '[Webmention] Failed processing webmention'); - } - }); + const { source, target, ...payload } = frame.data; + await this.#jobsService.dispatch(new ProcessWebmentionJob({ source, target, payload })); + } + + /** + * @param {import('./process-webmention-job').default} job + * @returns {Promise} + */ + async processWebmention({ source, target, payload }) { + try { + await this.#api.processWebmention({ + source: new URL(source), + target: new URL(target), + payload, + }); + } catch (err) { + logging.error(err, '[Webmention] Failed processing webmention'); + } } }; diff --git a/ghost/core/core/server/services/mentions/process-webmention-job.ts b/ghost/core/core/server/services/mentions/process-webmention-job.ts new file mode 100644 index 00000000000..ed2e2c37b39 --- /dev/null +++ b/ghost/core/core/server/services/mentions/process-webmention-job.ts @@ -0,0 +1,26 @@ +import { Job } from '../jobs-service/job'; + +export default class ProcessWebmentionJob extends Job { + static type = 'process-webmention'; + + readonly source: string; + + readonly target: string; + + readonly payload: Record; + + constructor({ + source, + target, + payload, + }: { + source: string; + target: string; + payload: Record; + }) { + super(); + this.source = source; + this.target = target; + this.payload = payload; + } +} diff --git a/ghost/core/core/server/services/mentions/service.js b/ghost/core/core/server/services/mentions/service.js index 989388bf791..f8fad73415e 100644 --- a/ghost/core/core/server/services/mentions/service.js +++ b/ghost/core/core/server/services/mentions/service.js @@ -15,7 +15,7 @@ const urlService = require('../url'); const settingsCache = require('../../../shared/settings-cache'); const DomainEvents = require('@tryghost/domain-events'); const logging = require('@tryghost/logging'); -const jobsService = require('../mentions-jobs'); +const mentionsJobService = require('../mentions-jobs'); // Serializes a post model to the data the URL service needs, loading the // relations it reads for filtered collections (event-emitted models don't @@ -45,7 +45,7 @@ function makeLoggingJobService() { return { async addJob(name, fn) { logging.info(`[Background Job] ${name} queued`); - jobsService.addJob({ + mentionsJobService.addJob({ name, job: async () => { const startedAt = Date.now(); @@ -75,7 +75,11 @@ module.exports = { /** @type {import('./mention-sending-service')} */ sendingService: null, didInit: false, - async init() { + /** + * @param {object} deps + * @param {import('../jobs-service/jobs-service').JobsService} deps.jobsService + */ + async init({ jobsService }) { if (this.didInit) { return; } @@ -110,7 +114,7 @@ module.exports = { this.controller.init({ api, - jobService: makeLoggingJobService(), + jobsService, mentionResourceService: { async getByID(id) { if (!id) { diff --git a/ghost/core/core/shared/config/env/config.testing-mysql.json b/ghost/core/core/shared/config/env/config.testing-mysql.json index f94fe1f16a5..c9e783c92d4 100644 --- a/ghost/core/core/shared/config/env/config.testing-mysql.json +++ b/ghost/core/core/shared/config/env/config.testing-mysql.json @@ -11,7 +11,7 @@ "connection": { "host": "127.0.0.1", "user": "root", - "password": "", + "password": "root", "database": "ghost_testing" } }, diff --git a/ghost/core/core/shared/labs.js b/ghost/core/core/shared/labs.js index 41aa37fe97b..74767ccb4c1 100644 --- a/ghost/core/core/shared/labs.js +++ b/ghost/core/core/shared/labs.js @@ -27,7 +27,7 @@ const messages = { }; // flags in this list always return `true`, allows quick global enable prior to full flag removal -const GA_FEATURES = ['automationAnalytics', 'giftSubCustomization']; +const GA_FEATURES = ['automationAnalytics', 'giftSubCustomization', 'tagDetailsReact']; // These features are considered publicly available and can be enabled/disabled by users const PUBLIC_BETA_FEATURES = [ @@ -54,7 +54,6 @@ const PRIVATE_FEATURES = [ 'membersCustomFields', 'membersImportRedesign', 'paywallImprovements', - 'tagDetailsReact', 'selfServeArchives', 'machinePayments', 'postsListReact', diff --git a/ghost/core/package.json b/ghost/core/package.json index 142330480dd..9b02750a826 100644 --- a/ghost/core/package.json +++ b/ghost/core/package.json @@ -387,12 +387,6 @@ "{projectRoot}/coverage-e2e" ] }, - "test:ci:legacy": { - "dependsOn": [ - "build:assets", - "^build" - ] - }, "test:ci:integration": { "dependsOn": [ "build:assets", diff --git a/ghost/core/test/e2e-api/admin/__snapshots__/config.test.js.snap b/ghost/core/test/e2e-api/admin/__snapshots__/config.test.js.snap index 1a081d1865e..dea3ce5e03a 100644 --- a/ghost/core/test/e2e-api/admin/__snapshots__/config.test.js.snap +++ b/ghost/core/test/e2e-api/admin/__snapshots__/config.test.js.snap @@ -4,10 +4,10 @@ exports[`Config API As Owner Can retrieve config and all expected properties 1: Object { "config": Object { "clientExtensions": Object {}, - "database": StringMatching /sqlite3\\|mysql\\|mysql2/, + "database": "mysql8", "emailAnalytics": true, "enableDeveloperExperiments": false, - "environment": StringMatching /\\^testing/, + "environment": "testing-mysql", "klipy": Object { "apiKey": null, "contentFilter": "off", diff --git a/ghost/core/test/e2e-api/admin/config.test.js b/ghost/core/test/e2e-api/admin/config.test.js index 3890c989890..b8a6123f207 100644 --- a/ghost/core/test/e2e-api/admin/config.test.js +++ b/ghost/core/test/e2e-api/admin/config.test.js @@ -41,8 +41,8 @@ describe('Config API', function () { .expectStatus(200) .matchBodySnapshot({ config: { - database: stringMatching(/sqlite3|mysql|mysql2/), - environment: stringMatching(/^testing/), + database: 'mysql8', + environment: 'testing-mysql', version: stringMatching(/\d+\.\d+\.\d+/), // labs is matched dynamically so adding/removing feature // flags doesn't churn the snapshot @@ -64,7 +64,7 @@ describe('Config API', function () { }) .matchHeaderSnapshot({ 'content-version': anyContentVersion, - 'content-length': anyContentLength, // Length can differ slightly based on the database, environment and version values + 'content-length': anyContentLength, // Length can differ slightly based on environment and version values etag: anyEtag, }); }); diff --git a/ghost/core/test/e2e-api/admin/integrations.test.js b/ghost/core/test/e2e-api/admin/integrations.test.js index 87d47c02eba..331777429c4 100644 --- a/ghost/core/test/e2e-api/admin/integrations.test.js +++ b/ghost/core/test/e2e-api/admin/integrations.test.js @@ -29,7 +29,7 @@ describe('Integrations API', function () { assert.equal(res.body.integrations.length, 5); - // there is no enforced order for integrations which makes order different on SQLite and MySQL + // There is no enforced order for integrations. const zapierIntegration = _.find(res.body.integrations, { name: 'Zapier' }); // from migrations assertExists(zapierIntegration); diff --git a/ghost/core/test/e2e-api/admin/members-filter-matrix.test.ts b/ghost/core/test/e2e-api/admin/members-filter-matrix.test.ts index 27ed4101139..3fe2ab5748d 100644 --- a/ghost/core/test/e2e-api/admin/members-filter-matrix.test.ts +++ b/ghost/core/test/e2e-api/admin/members-filter-matrix.test.ts @@ -26,10 +26,7 @@ describe('Members filtering, every operator admin can emit', function () { return body.members.map((member: { email: string }) => member.email).sort(); } - /** - * Dates must be the `YYYY-MM-DD HH:mm:ss` UTC string Ghost stores. A JS Date reaches SQLite - * as epoch milliseconds and compares as a number against the filter's text, matching every row. - */ + /** Dates must use the same `YYYY-MM-DD HH:mm:ss` UTC format Ghost stores. */ async function setColumns(email: string, columns: Record) { await models.Base.knex('members').where({ email }).update(columns); } @@ -116,9 +113,8 @@ describe('Members filtering, every operator admin can emit', function () { ]); }); - // Boundaries fall between members, never on one. SQLite compares the stored - // `2024-06-01 00:00:00` against the filter's `2024-06-01T00:00:00.000Z` as text, and a space - // sorts before `T`, so an exact tie tests storage format rather than the operator. + // Boundaries fall between members, never on one, so an exact tie does not test + // storage formatting rather than the operator. describe('native — a date', function () { matrix([ { what: 'created before', nql: "created_at:<'2024-03-01T00:00:00.000Z'", expect: [ALICE] }, diff --git a/ghost/core/test/e2e-api/admin/pages-legacy.test.js b/ghost/core/test/e2e-api/admin/pages-legacy.test.js index 00da89479a1..83a0264f666 100644 --- a/ghost/core/test/e2e-api/admin/pages-legacy.test.js +++ b/ghost/core/test/e2e-api/admin/pages-legacy.test.js @@ -37,7 +37,7 @@ describe('Pages API', function () { // Absolute urls by default. Match pages by url rather than by sort index: // fixture pages are seeded with near-identical timestamps, so the relative - // order of same-status pages is not stable across databases. + // order of same-status pages is not stable. const draftPage = jsonResponse.pages.find((page) => /\/p\/[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}\//.test( new URL(page.url).pathname, diff --git a/ghost/core/test/e2e-api/admin/posts-importer.test.js b/ghost/core/test/e2e-api/admin/posts-importer.test.js index 11910d42fa2..686f93365cd 100644 --- a/ghost/core/test/e2e-api/admin/posts-importer.test.js +++ b/ghost/core/test/e2e-api/admin/posts-importer.test.js @@ -207,7 +207,7 @@ describe('Posts Importer API', function () { assert.match(email.html, /Created:<\/strong> 2/); assert.match(email.html, /Updated:<\/strong> 0/); assert.match(email.html, /Skipped:<\/strong> 2/); - assert.match(email.html, /Failed:<\/strong> 2/); + assert.match(email.html, /Failed:<\/strong> 2 \(see attached errors\.csv\)/); assert.equal(email.attachments.length, 2); const report = email.attachments.find(({ filename }) => filename === 'report.csv'); assert.ok(report); @@ -235,8 +235,10 @@ describe('Posts Importer API', function () { const errorsFile = email.attachments.find(({ filename }) => filename === 'errors.csv'); assert.ok(errorsFile); const { data: errorRows, meta } = papaparse.parse(errorsFile.content.trim(), { header: true }); - assert.deepEqual(meta.fields.slice(0, 7), [ + assert.deepEqual(meta.fields.slice(0, 9), [ 'import_status', + 'import_reason', + 'import_media_failures', 'title', 'slug', 'status', @@ -281,18 +283,18 @@ describe('Posts Importer API', function () { subject: 'Your content import was unsuccessful', }); assert.match(email.html, /Skipped:<\/strong> 0/); - assert.match(email.html, /Failed:<\/strong> 1/); + assert.match(email.html, /Failed:<\/strong> 1 \(see attached errors\.csv\)/); const errorsFile = email.attachments.find(({ filename }) => filename === 'errors.csv'); assert.ok(errorsFile); const parsed = papaparse.parse(errorsFile.content.trim(), { header: true }); assert.deepEqual(parsed.meta.fields, [ 'import_status_2', + 'import_reason', + 'import_media_failures', 'Body', 'Headline', 'State', 'import_status', - 'import_reason', - 'import_media_failures', ]); assert.equal(parsed.data[0].Body, '

    Keep source cells

    '); assert.equal(parsed.data[0].Headline, 'ZIP invalid'); @@ -1023,6 +1025,10 @@ describe('Posts Importer API', function () { mapping: { First: 'title', Second: 'newsletter_id' }, reason: /Unknown post field mapping: "newsletter_id"/, }, + { + mapping: { First: 'title', Second: 'frontmatter' }, + reason: /Unknown post field mapping: "frontmatter"/, + }, { mapping: { First: 'title', Second: 'title' }, reason: /Post field is mapped more than once: "title"/, @@ -1044,6 +1050,25 @@ describe('Posts Importer API', function () { } }); + it('Ignores an unmapped frontmatter identity header', async function () { + await agent.loginAsOwner(); + + const frontmatterCsvPath = await csvFile( + 'posts-import-frontmatter.csv', + 'title,frontmatter\nFrontmatter source column,key: value\n', + ); + + await agent.post('posts/upload/').attach('postsfile', frontmatterCsvPath).expectStatus(202); + await contentImportService.allSettled(); + + const post = await models.Post.findOne( + { title: 'Frontmatter source column' }, + { withRelated: ['posts_meta'] }, + ); + assert.ok(post); + assert.equal(post.related('posts_meta').get('frontmatter'), undefined); + }); + it('Imports each CSV row as a post with its content and publish date', async function () { await agent.loginAsOwner(); diff --git a/ghost/core/test/e2e-api/content/posts.test.js b/ghost/core/test/e2e-api/content/posts.test.js index d4d1f6fdd84..9cc36b37b51 100644 --- a/ghost/core/test/e2e-api/content/posts.test.js +++ b/ghost/core/test/e2e-api/content/posts.test.js @@ -29,12 +29,8 @@ const postMatcherShallowIncludes = Object.assign({}, postMatcher, { authors: anyArray, }); -async function trackDb(fn, skip) { +async function trackDb(fn) { const db = require('../../../core/server/data/db'); - if (db?.knex?.client?.config?.client !== 'better-sqlite3') { - return skip(); - } - /** @type {import('knex').Knex.Client} */ const client = db.knex.client; @@ -44,19 +40,15 @@ async function trackDb(fn, skip) { } client.on('query', handler); - - await fn(); - - client.off('query', handler); + try { + await fn(); + } finally { + client.off('query', handler); + } return queries; } -// trackDb introspects sqlite query traffic, so its tests are sqlite-only; the -// invalid-filter test is mysql-only. Decided at registration from NODE_ENV -// (vitest has no runtime this.skip). -const isMySQL = (process.env.NODE_ENV || '').includes('mysql'); - describe('Posts Content API', function () { let agent; @@ -146,7 +138,7 @@ describe('Posts Content API', function () { }); }); - it.runIf(isMySQL)('Errors upon invalid filter value', async function () { + it('Errors upon invalid filter value', async function () { await agent .get(`posts/?filter=published_at%3A%3C%271715091791890%27`) .expectStatus(422) @@ -499,31 +491,22 @@ describe('Posts Content API', function () { ); }); - it.skipIf(isMySQL)('Does not select * by default', async function () { - let queries = await trackDb( - () => agent.get('posts/?limit=all').expectStatus(200), - () => {}, - ); + it('Does not select * by default', async function () { + let queries = await trackDb(() => agent.get('posts/?limit=all').expectStatus(200)); let postsRelatedQueries = queries.filter((q) => q.sql.includes('`posts`')); for (const query of postsRelatedQueries) { const sqlWithoutCount = query.sql.replace(/count\(\*\)/g, ''); assert(!sqlWithoutCount.includes('*'), 'Query should not select *'); } - queries = await trackDb( - () => agent.get('posts/?limit=3').expectStatus(200), - () => {}, - ); + queries = await trackDb(() => agent.get('posts/?limit=3').expectStatus(200)); postsRelatedQueries = queries.filter((q) => q.sql.includes('`posts`')); for (const query of postsRelatedQueries) { const sqlWithoutCount = query.sql.replace(/count\(\*\)/g, ''); assert(!sqlWithoutCount.includes('*'), 'Query should not select *'); } - queries = await trackDb( - () => agent.get('posts/?include=tags,authors').expectStatus(200), - () => {}, - ); + queries = await trackDb(() => agent.get('posts/?include=tags,authors').expectStatus(200)); postsRelatedQueries = queries.filter((q) => q.sql.includes('`posts`')); for (const query of postsRelatedQueries) { const sqlWithoutCount = query.sql.replace(/count\(\*\)/g, ''); @@ -531,19 +514,16 @@ describe('Posts Content API', function () { } }); - it.skipIf(isMySQL)('Can skip pagination counts when skipPagination is true', async function () { - const queries = await trackDb( - () => { - return api.postsPublic.browse({ - filter: "published_at:>'2015-07-20'", - skipPagination: true, - limit: 1, - order: 'published_at asc', - context: {}, - }); - }, - () => {}, - ); + it('Can skip pagination counts when skipPagination is true', async function () { + const queries = await trackDb(() => { + return api.postsPublic.browse({ + filter: "published_at:>'2015-07-20'", + skipPagination: true, + limit: 1, + order: 'published_at asc', + context: {}, + }); + }); const postsCountQueries = queries.filter((query) => { return query.sql.includes('count(') && query.sql.includes('`posts`'); diff --git a/ghost/core/test/e2e-api/content/tags.test.js b/ghost/core/test/e2e-api/content/tags.test.js index ef1ca397bdd..ab25b997034 100644 --- a/ghost/core/test/e2e-api/content/tags.test.js +++ b/ghost/core/test/e2e-api/content/tags.test.js @@ -5,7 +5,6 @@ const _ = require('lodash'); const configUtils = require('../../utils/config-utils'); const config = require('../../../core/shared/config'); const testUtils = require('../../utils'); -const dbUtils = require('../../utils/db-utils'); const localUtils = require('./utils'); describe('Tags Content API', function () { @@ -40,15 +39,8 @@ describe('Tags Content API', function () { localUtils.API.checkResponse(jsonResponse.meta.pagination, 'pagination'); // Default order 'name asc' check - // the ordering difference is described in https://github.com/TryGhost/Ghost/issues/6104 - // this condition should be removed once issue mentioned above ^ is resolved - if (dbUtils.isMySQL()) { - assert.equal(jsonResponse.tags[0].name, 'bacon'); - assert.equal(jsonResponse.tags[3].name, 'kitchen sink'); - } else { - assert.equal(jsonResponse.tags[0].name, 'Getting Started'); - assert.equal(jsonResponse.tags[4].name, 'kitchen sink'); - } + assert.equal(jsonResponse.tags[0].name, 'bacon'); + assert.equal(jsonResponse.tags[3].name, 'kitchen sink'); assert(new URL(res.body.tags[0].url).protocol); assert(new URL(res.body.tags[0].url).host); diff --git a/ghost/core/test/e2e-api/webmentions/webmentions.test.js b/ghost/core/test/e2e-api/webmentions/webmentions.test.js index fec16b694d9..6ccd3369251 100644 --- a/ghost/core/test/e2e-api/webmentions/webmentions.test.js +++ b/ghost/core/test/e2e-api/webmentions/webmentions.test.js @@ -9,23 +9,71 @@ const models = require('../../../core/server/models'); const assert = require('node:assert/strict'); const urlUtils = require('../../../core/shared/url-utils').default; const nock = require('nock'); -const jobsService = require('../../../core/server/services/mentions-jobs'); +const sinon = require('sinon'); +const mentionsService = require('../../../core/server/services/mentions'); const DomainEvents = require('@tryghost/domain-events'); -async function allSettled() { - await jobsService.allSettled(); - await DomainEvents.allSettled(); +let processedJobs; +let waiters; + +function processedCount(source) { + return processedJobs.filter((job) => job.source === source).length; } -async function receiveWebmention(agent, body) { - const processWebmentionJob = jobsService.awaitCompletion('processWebmention'); +function recordProcessed(job) { + processedJobs.push(job); + for (const waiter of [...waiters]) { + if (processedCount(waiter.source) >= waiter.count) { + waiters.splice(waiters.indexOf(waiter), 1); + waiter.resolve(); + } + } +} - await agent.post('/receive').body(body).expectStatus(202); +// Wrap the seam the job handler calls: completion here means the job ran, +// without coupling the test to log output or polling on a timer. +function trackWebmentionProcessing() { + const controller = mentionsService.controller; + const original = controller.processWebmention.bind(controller); + sinon.stub(controller, 'processWebmention').callsFake(async (job) => { + try { + return await original(job); + } finally { + // Recorded in finally: "processed" means the handler finished, + // successful or not - the DB assertions decide correctness. + recordProcessed(job); + } + }); +} - await processWebmentionJob; +async function waitForWebmention(source, count = 1, { timeoutMs = 5000 } = {}) { + if (processedCount(source) < count) { + await new Promise((resolve, reject) => { + const timer = setTimeout(() => { + waiters.splice( + waiters.findIndex((waiter) => waiter.resolve === settle), + 1, + ); + reject( + new Error(`Timed out waiting for ${count} webmention(s) from ${source} to be processed`), + ); + }, timeoutMs); + const settle = () => { + clearTimeout(timer); + resolve(); + }; + waiters.push({ source, count, resolve: settle }); + }); + } await DomainEvents.allSettled(); } +async function receiveWebmention(agent, body, count = 1) { + await agent.post('/receive').body(body).expectStatus(202); + + await waitForWebmention(body.source, count); +} + describe('Webmentions (receiving)', function () { let agent; @@ -35,12 +83,15 @@ describe('Webmentions (receiving)', function () { }); beforeEach(async function () { - await allSettled(); + await DomainEvents.allSettled(); + processedJobs = []; + waiters = []; + trackWebmentionProcessing(); mockManager.disableNetwork(); }); afterEach(async function () { - await allSettled(); + await DomainEvents.allSettled(); mockManager.restore(); await dbUtils.truncate('brute'); }); @@ -67,7 +118,7 @@ describe('Webmentions (receiving)', function () { }) .expectStatus(202); - await allSettled(); + await waitForWebmention(sourceUrl.href); const mention = await models.Mention.findOne({ source: 'http://testpage.com/external-article/', @@ -111,7 +162,7 @@ describe('Webmentions (receiving)', function () { }) .expectStatus(202); - await allSettled(); + await waitForWebmention(sourceUrl.href); const mention = await models.Mention.findOne({ source: 'http://testpage.com/update-mention-test-1/', @@ -146,7 +197,7 @@ describe('Webmentions (receiving)', function () { }) .expectStatus(202); - await allSettled(); + await waitForWebmention(sourceUrl.href, 2); const mention = await models.Mention.findOne({ source: 'http://testpage.com/update-mention-test-1/', @@ -197,10 +248,14 @@ describe('Webmentions (receiving)', function () { nock(targetUrl.origin).persist().head(targetUrl.pathname).reply(404); testUpdatingTheMention: { - await receiveWebmention(agent, { - source: sourceUrl.href, - target: targetUrl.href, - }); + await receiveWebmention( + agent, + { + source: sourceUrl.href, + target: targetUrl.href, + }, + 2, + ); const mention = await models.Mention.findOne({ source: 'http://testpage.com/update-mention-test-2/', @@ -238,7 +293,7 @@ describe('Webmentions (receiving)', function () { }) .expectStatus(202); - await allSettled(); + await waitForWebmention(sourceUrl.href); const mention = await models.Mention.findOne({ source: 'http://testpage.com/external-article-2/', @@ -274,7 +329,7 @@ describe('Webmentions (receiving)', function () { }) .expectStatus(202); - await allSettled(); + await waitForWebmention(sourceUrl.href); const mention = await models.Mention.findOne({ source: 'http://testpage.com/external-article-2/', @@ -305,7 +360,7 @@ describe('Webmentions (receiving)', function () { }) .expectStatus(202); - await allSettled(); + await waitForWebmention(sourceUrl.href); const mention = await models.Mention.findOne({ source: sourceUrl.href }); @@ -334,7 +389,7 @@ describe('Webmentions (receiving)', function () { }) .expectStatus(202); - await allSettled(); + await waitForWebmention(sourceUrl.href); const mention = await models.Mention.findOne({ source: sourceUrl.href }); @@ -363,7 +418,7 @@ describe('Webmentions (receiving)', function () { }) .expectStatus(202); - await allSettled(); + await waitForWebmention(sourceUrl.href); const mention = await models.Mention.findOne({ source: sourceUrl.toString() }); @@ -392,7 +447,7 @@ describe('Webmentions (receiving)', function () { }) .expectStatus(202); - await allSettled(); + await waitForWebmention(sourceUrl.href); const mention = await models.Mention.findOne({ source: 'http://testpage.com/external-article-2/', @@ -423,7 +478,7 @@ describe('Webmentions (receiving)', function () { }) .expectStatus(202); - await allSettled(); + await waitForWebmention(sourceUrl.href); const mention = await models.Mention.findOne({ source: 'http://testpage.com/external-article-2/', @@ -473,7 +528,7 @@ describe('Webmentions (receiving)', function () { payload: {}, }) .expectStatus(429); - await allSettled(); + await waitForWebmention(sourceUrl.href, webmentionBlock.freeRetries + 1); }); // NOTE: do not list other tests after the spam prevention test }); diff --git a/ghost/core/test/integration/exporter/exporter.test.js b/ghost/core/test/integration/exporter/exporter.test.js index 03295b5715e..8ca89f8b079 100644 --- a/ghost/core/test/integration/exporter/exporter.test.js +++ b/ghost/core/test/integration/exporter/exporter.test.js @@ -181,8 +181,5 @@ describe('Exporter', function () { }); assert.equal(_.find(exportData.data.settings, { key: 'permalinks' }), undefined); - - // should not export sqlite data - assert.equal(exportData.data.sqlite_sequence, undefined); }); }); diff --git a/ghost/core/test/integration/importer/v2.test.js b/ghost/core/test/integration/importer/v2.test.js index c85ac56f8c2..efdcec7a419 100644 --- a/ghost/core/test/integration/importer/v2.test.js +++ b/ghost/core/test/integration/importer/v2.test.js @@ -100,7 +100,7 @@ describe('Importer', function () { ); assert.equal(moment(importResult.data.tags[0].updated_at).format(), '2016-07-17T12:02:54Z'); - // Ensure sqlite3 & mysql import of dates works as expected + // Ensure imported dates are normalized as expected. assert.equal(moment(importResult.data.posts[1].created_at).valueOf(), 1388318310000); assert.equal(moment(importResult.data.posts[1].updated_at).valueOf(), 1388318310000); assert.equal(moment(importResult.data.posts[1].published_at).valueOf(), 1388404710000); diff --git a/ghost/core/test/integration/migrations/migration.test.js b/ghost/core/test/integration/migrations/migration.test.js index 0fbf4ad45af..26b673aa8c4 100644 --- a/ghost/core/test/integration/migrations/migration.test.js +++ b/ghost/core/test/integration/migrations/migration.test.js @@ -54,11 +54,7 @@ describe('Migrations', function () { it('should have idempotent migrations', async function () { // Delete all knowledge that we've run migrations so we can run them again - if (dbUtils.isMySQL()) { - await db.knex('migrations').whereILike('version', `${currentMajor}.%`).del(); - } else { - await db.knex('migrations').whereLike('version', `${currentMajor}.%`).del(); - } + await db.knex('migrations').whereILike('version', `${currentMajor}.%`).del(); await knexMigrator.migrate({ force: true, diff --git a/ghost/core/test/integration/migrations/nullable-utils.test.js b/ghost/core/test/integration/migrations/nullable-utils.test.js index ff27895c9f8..b95fd1d2b49 100644 --- a/ghost/core/test/integration/migrations/nullable-utils.test.js +++ b/ghost/core/test/integration/migrations/nullable-utils.test.js @@ -1,15 +1,13 @@ const assert = require('node:assert/strict'); const sinon = require('sinon'); const testUtils = require('../../utils'); -const dbUtils = require('../../utils/db-utils'); const logging = require('@tryghost/logging'); const utils = require('../../../core/server/data/migrations/utils'); const db = require('../../../core/server/data/db'); -// Run a migration step the way knex-migrator would: inside a transaction when the -// migration declares config.transaction (MySQL), otherwise on a plain connection -// (SQLite, where the table rebuild must toggle foreign_keys outside a transaction). +// Run a migration step the way knex-migrator does: inside a transaction when the +// migration declares config.transaction, otherwise on a plain connection. async function runMigrationStep(migration, method) { if (migration.config && migration.config.transaction) { const transacting = await db.knex.transaction(); @@ -81,27 +79,25 @@ describe('Migrations - schema utils', function () { // Drop tables in correct order due to foreign key constraints if (await knex.schema.hasTable(tableName)) { - if (dbUtils.isMySQL()) { - try { - // Get all foreign keys for the table - const fks = await knex.raw( - ` + try { + // Get all foreign keys for the table + const fks = await knex.raw( + ` SELECT CONSTRAINT_NAME FROM information_schema.TABLE_CONSTRAINTS WHERE TABLE_SCHEMA = DATABASE() AND TABLE_NAME = ? AND CONSTRAINT_TYPE = 'FOREIGN KEY' `, - [tableName], - ); - - // Drop each foreign key - for (const fk of fks[0]) { - await knex.raw(`ALTER TABLE ?? DROP FOREIGN KEY ??`, [tableName, fk.CONSTRAINT_NAME]); - } - } catch (err) { - // Foreign keys might not exist, continue + [tableName], + ); + + // Drop each foreign key + for (const fk of fks[0]) { + await knex.raw(`ALTER TABLE ?? DROP FOREIGN KEY ??`, [tableName, fk.CONSTRAINT_NAME]); } + } catch (err) { + // Foreign keys might not exist, continue } await knex.schema.dropTable(tableName); } @@ -114,31 +110,15 @@ describe('Migrations - schema utils', function () { }); async function isColumnNullable(table, column) { - const knex = db.knex; - - if (dbUtils.isSQLite()) { - const response = await knex.raw('PRAGMA table_info(??)', [table]); - const columnInfo = response.find((col) => col.name === column); - return columnInfo && columnInfo.notnull === 0; - } else { - const response = await knex.raw('SHOW COLUMNS FROM ??', [table]); - const columnInfo = response[0].find((col) => col.Field === column); - return columnInfo && columnInfo.Null === 'YES'; - } + const response = await db.knex.raw('SHOW COLUMNS FROM ??', [table]); + const columnInfo = response[0].find((col) => col.Field === column); + return columnInfo && columnInfo.Null === 'YES'; } async function isColumnNotNullable(table, column) { - const knex = db.knex; - - if (dbUtils.isSQLite()) { - const response = await knex.raw('PRAGMA table_info(??)', [table]); - const columnInfo = response.find((col) => col.name === column); - return columnInfo && columnInfo.notnull === 1; - } else { - const response = await knex.raw('SHOW COLUMNS FROM ??', [table]); - const columnInfo = response[0].find((col) => col.Field === column); - return columnInfo && columnInfo.Null === 'NO'; - } + const response = await db.knex.raw('SHOW COLUMNS FROM ??', [table]); + const columnInfo = response[0].find((col) => col.Field === column); + return columnInfo && columnInfo.Null === 'NO'; } describe('createSetNullableMigration', function () { @@ -295,12 +275,10 @@ describe('Migrations - schema utils', function () { const isNotNullable = await isColumnNotNullable(tableName, 'with_default'); assert.equal(isNotNullable, true, 'Column should be not nullable'); - // Verify default value is preserved (MySQL-specific check) - if (dbUtils.isMySQL()) { - const response = await db.knex.raw('SHOW COLUMNS FROM ??', [tableName]); - const columnInfo = response[0].find((col) => col.Field === 'with_default'); - assert.equal(columnInfo.Default, 'default', 'Column should still have its default value'); - } + // Verify default value is preserved + const response = await db.knex.raw('SHOW COLUMNS FROM ??', [tableName]); + const columnInfo = response[0].find((col) => col.Field === 'with_default'); + assert.equal(columnInfo.Default, 'default', 'Column should still have its default value'); }); it('Handles non-existent table errors', async function () { @@ -317,25 +295,15 @@ describe('Migrations - schema utils', function () { // Expected to fail when actually trying to alter the non-existent table } - // The behavior differs between databases: - // - MySQL: SHOW COLUMNS will throw an error for non-existent table, logging a warning - // - SQLite: PRAGMA table_info returns empty result, no error until ALTER TABLE - - if (dbUtils.isMySQL()) { - // MySQL should log a warning when checking nullable status fails - sinon.assert.calledWith(logWarnSpy, sinon.match('Could not check nullable status')); - } + sinon.assert.calledWith(logWarnSpy, sinon.match('Could not check nullable status')); - // Both databases should eventually fail when trying to ALTER the non-existent table + // The migration should fail when trying to alter the non-existent table. assert(errorThrown, 'Should throw an error when trying to alter non-existent table'); - // The error message varies between databases and Knex versions - // SQLite might give a Knex internal error or a 'no such table' error - // MySQL should give a 'table does not exist' error + // The error message varies between MySQL and Knex versions. const isExpectedError = - errorMessage.match(/no such table|does not exist|doesn't exist|Table .* not found/i) || - errorMessage.includes('Cannot read properties of undefined') || - errorMessage.includes('SQLITE_ERROR'); + errorMessage.match(/does not exist|doesn't exist|Table .* not found/i) || + errorMessage.includes('Cannot read properties of undefined'); assert(isExpectedError, `Error should be related to missing table, but was: ${errorMessage}`); }); diff --git a/ghost/core/test/integration/migrations/view-security.test.js b/ghost/core/test/integration/migrations/view-security.test.js index b7f2e7df570..d40a650c78a 100644 --- a/ghost/core/test/integration/migrations/view-security.test.js +++ b/ghost/core/test/integration/migrations/view-security.test.js @@ -5,9 +5,6 @@ const commands = require('../../../core/server/data/schema/commands'); const views = require('../../../core/server/data/schema/views'); const repairMigration = require('../../../core/server/data/migrations/versions/6.50/2026-06-30-09-00-00-set-members-resolved-subscription-view-security-invoker'); -// SQLite has no SQL SECURITY / DEFINER concept, so this only applies to MySQL. -const isMySQL = (process.env.NODE_ENV || '').includes('mysql'); - async function securityType(viewName) { const [rows] = await db.knex.raw( ` @@ -28,42 +25,36 @@ describe('Migrations - view security', function () { // Fresh installs create views through the init path, which now routes // through createViewOrReplace, so the shipped view must be INVOKER. - it.runIf(isMySQL)( - 'ships members_resolved_subscription with SQL SECURITY INVOKER', - async function () { - assert.equal(await securityType('members_resolved_subscription'), 'INVOKER'); - }, - ); + it('ships members_resolved_subscription with SQL SECURITY INVOKER', async function () { + assert.equal(await securityType('members_resolved_subscription'), 'INVOKER'); + }); // Existing installs are repaired by the versioned migration. Reproduce the // pre-6.50 state (MySQL's default SQL SECURITY DEFINER) on the real view, // then run the actual migration and assert it flips to INVOKER. A valid // (current-user) definer keeps the view queryable so the migration, not a // broken precondition, is what is under test. - it.runIf(isMySQL)( - 'the 6.50 migration converts an existing DEFINER view to INVOKER', - async function () { - await db.knex.raw( - 'CREATE OR REPLACE SQL SECURITY DEFINER VIEW `members_resolved_subscription` AS ' + - views.members_resolved_subscription, - ); - assert.equal( - await securityType('members_resolved_subscription'), - 'DEFINER', - 'precondition: view should be DEFINER before the migration', - ); + it('the 6.50 migration converts an existing DEFINER view to INVOKER', async function () { + await db.knex.raw( + 'CREATE OR REPLACE SQL SECURITY DEFINER VIEW `members_resolved_subscription` AS ' + + views.members_resolved_subscription, + ); + assert.equal( + await securityType('members_resolved_subscription'), + 'DEFINER', + 'precondition: view should be DEFINER before the migration', + ); - await repairMigration.up({ connection: db.knex }); + await repairMigration.up({ connection: db.knex }); - assert.equal(await securityType('members_resolved_subscription'), 'INVOKER'); - }, - ); + assert.equal(await securityType('members_resolved_subscription'), 'INVOKER'); + }); // Demonstrates the user-visible failure and that the helper fixes it: a // DEFINER-bound view whose definer account is absent — what a dump restored // onto a different server looks like — errors at query time (1449) until the // helper recreates it with INVOKER. - it.runIf(isMySQL)('recovers a view whose DEFINER account is absent', async function () { + it('recovers a view whose DEFINER account is absent', async function () { const viewName = 'ber3756_view_security_demo'; const viewSql = 'SELECT 1 AS one'; diff --git a/ghost/core/test/integration/services/email-service/domain-warming.test.js b/ghost/core/test/integration/services/email-service/domain-warming.test.js index 5a521702500..1a755dc770b 100644 --- a/ghost/core/test/integration/services/email-service/domain-warming.test.js +++ b/ghost/core/test/integration/services/email-service/domain-warming.test.js @@ -9,8 +9,6 @@ const crypto = require('crypto'); const db = require('../../../../core/server/data/db'); const { mockSystemTime } = require('../../../utils/clock-utils'); -const isMySQL = (process.env.NODE_ENV || '').includes('mysql'); - describe('Domain Warming Integration Tests', function () { let agent; let clock; @@ -346,9 +344,8 @@ describe('Domain Warming Integration Tests', function () { ); }); - // mysql-only: SQLite's small bound-parameter limit can't take the 800-member - // bulk insert. 60s timeout: 800 members across 5 days of batch sends. - it.runIf(isMySQL)('handles maximum limit scenarios', { timeout: 60000 }, async function () { + // 60s timeout: 800 members across 5 days of batch sends. + it('handles maximum limit scenarios', { timeout: 60000 }, async function () { await createMembers(800, 'maxlimit'); let previousCsdCount = 0; diff --git a/ghost/core/test/integration/services/gift-links.test.ts b/ghost/core/test/integration/services/gift-links.test.ts index f44ca5ea784..b65fdcb84cf 100644 --- a/ghost/core/test/integration/services/gift-links.test.ts +++ b/ghost/core/test/integration/services/gift-links.test.ts @@ -98,7 +98,6 @@ describe('GiftLinksService (integration)', function () { gift_link_token: 'second-live-token', created_at: new Date(), }), - // SQLite: "UNIQUE constraint failed"; MySQL: "Duplicate entry ... for key" /unique|duplicate/i, ); }); diff --git a/ghost/core/test/integration/services/mentions/process-webmention-job.test.ts b/ghost/core/test/integration/services/mentions/process-webmention-job.test.ts new file mode 100644 index 00000000000..37871d134b5 --- /dev/null +++ b/ghost/core/test/integration/services/mentions/process-webmention-job.test.ts @@ -0,0 +1,69 @@ +import { describe, it, beforeAll, afterAll } from 'vitest'; +import assert from 'node:assert/strict'; +import nock from 'nock'; + +const { agentProvider, fixtureManager } = require('../../../utils/e2e-framework'); +const models = require('../../../../core/server/models'); +const urlUtils = require('../../../../core/shared/url-utils').default; +const { getInstance: getJobsService } = require('../../../../core/server/services/jobs-service'); +const ProcessWebmentionJob = + require('../../../../core/server/services/mentions/process-webmention-job').default; + +async function waitFor( + check: () => boolean | Promise, + { timeoutMs = 5000, intervalMs = 25 } = {}, +): Promise { + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + if (await check()) { + return true; + } + await new Promise((resolve) => { + setTimeout(resolve, intervalMs); + }); + } + return false; +} + +describe('Job: Process webmention', function () { + beforeAll(async function () { + await agentProvider.getAdminAPIAgent(); + await fixtureManager.init('posts'); + }); + + afterAll(function () { + nock.cleanAll(); + }); + + it('records the mention when the dispatched job runs', async function () { + const targetUrl = new URL(urlUtils.getSiteUrl()); + const sourceUrl = new URL('http://dispatched-webmention.com/external-article/'); + const html = `Test Page`; + + nock(targetUrl.origin).persist().head(targetUrl.pathname).reply(200); + nock(sourceUrl.origin) + .persist() + .get(sourceUrl.pathname) + .reply(200, html, { 'Content-Type': 'text/html' }); + + await getJobsService().dispatch( + new ProcessWebmentionJob({ + source: sourceUrl.href, + target: targetUrl.href, + payload: { withExtension: true }, + }), + ); + + const recorded = await waitFor(async () => { + return !!(await models.Mention.findOne({ source: sourceUrl.href })); + }); + assert.ok(recorded, 'The webmention was processed'); + + const mention = await models.Mention.findOne({ source: sourceUrl.href }); + assert.equal(mention.get('target'), targetUrl.href); + assert.equal(mention.get('source_title'), 'Test Page'); + assert.equal(mention.get('source_excerpt'), 'Test description'); + assert.equal(mention.get('source_author'), 'John Doe'); + assert.equal(mention.get('payload'), JSON.stringify({ withExtension: true })); + }); +}); diff --git a/ghost/core/test/integration/url-serialization.test.js b/ghost/core/test/integration/url-serialization.test.js index d8a1d53b231..797601a2d2f 100644 --- a/ghost/core/test/integration/url-serialization.test.js +++ b/ghost/core/test/integration/url-serialization.test.js @@ -368,8 +368,7 @@ describe('Integration: Content API URL serialization (primary_tag permalinks)', // tags differently per query on MySQL, changing the post's URL under a // {primary_tag} permalink (seen as /disaster-preparedness/ vs // /claremont-elmwood/). The posts_tags.id tie-break pins it to the - // first-attached public tag. Only diverges on MySQL; on sqlite this pins - // the expected ordering. + // first-attached public tag. describe('tied tag sort_order', function () { beforeAll(async function () { // Two public tags with an internal one between them, all sharing diff --git a/ghost/core/test/legacy/models/model-collections.test.js b/ghost/core/test/legacy/models/model-collections.test.js index 534927196a1..29b8bfa1c73 100644 --- a/ghost/core/test/legacy/models/model-collections.test.js +++ b/ghost/core/test/legacy/models/model-collections.test.js @@ -3,12 +3,6 @@ const testUtils = require('../../utils'); const models = require('../../../core/server/models'); const db = require('../../../core/server/data/db'); -// These two tests inspect raw sqlite query traffic (db.knex.client as an sqlite3 -// Database), so they only apply on the sqlite leg. Decide at registration from -// NODE_ENV (the mysql leg sets testing-mysql) — db.knex isn't connected yet when -// the file loads, and Vitest has no Mocha-style runtime this.skip(). -const isMySQL = (process.env.NODE_ENV || '').includes('mysql'); - describe('Collection Model', function () { beforeAll(testUtils.teardownDb); beforeAll(testUtils.stopGhost); @@ -19,24 +13,22 @@ describe('Collection Model', function () { beforeAll(testUtils.setup('users:roles', 'posts')); describe('add', function () { - it.skipIf(isMySQL)( - 'does not update the sort_order of the collections_posts table if the type is "automatic"', - async function () { - /** @type {import('knex').Knex.Client} */ - const database = db.knex.client; + it('does not update the sort_order of the collections_posts table if the type is "automatic"', async function () { + /** @type {import('knex').Knex.Client} */ + const database = db.knex.client; - let didUpdateCollectionPosts = false; + let didUpdateCollectionPosts = false; - function handler(/** @type {{sql: string}} */ query) { - if (query.sql.toLowerCase().includes('update `collections_posts` set `sort_order`')) { - didUpdateCollectionPosts = true; - } + function handler(/** @type {{sql: string}} */ query) { + if (query.sql.toLowerCase().includes('update `collections_posts` set `sort_order`')) { + didUpdateCollectionPosts = true; } + } - const posts = await models.Post.findAll(); - - database.on('query', handler); + const posts = await models.Post.findAll(); + database.on('query', handler); + try { await models.Collection.add({ title: 'Test Collection', slug: 'test-collection-automatic', @@ -46,34 +38,33 @@ describe('Collection Model', function () { posts: posts.toJSON().map((post) => ({ id: post.id })), feature_image: null, }); - + } finally { database.off('query', handler); + } - const actual = didUpdateCollectionPosts; - const expected = false; - - assert.equal(actual, expected, 'collections_posts should not have been updated'); - }, - ); + assert.equal( + didUpdateCollectionPosts, + false, + 'collections_posts should not have been updated', + ); + }); - it.skipIf(isMySQL)( - 'does update the sort_order of the collections_posts table if the type is "manual"', - async function () { - /** @type {import('knex').Knex.Client} */ - const database = db.knex.client; + it('does update the sort_order of the collections_posts table if the type is "manual"', async function () { + /** @type {import('knex').Knex.Client} */ + const database = db.knex.client; - let didUpdateCollectionPosts = false; + let didUpdateCollectionPosts = false; - function handler(/** @type {{sql: string}} */ query) { - if (query.sql.toLowerCase().includes('update `collections_posts` set `sort_order`')) { - didUpdateCollectionPosts = true; - } + function handler(/** @type {{sql: string}} */ query) { + if (query.sql.toLowerCase().includes('update `collections_posts` set `sort_order`')) { + didUpdateCollectionPosts = true; } + } - const posts = await models.Post.findAll(); - - database.on('query', handler); + const posts = await models.Post.findAll(); + database.on('query', handler); + try { await models.Collection.add({ title: 'Test Collection', slug: 'test-collection-manual', @@ -83,14 +74,11 @@ describe('Collection Model', function () { posts: posts.toJSON().map((post) => ({ id: post.id })), feature_image: null, }); - + } finally { database.off('query', handler); + } - const actual = didUpdateCollectionPosts; - const expected = true; - - assert.equal(actual, expected, 'collections_posts should not have been updated'); - }, - ); + assert.equal(didUpdateCollectionPosts, true, 'collections_posts should have been updated'); + }); }); }); diff --git a/ghost/core/test/unit/server/data/schema/commands.test.js b/ghost/core/test/unit/server/data/schema/commands.test.js index 68366202a83..d103ed84b81 100644 --- a/ghost/core/test/unit/server/data/schema/commands.test.js +++ b/ghost/core/test/unit/server/data/schema/commands.test.js @@ -4,6 +4,21 @@ const errors = require('@tryghost/errors'); const commands = require('../../../../../core/server/data/schema/commands'); describe('schema commands', function () { + describe('getTables', function () { + it('excludes the SQLite sequence table', async function () { + const fakeKnex = { + client: { config: { client: 'better-sqlite3' } }, + raw: async () => [ + { tbl_name: 'posts' }, + { tbl_name: 'sqlite_sequence' }, + { tbl_name: 'users' }, + ], + }; + + assert.deepEqual(await commands.getTables(fakeKnex), ['posts', 'users']); + }); + }); + it('_hasForeignSQLite throws when knex is nox configured to use sqlite3', async function () { const Knex = require('knex'); const knex = Knex({ diff --git a/ghost/core/test/unit/server/lib/image/image-size.test.js b/ghost/core/test/unit/server/lib/image/image-size.test.js index 1b44993e8d9..8921f964c28 100644 --- a/ghost/core/test/unit/server/lib/image/image-size.test.js +++ b/ghost/core/test/unit/server/lib/image/image-size.test.js @@ -575,6 +575,107 @@ describe('lib/image: image size', function () { ); assert.equal(requestMock.isDone(), true); }); + + describe('concurrent lookups', function () { + it('[success] coalesces concurrent lookups for the same URL into a single request', async function () { + const url = 'http://img.stockfresh.com/files/f/feedough/x/11/1540353_20925115.jpg'; + const expectedImageObject = { height: 1, url, width: 1 }; + + // Only one interceptor is registered — if the second/third concurrent call + // issued its own request, nock would have no interceptor left to match it + // and that call would reject, failing this test. + const requestMock = nock('http://img.stockfresh.com') + .get('/files/f/feedough/x/11/1540353_20925115.jpg') + .reply(200, GIF1x1); + + const imageSize = createImageSize(); + + const [res1, res2, res3] = await Promise.all([ + imageSize.getImageSizeFromUrl(url), + imageSize.getImageSizeFromUrl(url), + imageSize.getImageSizeFromUrl(url), + ]); + + assert.equal(requestMock.isDone(), true); + assertImageObject(res1, expectedImageObject); + assertImageObject(res2, expectedImageObject); + assertImageObject(res3, expectedImageObject); + }); + + it('[success] does not coalesce concurrent lookups for different URLs', async function () { + const url1 = 'http://img.stockfresh.com/files/f/feedough/x/11/1540353_20925115.jpg'; + const url2 = 'http://img.stockfresh.com/files/f/feedough/x/12/1540353_20925116.jpg'; + + const requestMock1 = nock('http://img.stockfresh.com') + .get('/files/f/feedough/x/11/1540353_20925115.jpg') + .reply(200, GIF1x1); + const requestMock2 = nock('http://img.stockfresh.com') + .get('/files/f/feedough/x/12/1540353_20925116.jpg') + .reply(200, GIF1x1); + + const imageSize = createImageSize(); + + const [res1, res2] = await Promise.all([ + imageSize.getImageSizeFromUrl(url1), + imageSize.getImageSizeFromUrl(url2), + ]); + + assert.equal(requestMock1.isDone(), true); + assert.equal(requestMock2.isDone(), true); + assertImageObject(res1, { height: 1, url: url1, width: 1 }); + assertImageObject(res2, { height: 1, url: url2, width: 1 }); + }); + + it('[success] issues a fresh request for a later, non-concurrent lookup of the same URL', async function () { + const url = 'http://img.stockfresh.com/files/f/feedough/x/11/1540353_20925115.jpg'; + const imageSize = createImageSize(); + + const requestMock1 = nock('http://img.stockfresh.com') + .get('/files/f/feedough/x/11/1540353_20925115.jpg') + .reply(200, GIF1x1); + await imageSize.getImageSizeFromUrl(url); + assert.equal(requestMock1.isDone(), true); + + // The in-flight entry should have been cleaned up after the first lookup + // resolved, so this second, non-overlapping call issues its own request + // rather than reusing a stale settled promise. + const requestMock2 = nock('http://img.stockfresh.com') + .get('/files/f/feedough/x/11/1540353_20925115.jpg') + .reply(200, GIF1x1); + await imageSize.getImageSizeFromUrl(url); + assert.equal(requestMock2.isDone(), true); + }); + + it('[failure] rejects every concurrent caller when the shared lookup fails, then allows a retry', async function () { + const url = 'http://noimagehere.com/files/f/feedough/x/11/1540353_20925115.jpg'; + + const requestMock1 = nock('http://noimagehere.com') + .get('/files/f/feedough/x/11/1540353_20925115.jpg') + .reply(404); + + const imageSize = createImageSize(); + + const results = await Promise.allSettled([ + imageSize.getImageSizeFromUrl(url), + imageSize.getImageSizeFromUrl(url), + ]); + + assert.equal(requestMock1.isDone(), true); + assert.equal(results[0].status, 'rejected'); + assert.equal(results[1].status, 'rejected'); + assert.equal(results[0].reason.errorType, 'NotFoundError'); + assert.equal(results[1].reason.errorType, 'NotFoundError'); + + // In-flight entry must be cleared even on failure, so a later retry + // gets a real second chance instead of reusing the rejected promise. + const requestMock2 = nock('http://noimagehere.com') + .get('/files/f/feedough/x/11/1540353_20925115.jpg') + .reply(200, GIF1x1); + const retryResult = await imageSize.getImageSizeFromUrl(url); + assert.equal(requestMock2.isDone(), true); + assertImageObject(retryResult, { height: 1, url, width: 1 }); + }); + }); }); describe('getImageSizeFromStoragePath', function () { diff --git a/ghost/core/test/unit/server/services/content-import/import/completion-email.test.ts b/ghost/core/test/unit/server/services/content-import/import/completion-email.test.ts index 97fc3f25130..b41f468b91e 100644 --- a/ghost/core/test/unit/server/services/content-import/import/completion-email.test.ts +++ b/ghost/core/test/unit/server/services/content-import/import/completion-email.test.ts @@ -44,6 +44,7 @@ describe('content import completion email', function () { assert.match(email.html, /Updated:<\/strong> 1/); assert.match(email.html, /Skipped:<\/strong> 1/); assert.match(email.html, /Failed:<\/strong> 1/); + assert.doesNotMatch(email.html, /see attached errors\.csv/); assert.match(email.html, /1<\/strong> post has warnings/); }); @@ -51,7 +52,16 @@ describe('content import completion email', function () { const email = buildCompletionEmail( run({ total: 1, - rows: [{ line: 2, title: 'Failed', status: 'failed', reason: 'Write failed' }], + sourceColumns: ['title'], + rows: [ + { + line: 2, + title: 'Failed', + status: 'failed', + reason: 'Write failed', + source: { title: 'Failed' }, + }, + ], }), 'owner@example.com', ADMIN_URL, @@ -59,6 +69,7 @@ describe('content import completion email', function () { assert.equal(email.subject, 'Your content import was unsuccessful'); assert.match(email.html, /processed 1 row:/); + assert.match(email.html, /Failed:<\/strong> 1 \(see attached errors\.csv\)/); }); it('attaches a report for a completely clean run', function () { @@ -78,6 +89,7 @@ describe('content import completion email', function () { email.attachments.map(({ filename }) => filename), ['report.csv'], ); + assert.doesNotMatch(email.html, /see attached errors\.csv/); }); it('attaches both the report and actionable errors file in stable order', function () { @@ -103,6 +115,7 @@ describe('content import completion email', function () { email.attachments.map(({ filename }) => filename), ['report.csv', 'errors.csv'], ); + assert.match(email.html, /Failed:<\/strong> 1 \(see attached errors\.csv\)/); }); it('uses plural warning copy for multiple warning-bearing posts', function () { @@ -131,6 +144,7 @@ describe('content import completion email', function () { assert.equal(email.subject, 'Your content import could not be completed'); assert.match(email.html, /Something went wrong on our end/); assert.doesNotMatch(email.html, /database password/); + assert.doesNotMatch(email.html, /see attached errors\.csv/); }); it('omits attachments when a fatal run stopped before processing a row', function () { diff --git a/ghost/core/test/unit/server/services/content-import/import/errors-file.test.ts b/ghost/core/test/unit/server/services/content-import/import/errors-file.test.ts index 88ddfcecf7a..1533efc0300 100644 --- a/ghost/core/test/unit/server/services/content-import/import/errors-file.test.ts +++ b/ghost/core/test/unit/server/services/content-import/import/errors-file.test.ts @@ -81,11 +81,11 @@ describe('content import errors file', function () { const parsed = papaparse.parse>(csv, { header: true }); assert.deepEqual(parsed.meta.fields, [ 'import_status_2', + 'import_reason', + 'import_media_failures', 'Body', 'Headline', 'import_status', - 'import_reason', - 'import_media_failures', ]); assert.equal(parsed.data[0].Headline, "'=Failed()"); assert.equal(parsed.data[0].import_status, 'publisher value'); @@ -119,8 +119,10 @@ describe('content import errors file', function () { assert.ok(csv); const parsed = papaparse.parse(csv, { header: true }); - assert.equal(parsed.meta.fields?.[0], 'import_status_3'); - assert.ok(parsed.meta.fields?.includes('import_reason_2')); - assert.ok(parsed.meta.fields?.includes('import_media_failures_2')); + assert.deepEqual(parsed.meta.fields?.slice(0, 3), [ + 'import_status_3', + 'import_reason_2', + 'import_media_failures_2', + ]); }); }); diff --git a/ghost/core/test/unit/server/services/content-import/import/post-data.test.ts b/ghost/core/test/unit/server/services/content-import/import/post-data.test.ts index c6272df68cb..c431af0773d 100644 --- a/ghost/core/test/unit/server/services/content-import/import/post-data.test.ts +++ b/ghost/core/test/unit/server/services/content-import/import/post-data.test.ts @@ -210,7 +210,7 @@ describe('buildPostData', function () { assert.equal(data.comment_id, 'legacy-source-123'); }); - it('puts feature metadata, SEO, social fields, and frontmatter in posts_meta', function () { + it('puts feature metadata, SEO, and social fields in posts_meta', function () { const data = buildPostData( row({ title: 'Metadata post', @@ -224,7 +224,6 @@ describe('buildPostData', function () { twitter_image: 'https://example.com/twitter.jpg', twitter_title: 'Twitter title', twitter_description: 'Twitter description', - frontmatter: 'key: value', }), htmlToLexical, TAGS, @@ -241,10 +240,20 @@ describe('buildPostData', function () { twitter_image: 'https://example.com/twitter.jpg', twitter_title: 'Twitter title', twitter_description: 'Twitter description', - frontmatter: 'key: value', }); }); + it('ignores an unmapped frontmatter source column', function () { + const data = buildPostData( + row({ title: 'Unsupported metadata', frontmatter: 'key: value' }), + htmlToLexical, + TAGS, + ); + + assert.equal(data.posts_meta, undefined); + assert.equal('frontmatter' in data, false); + }); + it('omits every date when the cell is absent, leaving the model to stamp now', function () { const data = buildPostData(row({ title: 'T' }), htmlToLexical, TAGS); diff --git a/ghost/core/test/unit/server/services/content-import/import/row.test.ts b/ghost/core/test/unit/server/services/content-import/import/row.test.ts index a89c0db46c5..91c6a60088e 100644 --- a/ghost/core/test/unit/server/services/content-import/import/row.test.ts +++ b/ghost/core/test/unit/server/services/content-import/import/row.test.ts @@ -38,7 +38,6 @@ describe('post import row schema', function () { slug: '', feature_image: 'undefined', meta_title: '', - frontmatter: '', comment_id: '', authors: '', author_emails: 'undefined', @@ -48,7 +47,6 @@ describe('post import row schema', function () { assert.equal(parsed.slug, undefined); assert.equal(parsed.feature_image, undefined); assert.equal(parsed.meta_title, undefined); - assert.equal(parsed.frontmatter, undefined); assert.equal(parsed.comment_id, undefined); assert.equal(parsed.authors, undefined); assert.equal(parsed.author_emails, undefined); diff --git a/ghost/core/test/unit/server/services/content-import/import/schema.test.ts b/ghost/core/test/unit/server/services/content-import/import/schema.test.ts index 135132fc881..824a162bc2b 100644 --- a/ghost/core/test/unit/server/services/content-import/import/schema.test.ts +++ b/ghost/core/test/unit/server/services/content-import/import/schema.test.ts @@ -88,6 +88,9 @@ describe('content import schema', function () { assert.deepEqual(issuesFor({ Headline: 'title', Body: 'not_a_field' }), [ 'Unknown post field mapping: "not_a_field"', ]); + assert.deepEqual(issuesFor({ Headline: 'title', Metadata: 'frontmatter' }), [ + 'Unknown post field mapping: "frontmatter"', + ]); assert.deepEqual(issuesFor({ Headline: 'title', Duplicate: 'title' }), [ 'Post field is mapped more than once: "title"', ]); diff --git a/ghost/core/test/unit/server/services/jobs-service/register-job-handlers.test.ts b/ghost/core/test/unit/server/services/jobs-service/register-job-handlers.test.ts index f48bcffe6a0..b0d734fefbb 100644 --- a/ghost/core/test/unit/server/services/jobs-service/register-job-handlers.test.ts +++ b/ghost/core/test/unit/server/services/jobs-service/register-job-handlers.test.ts @@ -1,24 +1,22 @@ import assert from 'node:assert/strict'; import sinon from 'sinon'; import { describe, it, beforeEach, afterEach } from 'vitest'; -import logging from '@tryghost/logging'; import { JobsService } from '../../../../../core/server/services/jobs-service/jobs-service'; import ExternalMediaInliner from '../../../../../core/server/services/media-inliner/external-media-inliner'; import ExternalMediaInlinerJob from '../../../../../core/server/services/media-inliner/external-media-inliner-job'; import ContentCSVImportJob from '../../../../../core/server/services/content-import/jobs/content-csv-import-job'; import UpdateCheckJob from '../../../../../core/server/services/update-check/jobs/update-check-job'; +import ProcessWebmentionJob from '../../../../../core/server/services/mentions/process-webmention-job'; const registerJobHandlers = require('../../../../../core/server/services/jobs-service/register-job-handlers').default; describe('register-job-handlers', function () { let jobsService: sinon.SinonStubbedInstance; - let db: { knex: sinon.SinonStub & { transaction?: sinon.SinonStub } }; - let loggingStub: sinon.SinonStubbedInstance; let mediaInliner: sinon.SinonStubbedInstance; - let models: { Member: { findOne: sinon.SinonStub } }; - let events: { emit: sinon.SinonStub }; - let sentry: { captureException: sinon.SinonStub }; + let memberJobs: { cleanTokens: sinon.SinonStub; cleanExpiredComped: sinon.SinonStub }; + let giftService: { cleanup: sinon.SinonStub }; + let mentionsController: { processWebmention: sinon.SinonStub }; // Handlers are looked up by their job type rather than registration order, // so adding a handler does not silently shift which one a test exercises. @@ -32,21 +30,20 @@ describe('register-job-handlers', function () { beforeEach(function () { jobsService = sinon.createStubInstance(JobsService); - db = { knex: sinon.stub() }; - loggingStub = sinon.stub(logging); mediaInliner = sinon.createStubInstance(ExternalMediaInliner); - models = { Member: { findOne: sinon.stub() } }; - events = { emit: sinon.stub() }; - sentry = { captureException: sinon.stub() }; + memberJobs = { + cleanTokens: sinon.stub().resolves(0), + cleanExpiredComped: sinon.stub().resolves(), + }; + giftService = { cleanup: sinon.stub().resolves() }; + mentionsController = { processWebmention: sinon.stub().resolves() }; registerJobHandlers({ jobsService, - db, - logging: loggingStub, - models, - events, - sentry, + memberJobs, + giftService, mediaInliner, + mentionsController, }); }); @@ -54,49 +51,28 @@ describe('register-job-handlers', function () { sinon.restore(); }); - // Nothing initialises the gifts service here, which is the state the guard - // exists for: a dispatch that lands before boot has built the service must - // fail loudly rather than reading undefined off the module. - it('fails a clean-gifts delivery when the gift service is not initialised', async function () { + it('runs clean-gifts with the injected gift service', async function () { const cleanGiftsHandler = handlerFor('clean-gifts'); - await assert.rejects(async () => { - await cleanGiftsHandler({}); - }, /clean-gifts ran before the gifts service was initialised/); + await cleanGiftsHandler({}); + + assert.ok(giftService.cleanup.calledOnce); }); - it('runs clean-tokens with the injected database and logger', async function () { - const deleteStub = sinon.stub().resolves(2); - const whereStub = sinon.stub().returns({ delete: deleteStub }); - db.knex.withArgs('tokens').returns({ where: whereStub }); + it('runs clean-tokens with the injected member jobs module', async function () { const cleanTokensHandler = handlerFor('clean-tokens'); await cleanTokensHandler({}); - assert.ok(db.knex.calledOnceWithExactly('tokens')); - assert.ok(loggingStub.info.calledOnce); - const metadata = loggingStub.info.firstCall.args[0] as { - system: { deleted_count: number }; - }; - assert.equal(metadata.system.deleted_count, 2); + assert.ok(memberJobs.cleanTokens.calledOnce); }); - it('runs clean-expired-comped with the injected database, models, events and logger', async function () { - db.knex.transaction = sinon.stub().callsFake(async (fn: (trx: unknown) => unknown) => { - const trx = () => ({ - where: () => ({ select: async () => [] }), - }); - return fn(trx); - }); + it('runs clean-expired-comped with the injected member jobs module', async function () { const cleanExpiredCompedHandler = handlerFor('clean-expired-comped'); await cleanExpiredCompedHandler({}); - const completionLog = loggingStub.info.getCalls().find((call) => { - const metadata = call.args[0] as { system?: { event?: string } }; - return metadata?.system?.event === 'clean_expired_comped.completed'; - }); - assert.ok(completionLog, 'the handler runs the task against the injected dependencies'); + assert.ok(memberJobs.cleanExpiredComped.calledOnce); }); it('runs external-media-inliner with the injected media inliner', async function () { @@ -146,4 +122,17 @@ describe('register-job-handlers', function () { await updateCheckHandler(new UpdateCheckJob()); }); + + it('runs process-webmention with the injected mentions controller', async function () { + const processWebmentionHandler = handlerFor('process-webmention'); + const job = new ProcessWebmentionJob({ + source: 'https://source.com/post/', + target: 'https://target.com/post/', + payload: {}, + }); + + await processWebmentionHandler(job); + + assert.ok(mentionsController.processWebmention.calledOnceWithExactly(job)); + }); }); diff --git a/ghost/core/test/unit/server/services/members/jobs/cleanup-tasks.test.ts b/ghost/core/test/unit/server/services/members/jobs/cleanup-tasks.test.ts new file mode 100644 index 00000000000..0d0a1f2e0fa --- /dev/null +++ b/ghost/core/test/unit/server/services/members/jobs/cleanup-tasks.test.ts @@ -0,0 +1,20 @@ +import assert from 'node:assert/strict'; +import { describe, it } from 'vitest'; + +// require, not import: this must resolve to the same CommonJS module instance +// the boot layer loads, so the uninitialised state below is the real pre-boot +// state and not a parallel ESM copy. +const memberJobs = require('../../../../../../core/server/services/members/jobs'); + +// Nothing initialises the module here, which is the state the guard exists +// for: a delivery that lands before boot has wired the tasks must fail loudly +// rather than reading undefined. +describe('member jobs: cleanup task guards', function () { + it('fails a clean-tokens run before init()', function () { + assert.throws(() => memberJobs.cleanTokens(), /Member jobs used before init\(\)/); + }); + + it('fails a clean-expired-comped run before init()', function () { + assert.throws(() => memberJobs.cleanExpiredComped(), /Member jobs used before init\(\)/); + }); +}); diff --git a/ghost/core/test/unit/server/services/members/jobs/schedule-expired-comped-cleanup.test.ts b/ghost/core/test/unit/server/services/members/jobs/schedule-expired-comped-cleanup.test.ts index af77aaf93e4..d12d21f1c18 100644 --- a/ghost/core/test/unit/server/services/members/jobs/schedule-expired-comped-cleanup.test.ts +++ b/ghost/core/test/unit/server/services/members/jobs/schedule-expired-comped-cleanup.test.ts @@ -3,32 +3,28 @@ import sinon from 'sinon'; import { describe, it, beforeEach, afterEach } from 'vitest'; import logging from '@tryghost/logging'; -// require, not import: these must resolve to the same CommonJS module -// instances that core/server/services/members/jobs/index.js loads, so the -// init() here is the instance scheduleExpiredCompCleanupJob() reads. -const jobsService = require('../../../../../../core/server/services/jobs-service'); -const adapterManager = require('../../../../../../core/server/services/adapter-manager').default; +// require, not import: this must resolve to the same CommonJS module instance +// the boot layer loads, so the module-level "already scheduled" state is shared. const memberJobs = require('../../../../../../core/server/services/members/jobs'); +const CleanExpiredCompedJob = + require('../../../../../../core/server/services/members/jobs/clean-expired-comped-job').default; describe('member jobs: expired comped cleanup scheduling', function () { - let scheduleStub: sinon.SinonStub; + let jobsService: { scheduleRecurring: sinon.SinonStub }; beforeEach(function () { - jobsService.init(); - const backend = adapterManager.getAdapter('jobs'); - scheduleStub = sinon.stub(backend, 'scheduleRecurring'); + jobsService = { scheduleRecurring: sinon.stub().resolves() }; }); - afterEach(async function () { - await jobsService.shutdown({ timeoutMs: 100 }); + afterEach(function () { sinon.restore(); }); it('does not schedule expired comped cleanup under the test environment', async function () { - await memberJobs.scheduleExpiredCompCleanupJob(); + await memberJobs.scheduleExpiredCompCleanupJob(jobsService); assert.ok( - scheduleStub.notCalled, + jobsService.scheduleRecurring.notCalled, 'expired comped cleanup must not be scheduled under NODE_ENV=test*', ); }); @@ -38,18 +34,18 @@ describe('member jobs: expired comped cleanup scheduling', function () { sinon.stub(logging, 'info'); process.env.NODE_ENV = 'production'; try { - await memberJobs.scheduleExpiredCompCleanupJob(); - await memberJobs.scheduleExpiredCompCleanupJob(); + await memberJobs.scheduleExpiredCompCleanupJob(jobsService); + await memberJobs.scheduleExpiredCompCleanupJob(jobsService); } finally { process.env.NODE_ENV = originalEnv; } assert.ok( - scheduleStub.calledOnce, + jobsService.scheduleRecurring.calledOnce, 'clean-expired-comped is scheduled once, however often scheduling is attempted', ); - const [envelope, schedule] = scheduleStub.firstCall.args; - assert.equal(envelope.type, 'clean-expired-comped'); + const [job, schedule] = jobsService.scheduleRecurring.firstCall.args; + assert.ok(job instanceof CleanExpiredCompedJob); assert.match( schedule.cron, /^\d{1,2} \d{1,2} [0-5] \* \* \*$/, diff --git a/ghost/core/test/unit/server/services/members/jobs/schedule-token-cleanup.test.ts b/ghost/core/test/unit/server/services/members/jobs/schedule-token-cleanup.test.ts index 4d7a0598157..16373df9063 100644 --- a/ghost/core/test/unit/server/services/members/jobs/schedule-token-cleanup.test.ts +++ b/ghost/core/test/unit/server/services/members/jobs/schedule-token-cleanup.test.ts @@ -3,31 +3,30 @@ import sinon from 'sinon'; import { describe, it, beforeEach, afterEach } from 'vitest'; import logging from '@tryghost/logging'; -// require, not import: these must resolve to the same CommonJS module -// instances that core/server/services/members/jobs/index.js loads, so the -// init() here is the instance scheduleTokenCleanupJob() reads. -const jobsService = require('../../../../../../core/server/services/jobs-service'); -const adapterManager = require('../../../../../../core/server/services/adapter-manager').default; +// require, not import: this must resolve to the same CommonJS module instance +// the boot layer loads, so the module-level "already scheduled" state is shared. const memberJobs = require('../../../../../../core/server/services/members/jobs'); +const CleanTokensJob = + require('../../../../../../core/server/services/members/jobs/clean-tokens-job').default; describe('member jobs: token cleanup scheduling', function () { - let scheduleStub: sinon.SinonStub; + let jobsService: { scheduleRecurring: sinon.SinonStub }; beforeEach(function () { - jobsService.init(); - const backend = adapterManager.getAdapter('jobs'); - scheduleStub = sinon.stub(backend, 'scheduleRecurring'); + jobsService = { scheduleRecurring: sinon.stub().resolves() }; }); - afterEach(async function () { - await jobsService.shutdown({ timeoutMs: 100 }); + afterEach(function () { sinon.restore(); }); it('does not schedule token cleanup under the test environment', async function () { - await memberJobs.scheduleTokenCleanupJob(); + await memberJobs.scheduleTokenCleanupJob(jobsService); - assert.ok(scheduleStub.notCalled, 'token cleanup must not be scheduled under NODE_ENV=test*'); + assert.ok( + jobsService.scheduleRecurring.notCalled, + 'token cleanup must not be scheduled under NODE_ENV=test*', + ); }); it('schedules a daily clean-tokens job outside the test environment', async function () { @@ -35,14 +34,17 @@ describe('member jobs: token cleanup scheduling', function () { sinon.stub(logging, 'info'); process.env.NODE_ENV = 'production'; try { - await memberJobs.scheduleTokenCleanupJob(); + await memberJobs.scheduleTokenCleanupJob(jobsService); } finally { process.env.NODE_ENV = originalEnv; } - assert.ok(scheduleStub.calledOnce, 'clean-tokens is scheduled outside the test environment'); - const [envelope, schedule] = scheduleStub.firstCall.args; - assert.equal(envelope.type, 'clean-tokens'); + assert.ok( + jobsService.scheduleRecurring.calledOnce, + 'clean-tokens is scheduled outside the test environment', + ); + const [job, schedule] = jobsService.scheduleRecurring.firstCall.args; + assert.ok(job instanceof CleanTokensJob); assert.match(schedule.cron, /^\d+ \d+ \d+ \* \* \*$/, 'a random daily 6-field cron'); }); }); diff --git a/ghost/core/test/unit/server/services/mentions/mention-controller.test.ts b/ghost/core/test/unit/server/services/mentions/mention-controller.test.ts new file mode 100644 index 00000000000..3227d9e0388 --- /dev/null +++ b/ghost/core/test/unit/server/services/mentions/mention-controller.test.ts @@ -0,0 +1,97 @@ +import assert from 'node:assert/strict'; +import sinon from 'sinon'; +import { describe, it, beforeEach, afterEach } from 'vitest'; +import logging from '@tryghost/logging'; +// Required, not imported: the controller requires the same module, and an ESM +// import would give this test a second copy of the class to compare against. +const ProcessWebmentionJob = + require('../../../../../core/server/services/mentions/process-webmention-job').default; +const MentionController = require('../../../../../core/server/services/mentions/mention-controller'); + +describe('MentionController', function () { + let controller: any; + let api: { processWebmention: sinon.SinonStub }; + let jobsService: { dispatch: sinon.SinonStub }; + let loggingStub: sinon.SinonStubbedInstance; + + beforeEach(async function () { + api = { processWebmention: sinon.stub().resolves() }; + jobsService = { dispatch: sinon.stub().resolves() }; + loggingStub = sinon.stub(logging); + controller = new MentionController(); + await controller.init({ api, jobsService, mentionResourceService: { getByID: sinon.stub() } }); + }); + + afterEach(function () { + sinon.restore(); + }); + + describe('processWebmention', function () { + it('parses the urls and forwards the payload to the api', async function () { + await controller.processWebmention({ + source: 'https://source.com/post/', + target: 'https://target.com/post/', + payload: { withExtension: true }, + }); + + sinon.assert.calledOnce(api.processWebmention); + const webmention = api.processWebmention.firstCall.args[0]; + assert.deepEqual(webmention.source, new URL('https://source.com/post/')); + assert.deepEqual(webmention.target, new URL('https://target.com/post/')); + assert.deepEqual(webmention.payload, { withExtension: true }); + }); + + it('swallows and logs a failure from the api', async function () { + const error = new Error('Could not process'); + api.processWebmention.rejects(error); + + await controller.processWebmention({ + source: 'https://source.com/post/', + target: 'https://target.com/post/', + payload: {}, + }); + + sinon.assert.calledWith( + loggingStub.error, + error, + '[Webmention] Failed processing webmention', + ); + }); + + it('swallows and logs an unparseable url', async function () { + await controller.processWebmention({ + source: 'not a url', + target: 'https://target.com/post/', + payload: {}, + }); + + sinon.assert.notCalled(api.processWebmention); + sinon.assert.calledWith( + loggingStub.error, + sinon.match.instanceOf(Error), + '[Webmention] Failed processing webmention', + ); + }); + }); + + describe('receive', function () { + it('dispatches a job that processes the webmention off the request', async function () { + await controller.receive({ + data: { + source: 'https://source.com/post/', + target: 'https://target.com/post/', + withExtension: true, + }, + }); + + sinon.assert.notCalled(api.processWebmention); + sinon.assert.calledOnce(jobsService.dispatch); + + const job = jobsService.dispatch.firstCall.args[0]; + assert.ok(job instanceof ProcessWebmentionJob); + assert.equal(job.source, 'https://source.com/post/'); + assert.equal(job.target, 'https://target.com/post/'); + assert.deepEqual(job.payload, { withExtension: true }); + }); + }); +}); diff --git a/ghost/core/test/unit/server/services/mentions/process-webmention-job.test.ts b/ghost/core/test/unit/server/services/mentions/process-webmention-job.test.ts new file mode 100644 index 00000000000..c503ef4bf49 --- /dev/null +++ b/ghost/core/test/unit/server/services/mentions/process-webmention-job.test.ts @@ -0,0 +1,21 @@ +import assert from 'node:assert/strict'; +import { describe, it } from 'vitest'; +import ProcessWebmentionJob from '../../../../../core/server/services/mentions/process-webmention-job'; + +describe('ProcessWebmentionJob', function () { + it('is dispatched under its own type', function () { + assert.equal(ProcessWebmentionJob.type, 'process-webmention'); + }); + + it('survives the round trip through the queue', function () { + const job = new ProcessWebmentionJob({ + source: 'https://source.com/post/', + target: 'https://target.com/post/', + payload: { withExtension: true, nested: { a: 'b' } }, + }); + + const revived = new ProcessWebmentionJob(JSON.parse(JSON.stringify(job))); + + assert.deepEqual(revived, job); + }); +}); diff --git a/ghost/core/test/unit/shared/config/loader.test.js b/ghost/core/test/unit/shared/config/loader.test.js index aa4c43f1c29..23bbcdbb43a 100644 --- a/ghost/core/test/unit/shared/config/loader.test.js +++ b/ghost/core/test/unit/shared/config/loader.test.js @@ -140,8 +140,6 @@ describe('Config Loader', function () { assert(!customConfig.get('paths:corePath').includes('try-to-override')); assert.equal(customConfig.get('database:client'), 'better-sqlite3'); - // Note: database:connection:filename is now set via process.env in test/utils/vitest-setup-db.ts - // for concurrent test isolation, so we skip asserting the config file value assert.equal(customConfig.get('database:debug'), true); // Note: url is now set via process.env in test/utils/vitest-setup-db.ts for dynamic port allocation assert.equal(customConfig.get('logging:level'), 'error'); diff --git a/ghost/core/test/utils/db-template-paths.js b/ghost/core/test/utils/db-template-paths.js deleted file mode 100644 index 4b18db679f2..00000000000 --- a/ghost/core/test/utils/db-template-paths.js +++ /dev/null @@ -1,34 +0,0 @@ -// Pure derivation of the shared DB-template identifiers from a per-fork database -// identifier. Kept dependency-free (NO Ghost config/db imports) so it can be -// required by the vitest globalSetup BEFORE Ghost's config loads. -// -// `vitest-setup-db.ts` appends a per-fork session suffix to a base database -// identifier. Stripping that suffix yields a value shared across every fork of -// the run, to which we add a stable `template` marker. globalSetup derives from -// the raw (un-suffixed) base; the forks derive from their suffixed config value -// at restore time — because both strip the suffix, they resolve to the identical -// template. - -// MySQL: the per-fork suffix is `_<8 hex>` appended to a database name. -const deriveMySQLTemplateDatabase = (database) => { - const base = database.replace(/_[a-f0-9]{8}$/i, ''); - return `${base}_template`; -}; - -// SQLite: the per-fork suffix is `-pool_` (or `-<8 hex>` when there is no -// VITEST_POOL_ID) inserted before the `.db` extension — see vitest-setup-db.ts. -// Strip whichever suffix is present, then append a `-template.db` marker. The -// marker is distinct from every per-fork name (no fork is named `template`), so -// the template file can never collide with a `pool_N.db` a fork would open. -// globalSetup passes the un-suffixed base (e.g. `/tmp/ghost-test.db`); a fork -// passes its own suffixed filename — both resolve to `-template.db`. -const deriveSQLiteTemplateFilename = (filename) => { - const withoutDb = filename.replace(/\.db$/i, ''); - const base = withoutDb.replace(/-(?:pool_\d+|[a-f0-9]{8})$/i, ''); - return `${base}-template.db`; -}; - -module.exports = { - deriveMySQLTemplateDatabase, - deriveSQLiteTemplateFilename, -}; diff --git a/ghost/core/test/utils/db-template.js b/ghost/core/test/utils/db-template.js index 159de742b4a..045d497fd88 100644 --- a/ghost/core/test/utils/db-template.js +++ b/ghost/core/test/utils/db-template.js @@ -1,6 +1,5 @@ const debug = require('@tryghost/debug')('test:dbTemplate'); const path = require('path'); -const fs = require('fs-extra'); const knex = require('knex'); const KnexMigrator = require('knex-migrator'); @@ -9,17 +8,13 @@ const db = require('../../core/server/data/db'); const schemaModule = require('../../core/server/data/schema'); const schemaTables = Object.keys(schemaModule.tables); const schemaViews = schemaModule.views || {}; -const { - deriveMySQLTemplateDatabase, - deriveSQLiteTemplateFilename, -} = require('./db-template-paths'); // A migrated + seeded database is expensive to build (full knex-migrator init: // create every table, record all ~120 versioned migrations as applied, and -// insert every default fixture). On MySQL each step is a network round-trip; on -// sqlite each is a file write. The DB-suite runner's `isolate:true` projects run -// every test FILE in a fresh fork, so without help each file pays that full init -// once — the bulk of the acceptance-test runtime regression. +// insert every default fixture). On MySQL each step is a network round-trip. The +// DB-suite runner's `isolate:true` projects run every test FILE in a fresh fork, +// so without help each file pays that full init once — the bulk of the +// acceptance-test runtime regression. // // Instead we build ONE migrated + seeded "template" database for the whole run // (in the vitest globalSetup, before any fork spawns) and have each fork RESTORE @@ -27,18 +22,6 @@ const { // with a cheap bulk copy. // // MySQL restores via a same-server `SHOW CREATE TABLE` + `INSERT ... SELECT`. -// sqlite restores by ATTACHing the template file to the fork's connection, -// replaying the template's schema from its sqlite_master, and bulk-copying every -// table with `INSERT ... SELECT`. We deliberately do NOT copy the template .db -// file over a fork's open connection — that approach destabilized sqlite read -// order for order-dependent tests and was reverted; building the fork -// DB from the template's CONTENTS via SQL keeps physical row order identical to a -// fresh init. -// -// Readiness is published from globalSetup to the forks via an env var (forks -// inherit the main process env at spawn time). When it is unset — e.g. -// `test:single` on one file with no globalSetup — the callers fall back to a full -// knex-migrator init, so behaviour is unchanged outside the orchestrated run. // // SCOPE: only the db.reset() provisioning path (agentProvider-based e2e / e2e-api // / e2e-* suites) uses this. The getFixtureOps `testUtils.setup()` path @@ -46,47 +29,25 @@ const { // that path still does a full init — those suites run isolate:true (a fresh // process per file), so the per-file boot, not provisioning, is their cost. -const TEMPLATE_ENV_VAR = 'GHOST_TEST_DB_TEMPLATE_READY'; - const getResetTables = () => { return schemaTables.concat(['migrations']); }; -// Client detection from config (NOT db.knex) for the build/teardown paths, which -// run in globalSetup where touching db.knex would bind Ghost's singleton -// connection to a template location. -const configuredClientIsSQLite = () => - ['sqlite3', 'better-sqlite3'].includes(config.get('database:client')); - -/** - * Whether the shared template has been built for this run (published by - * globalSetup). Callers use this to choose a cheap restore over a full init. - * @returns {boolean} - */ -const hasTemplate = () => { - return process.env[TEMPLATE_ENV_VAR] === '1'; +const deriveMySQLTemplateDatabase = (database, runId) => { + return `${database}_${runId}_template`; }; /** - * Resolve the template database name from the CURRENT fork connection's config. - * config holds the suffixed per-fork value; the pure deriver strips that suffix - * so this resolves to the same template globalSetup built from the un-suffixed - * base. + * Resolve the template database name for the current run. + * globalSetup publishes the unsuffixed base before workers spawn, so every fork + * resolves the same template database. * @returns {string} */ const getForkTemplateDatabase = () => { - return deriveMySQLTemplateDatabase(config.get('database:connection:database')); -}; - -/** - * Resolve the template sqlite filename from the CURRENT fork connection's config. - * config holds the suffixed per-fork filename (`-pool_N.db`); the pure - * deriver strips that suffix so this resolves to the same `-template.db` - * globalSetup built from the un-suffixed base. - * @returns {string} - */ -const getForkTemplateFilename = () => { - return deriveSQLiteTemplateFilename(config.get('database:connection:filename')); + return deriveMySQLTemplateDatabase( + process.env.GHOST_TEST_DB_BASE, + process.env.GHOST_TEST_DB_RUN_ID, + ); }; /** @@ -120,47 +81,11 @@ const ensureForkDatabaseExists = async () => { * knex-migrator reset + init. A fresh KnexMigrator is constructed AFTER the config * override so it reads the template location via MigratorConfig.js. * - * @param {{mysqlBase?: string, sqliteBase?: string}} base the run's base DB identifier, un-suffixed + * @param {{mysqlBase: string}} run the run's base DB identifier */ -const buildTemplate = async (base) => { - if (configuredClientIsSQLite()) { - // Build the template .db file at a path that can't collide with any - // per-fork `pool_N.db` (deriveSQLiteTemplateFilename appends a `-template` - // marker no fork uses). Remove any stale file first — knex-migrator's - // reset drops tables but not the file, and a leftover from a previous run - // could carry the wrong schema (mirrors db-utils' sqlite reset, which - // fs.remove()s before init). - const templateFile = deriveSQLiteTemplateFilename(base.sqliteBase); - debug(`Building shared sqlite DB template at ${templateFile}`); - for (const suffix of ['', '-journal', '-wal', '-shm']) { - await fs.remove(`${templateFile}${suffix}`); - } - - // Point Ghost's config at the template file so the KnexMigrator built - // below (which reads config.get('database') via MigratorConfig.js) targets - // it. We replace the whole `database:connection` node rather than the - // `:filename` leaf: CI exports `database__connection__filename` to the main - // process where globalSetup runs (see ci.yml), and nconf's env layer - // shadows a leaf set when an object-level get reads the node — so a leaf - // set would silently build the template into the base file. Setting the - // object node overrides the env layer. (The mysql path sets a leaf safely: - // its db NAME is not exported to the main process — vitest-setup-db.ts sets - // it per fork — so there is no env node to shadow it.) - config.set('database:connection', { - ...config.get('database:connection'), - filename: templateFile, - }); - const knexMigrator = new KnexMigrator({ knexMigratorFilePath: path.join(__dirname, '../..') }); - await knexMigrator.reset({ force: true }); - await knexMigrator.init(); - - process.env[TEMPLATE_ENV_VAR] = '1'; - debug('Shared sqlite DB template ready'); - return; - } - +const buildTemplate = async (run) => { debug('Building shared DB template'); - config.set('database:connection:database', deriveMySQLTemplateDatabase(base.mysqlBase)); + config.set('database:connection:database', deriveMySQLTemplateDatabase(run.mysqlBase, run.runId)); // Construct after the override so MigratorConfig.js captures the template // location. reset({force}) drops the template DB (DROP DATABASE, tolerating @@ -170,123 +95,16 @@ const buildTemplate = async (base) => { await knexMigrator.reset({ force: true }); await knexMigrator.init(); - process.env[TEMPLATE_ENV_VAR] = '1'; debug('Shared DB template ready'); }; -/** - * Restore the current fork's per-process sqlite database from the shared - * template. - * - * ATTACH is per-connection, and the fork's db.knex pool may route successive - * statements to different connections — so the whole restore runs on ONE - * connection pinned from that pool (acquireConnection + `.connection(conn)`). - * Pinning the LIVE pool's connection (rather than opening a second knex to the - * same file) means the restore writes the fork's own inode, so db.knex reads the - * committed result back directly — copying into a separate connection's inode, or - * fs.remove()ing the file out from under the open pool, would leave db.knex on a - * stale empty handle. (Mirrors the short-lived connection the mysql path opens in - * ensureForkDatabaseExists, but here it must be a pool connection, not a new pool.) - * - * The fork .db file is fresh on first provision (deleted at boot, see - * vitest-setup-db.ts), so the restore builds the entire schema + data from the - * template: replay every sqlite_master object's DDL in creation (rowid) order — - * tables, then their indexes, triggers and views, which always follow their table - * in that order — then bulk-copy each table's rows, foreign keys off during the - * load. This keeps physical row order identical to a fresh init — we do - * NOT copy the template file over the connection (the previously-reverted approach). - */ -const restoreFromTemplateSQLite = async () => { - const templateFile = getForkTemplateFilename(); - debug( - `Restoring fork sqlite DB ${config.get('database:connection:filename')} from template ${templateFile}`, - ); - - // ATTACH on a missing file silently creates an empty DB, which would surface - // later as a baffling "no such table: template." mid-restore. Fail loudly - // up front instead if the template (built by globalSetup) is not where the - // fork derived it should be — a path-derivation drift between build and restore. - if (!fs.existsSync(templateFile)) { - throw new Error( - `sqlite DB template not found at ${templateFile} (built by vitest globalSetup)`, - ); - } - - // Pin one connection: ATTACH/DETACH and the schema replay must all run on the - // same connection, and it must be the live pool's so db.knex sees the result. - const connection = await db.knex.client.acquireConnection(); - const run = (sql, bindings) => db.knex.raw(sql, bindings || []).connection(connection); - - try { - await run('ATTACH DATABASE ? AS template', [templateFile]); - - // Every schema object with a CREATE statement, in creation order. Auto - // objects (sqlite_autoindex_*) have NULL sql and so are excluded — they - // are recreated implicitly when their table's DDL replays. sqlite_sequence - // carries non-null DDL but is reserved/auto-created, so skip it here; its - // counters are copied with the row data below. - const objects = ( - await run( - 'SELECT type, name, sql FROM template.sqlite_master WHERE sql IS NOT NULL ORDER BY rowid', - ) - ).filter((object) => object.name !== 'sqlite_sequence'); - - // Disable FK enforcement so tables load in any order and a table's FK can - // reference one not yet populated. Replaying the template's exact CREATE - // TABLE DDL (rather than a column-only copy) preserves its foreign keys, - // making the restore byte-faithful to a fresh init — the same faithfulness - // the mysql path needs. - await run('PRAGMA foreign_keys = OFF'); - try { - for (const object of objects) { - await run(object.sql); - } - - // Copy EVERY table the template holds, derived from its sqlite_master - // rather than getResetTables(): that list omits knex-migrator's own - // `migrations_lock`, whose released-lock row (locked=0) Ghost's boot - // requires — an empty lock table reads as "migration in progress" and - // boot dies with MigrationsAreLockedError. This also picks up - // `sqlite_sequence` (the AUTOINCREMENT counters, auto-created with the - // first AUTOINCREMENT table above) so inserts continue exactly where the - // template left off. Using the template's own table set keeps the copy - // complete and faithful by construction. - const dataTables = ( - await run('SELECT name FROM template.sqlite_master WHERE type = ?', ['table']) - ).map((row) => row.name); - - for (const table of dataTables) { - await run('DELETE FROM ??', [table]); - await run('INSERT INTO ?? SELECT * FROM template.??', [table, table]); - } - } finally { - await run('PRAGMA foreign_keys = ON'); - } - } finally { - // Always detach before releasing the connection — an error mid-restore must - // not return a connection still attached to the template back to the pool. - try { - await run('DETACH DATABASE template'); - } catch (e) { - // nothing attached (ATTACH may have failed) - } - await db.knex.client.releaseConnection(connection); - } -}; - /** * Restore the current fork's per-process database from the shared template. - * Assumes the caller has verified hasTemplate(). Copies every table from the - * template DB into the fork DB (replay the template's exact schema + INSERT ... - * SELECT). On mysql the template is referenced by qualified name on the fork's - * bound connection; on sqlite the template file is ATTACHed (see the sqlite - * helper above). + * Copies every table from the template DB into the fork DB by replaying the + * exact schema and copying its data. The template is referenced by qualified + * name on the fork's bound connection. */ const restoreFromTemplate = async () => { - if (configuredClientIsSQLite()) { - return restoreFromTemplateSQLite(); - } - const templateDb = getForkTemplateDatabase(); debug('Restoring fork DB from template'); @@ -333,53 +151,60 @@ const restoreFromTemplate = async () => { // them once). Recreate them here from the schema definitions, exactly as // migrations/init/1-create-tables.js does — through commands.createViewOrReplace // so the fork's views get the same SQL SECURITY INVOKER as a fully-migrated one - // (a plain knex createViewOrReplace would default to DEFINER on MySQL). (sqlite - // copies views as part of the sqlite_master replay above, so this is mysql-only.) + // (a plain knex createViewOrReplace would default to DEFINER on MySQL). for (const [name, sql] of Object.entries(schemaViews)) { await schemaModule.commands.createViewOrReplace(name, sql, db.knex); } }; /** - * Drop the shared template database. Called from globalSetup teardown. Best - * effort — on CI the whole server is ephemeral; this is for local hygiene and to - * avoid a stale template surviving into the next run. + * Drop the shared template and every worker database for this run. Called from + * globalSetup teardown after all workers have exited. Best effort so cleanup + * cannot hide a test failure. * - * @param {{mysqlBase?: string, sqliteBase?: string}} base the run's base DB id + * @param {{mysqlBase: string, runId: string}} run the run's database identifiers */ -const dropTemplate = async (base) => { - if (configuredClientIsSQLite()) { - // knex-migrator's reset drops tables but not the file; delete the template - // file (+ sidecars) outright. - try { - const templateFile = deriveSQLiteTemplateFilename(base.sqliteBase); - for (const suffix of ['', '-journal', '-wal', '-shm']) { - await fs.remove(`${templateFile}${suffix}`); - } - } catch (err) { - debug(`Failed to drop sqlite template (ignored): ${err.message}`); - } - return; - } +const dropRunDatabases = async (run) => { try { // Point config at the template and let knex-migrator reset({force}) drop // that database — reusing the same connection path build used, so we never // bind Ghost's singleton db.knex to a template. - config.set('database:connection:database', deriveMySQLTemplateDatabase(base.mysqlBase)); + config.set( + 'database:connection:database', + deriveMySQLTemplateDatabase(run.mysqlBase, run.runId), + ); const knexMigrator = new KnexMigrator({ knexMigratorFilePath: path.join(__dirname, '../..') }); await knexMigrator.reset({ force: true }); } catch (err) { debug(`Failed to drop template (ignored): ${err.message}`); } + + const connectionConfig = config.get('database:connection'); + const connectionWithoutDb = { ...connectionConfig }; + delete connectionWithoutDb.database; + const admin = knex({ + client: config.get('database:client'), + connection: connectionWithoutDb, + }); + try { + const [rows] = await admin.raw('SHOW DATABASES'); + const prefix = `${run.mysqlBase}_${run.runId}_`; + const workerDatabases = rows + .map((row) => Object.values(row)[0]) + .filter((name) => typeof name === 'string' && name.startsWith(prefix)); + + for (const workerDatabase of workerDatabases) { + await admin.raw('DROP DATABASE IF EXISTS ??', [workerDatabase]); + } + } catch (err) { + debug(`Failed to drop worker databases (ignored): ${err.message}`); + } finally { + await admin.destroy(); + } }; module.exports = { - TEMPLATE_ENV_VAR, - deriveMySQLTemplateDatabase, - deriveSQLiteTemplateFilename, - configuredClientIsSQLite, - hasTemplate, buildTemplate, restoreFromTemplate, - dropTemplate, + dropRunDatabases, }; diff --git a/ghost/core/test/utils/db-utils.js b/ghost/core/test/utils/db-utils.js index 2652cb000dd..d5dd9d5a223 100644 --- a/ghost/core/test/utils/db-utils.js +++ b/ghost/core/test/utils/db-utils.js @@ -1,7 +1,6 @@ const debug = require('@tryghost/debug')('test:dbUtils'); // Utility Packages -const fs = require('fs-extra'); const path = require('path'); const KnexMigrator = require('knex-migrator'); // Resolve MigratorConfig.js from the package root explicitly rather than via @@ -9,7 +8,6 @@ const KnexMigrator = require('knex-migrator'); // worker threads cannot chdir. From ghost/core this is the same path, so it // is a no-op for the standalone mocha/vitest runs. const knexMigrator = new KnexMigrator({ knexMigratorFilePath: path.join(__dirname, '../..') }); -const DatabaseInfo = require('@tryghost/database-info'); // Ghost Internals const config = require('../../core/shared/config'); @@ -21,26 +19,9 @@ const schemaTables = Object.keys(schema); const urlServiceUtils = require('./url-service-utils'); const dbTemplate = require('./db-template'); -let dbInitialized = false; let mysqlSnapshotDatabase = null; const mysqlSnapshotTablePrefix = '__ghost_snapshot_'; -/** - * Checks if the current active connection is a MySQL database - * @returns {boolean} isMySQL - */ -module.exports.isMySQL = () => { - return DatabaseInfo.isMySQL(db.knex); -}; - -/** - * Checks if the current active connection is a SQLite database - * @returns {boolean} isSQLite - */ -module.exports.isSQLite = () => { - return DatabaseInfo.isSQLite(db.knex); -}; - /** * Reset * - restores the DB to a fresh state with the default fixtures in place @@ -50,50 +31,18 @@ module.exports.isSQLite = () => { * @param {boolean} options.truncate whether to truncate rather thann fully reset */ module.exports.reset = async ({ truncate } = { truncate: false }) => { - if (module.exports.isSQLite()) { - const filename = config.get('database:connection:filename'); - const filenameOrig = `${filename}-orig`; - - if (dbInitialized) { - await fs.copyFile(filenameOrig, filename); - } else if (dbTemplate.hasTemplate()) { - // First provision in this fork: build the schema + fixtures from the - // run's shared (migrated + seeded) template — ATTACH the template file - // and bulk-copy it onto db.knex's own connection — instead of a full - // per-file migrate+seed. Then snapshot to `-orig` so later - // in-fork resets take the fast file-copy path above. The fork file is - // already fresh (deleted at boot, see vitest-setup-db.ts), and the - // restore writes db.knex's inode, so we must NOT fs.remove() it here — - // that would strand db.knex on a stale empty handle. - await dbTemplate.restoreFromTemplate(); - - await fs.copyFile(filename, filenameOrig); - dbInitialized = true; - } else { - await fs.remove(filename); - await fs.remove(`${filename}-journal`); - await fs.remove(filenameOrig); - - // Do a full database reset & initialisation + if (truncate) { + // Perform a fast reset by tearing down all the tables and inserting the fixtures + try { + await resetMySQLFromSnapshot(); + } catch (err) { + // If it fails, try a normal restore await forceReinit(); - - await fs.copyFile(filename, filenameOrig); - dbInitialized = true; + await createMySQLSnapshot(); } } else { - if (truncate) { - // Perform a fast reset by tearing down all the tables and inserting the fixtures - try { - await resetMySQLFromSnapshot(); - } catch (err) { - // If it fails, try a normal restore - await forceReinit(); - await createMySQLSnapshot(); - } - } else { - // Do a full database reset + initialisation - await forceReinit(); - } + // Do a full database reset + initialisation + await forceReinit(); } }; @@ -118,18 +67,6 @@ module.exports.teardown = async () => { * @param {string} tableName - the table to truncate */ module.exports.truncate = async (tableName) => { - if (module.exports.isSQLite()) { - const [foreignKeysEnabled] = await db.knex.raw('PRAGMA foreign_keys;'); - if (foreignKeysEnabled.foreign_keys) { - await db.knex.raw('PRAGMA foreign_keys = OFF;'); - } - await db.knex(tableName).truncate(); - if (foreignKeysEnabled.foreign_keys) { - await db.knex.raw('PRAGMA foreign_keys = ON;'); - } - return; - } - await db.knex.raw('SET FOREIGN_KEY_CHECKS=0;'); await db.knex(tableName).truncate(); await db.knex.raw('SET FOREIGN_KEY_CHECKS=1;'); @@ -162,17 +99,9 @@ const isMySQLSnapshotCurrent = () => { const resetMySQLFromSnapshot = async () => { if (!isMySQLSnapshotCurrent()) { - if (dbTemplate.hasTemplate()) { - // First provision in this fork: load the schema + fixtures from the - // run's shared (migrated + seeded) template — a same-server bulk - // table copy rather than a full migrate+seed — then build the - // per-process snapshot tables so later in-fork resets take the fast - // restoreMySQLSnapshot path. - await dbTemplate.restoreFromTemplate(); - } else { - await truncateAll(); - await knexMigrator.init({ only: 3 }); - } + // First provision in this fork: load the schema + fixtures from the run's + // shared template, then build snapshot tables for later in-fork resets. + await dbTemplate.restoreFromTemplate(); await createMySQLSnapshot(); return; } @@ -181,10 +110,6 @@ const resetMySQLFromSnapshot = async () => { }; const createMySQLSnapshot = async () => { - if (!module.exports.isMySQL()) { - return; - } - const tables = getResetTables(); for (const table of tables) { @@ -223,10 +148,6 @@ const restoreMySQLSnapshot = async () => { }; const dropMySQLSnapshots = async () => { - if (!module.exports.isMySQL()) { - return; - } - mysqlSnapshotDatabase = null; try { @@ -245,42 +166,13 @@ const dropMySQLSnapshots = async () => { /** * Internal helper to attempt to truncate all tables as fast as possible - * Has to run in a transaction for MySQL, otherwise the foreign key check does not work. - * Sqlite3 has no truncate command. + * Has to run in a transaction, otherwise the foreign key check does not work. */ const truncateAll = async () => { debug('Database teardown'); const tables = getResetTables(); - if (module.exports.isSQLite()) { - try { - const [foreignKeysEnabled] = await db.knex.raw('PRAGMA foreign_keys;'); - if (foreignKeysEnabled.foreign_keys) { - await db.knex.raw('PRAGMA foreign_keys = OFF;'); - } - - for (const table of tables) { - await db.knex.raw('DELETE FROM ' + table + ';'); - } - - if (foreignKeysEnabled.foreign_keys) { - await db.knex.raw('PRAGMA foreign_keys = ON;'); - } - - return; - } catch (err) { - // CASE: table does not exist - if (err.errno === 1) { - return Promise.resolve(); - } - - throw err; - } finally { - debug('Database teardown end'); - } - } - await db.knex.transaction(async (trx) => { try { await db.knex.raw('SET FOREIGN_KEY_CHECKS=0;').transacting(trx); diff --git a/ghost/core/test/utils/fixture-utils.js b/ghost/core/test/utils/fixture-utils.js index 1aacd8114df..0e15251fd47 100644 --- a/ghost/core/test/utils/fixture-utils.js +++ b/ghost/core/test/utils/fixture-utils.js @@ -508,7 +508,7 @@ const fixtures = { 'Super Editor': DataGenerator.Content.roles[6].id, }; - // CASE: if empty db will throw SQLITE_MISUSE, hard to debug + // Avoid an opaque database error from an empty insert. if (_.isEmpty(permsToInsert)) { return Promise.reject(new Error('no permission found:' + obj)); } diff --git a/ghost/core/test/utils/vitest-global-db-setup.ts b/ghost/core/test/utils/vitest-global-db-setup.ts index ccab22c0ccd..cddd9b5249c 100644 --- a/ghost/core/test/utils/vitest-global-db-setup.ts +++ b/ghost/core/test/utils/vitest-global-db-setup.ts @@ -5,49 +5,128 @@ // RESTORES from it when it first provisions its per-process DB (see // test/utils/db-utils.js) instead of running a full migrate+seed per file. That // per-file init is the dominant cost of the acceptance-test runtime regression. -// MySQL restores from a same-server template via -// a bulk table copy; sqlite ATTACHes the template file and bulk-copies it onto -// the fork's own connection (never copying the file over an open connection — a -// previously-reverted approach). Both keep the restore byte-faithful to a fresh -// init. +// MySQL restores from a same-server template via a bulk table copy that keeps +// the restore byte-faithful to a fresh init. // -// The fork learns the template is ready via an inherited env var — env set here, -// before the forks spawn, is inherited by them. We point the DB config at the -// template location while building; the forks derive the same location from -// their own (session-suffixed) config value via the shared pure helpers in -// db-template-paths.js. +// The forks inherit a run ID and base database name set here before they spawn. +// Those values give every worker a unique, discoverable database name so global +// teardown can remove all schemas created by this run. // Register tsx's CommonJS hook so requiring Ghost's .ts sources works here too // (mirrors vitest-setup-db.ts). Must run before any Ghost source is required. require('tsx/cjs'); +const crypto = require('crypto'); +const knex = require('knex'); // Reject vitest's own NODE_ENV='test' default (Ghost has no config.test.json); -// keep any `testing*` value (CI uses `testing-mysql`), else default to `testing`. -// Mirrors vitest-setup-db.ts so the templates are built under the same env the -// forks will run under. -process.env.NODE_ENV = process.env.NODE_ENV?.startsWith('testing') - ? process.env.NODE_ENV - : 'testing'; +// use the MySQL test environment. Mirrors vitest-setup-db.ts so the template is +// built under the same environment the forks run under. +process.env.NODE_ENV = 'testing-mysql'; process.env.WEBHOOK_SECRET = process.env.WEBHOOK_SECRET || 'TEST_STRIPE_WEBHOOK_SECRET'; +const portBlockSize = 128; +// Keep every worker port four digits long, matching the canonical snapshot port +// (2369). Response bodies normalize dynamic URLs before comparison, but their +// content-length headers are calculated first and therefore depend on port width. +const portBlockCount = 54; +const firstPortBlock = 3000; + +const reservePortBlock = async (runId: string) => { + const config = require('../../core/shared/config'); + const connectionConfig = { ...config.get('database:connection') }; + delete connectionConfig.database; + const admin = knex({ + client: config.get('database:client'), + connection: connectionConfig, + }); + const connection = await admin.client.acquireConnection().catch(async (err: unknown) => { + await admin.destroy(); + throw err; + }); + const firstCandidate = parseInt(runId, 16) % portBlockCount; + const closeAdmin = async () => { + try { + await admin.client.releaseConnection(connection); + } catch { + // Closing the pool below also releases its MySQL advisory locks. + } + try { + await admin.destroy(); + } catch { + // Best effort during setup/teardown cleanup. + } + }; + + try { + for (let offset = 0; offset < portBlockCount; offset += 1) { + const block = (firstCandidate + offset) % portBlockCount; + const lockName = `ghost-test-port-block-${block}`; + const [rows] = await admin + .raw('SELECT GET_LOCK(?, 0) AS acquired', [lockName]) + .connection(connection); + + if (rows[0].acquired === 1) { + return { + portBase: firstPortBlock + block * portBlockSize, + release: async () => { + try { + await admin.raw('SELECT RELEASE_LOCK(?)', [lockName]).connection(connection); + } catch { + // Closing the connection also releases its advisory locks. + } + await closeAdmin(); + }, + }; + } + } + } catch (err) { + await closeAdmin(); + throw err; + } + + await closeAdmin(); + throw new Error('No MySQL test port block is available'); +}; + export default async function setup() { // The run's BASE (un-suffixed) DB identifier. In this main process the base // env vars carry no per-fork session suffix, so deriving template locations // from them yields exactly the values the forks compute from their suffixed // config. Captured before loading config so it reflects the true base. - const base = { - sqliteBase: process.env.database__connection__filename || '/tmp/ghost-test.db', + const runId = crypto.randomBytes(4).toString('hex'); + const run = { mysqlBase: process.env.database__connection__database || 'ghost_testing', + runId, }; // Load Ghost's runtime overrides (nconf wiring) and the template builder. require('../../core/server/overrides'); - const { buildTemplate, dropTemplate } = require('./db-template'); + const { buildTemplate, dropRunDatabases } = require('./db-template'); + const portBlock = await reservePortBlock(runId); + + process.env.GHOST_TEST_DB_BASE = run.mysqlBase; + process.env.GHOST_TEST_DB_RUN_ID = run.runId; + process.env.GHOST_TEST_PORT_BASE = String(portBlock.portBase); + process.env.GHOST_TEST_PORT_BLOCK_SIZE = String(portBlockSize); - await buildTemplate(base); + try { + await buildTemplate(run); + } catch (err) { + try { + await dropRunDatabases(run); + } finally { + await portBlock.release(); + } + throw err; + } - // Teardown: drop the template once all forks have exited. Best effort. + // Teardown: drop the template and every worker database once all forks have + // exited. Best effort. return async () => { - await dropTemplate(base); + try { + await dropRunDatabases(run); + } finally { + await portBlock.release(); + } }; } diff --git a/ghost/core/test/utils/vitest-setup-db.ts b/ghost/core/test/utils/vitest-setup-db.ts index 0799f0809ae..12f32e21d95 100644 --- a/ghost/core/test/utils/vitest-setup-db.ts +++ b/ghost/core/test/utils/vitest-setup-db.ts @@ -27,11 +27,10 @@ const chalk = require('chalk'); // run before any Ghost source is required below. require('tsx/cjs'); -// Reject vitest's own `NODE_ENV='test'` default (Ghost has no config.test.json); -// keep any `testing*` value (CI uses `testing-mysql`), else default to `testing`. -process.env.NODE_ENV = process.env.NODE_ENV?.startsWith('testing') - ? process.env.NODE_ENV - : 'testing'; +// DB-backed suites run against MySQL. Reject vitest's own `NODE_ENV='test'` +// default (Ghost has no config.test.json) by setting the MySQL test environment +// before config loads. +process.env.NODE_ENV = 'testing-mysql'; process.env.WEBHOOK_SECRET = process.env.WEBHOOK_SECRET || 'TEST_STRIPE_WEBHOOK_SECRET'; // Generate unique session values for database and port BEFORE loading Ghost, so @@ -39,55 +38,19 @@ process.env.WEBHOOK_SECRET = process.env.WEBHOOK_SECRET || 'TEST_STRIPE_WEBHOOK_ // inherit these env vars and get the same values when they load a fresh nconf // instance. // -// Each worker is its own process, so it gets its own database — that's what lets -// the DB suites run fork-parallel. The per-fork sessionId is appended -// even to a CI-pinned *base*: the sqlite leg exports a single -// database__connection__filename=/dev/shm/ghost-test.db for the whole job, so -// without a unique suffix every fork would hammer the same file. (The mysql leg -// pins only host/port, so the database name is generated outright here.) -// -// sqlite names are keyed on VITEST_POOL_ID (1..poolSize, like the port below) so -// a run reuses ~poolSize stable files instead of leaving a fresh random DB in -// /tmp every run — that bounded reuse is what stops local /tmp accumulation. -// A reused file still holds the prior fork's data, though, and Ghost reads it at -// boot (settings cache, url service) before the suite resets — which corrupts -// whichever file lands on the slot (null Owner, stale URLs, bad export). So the -// file is deleted just below, before Ghost loads, so a reused slot boots from -// nothing exactly as a fresh name would. mysql keeps a random per-fork name: it -// has no /tmp to bound (CI databases die with the job) and a random name sidesteps -// the same stale-reuse hazard without a pre-boot DROP. -if (process.env.NODE_ENV.includes('mysql')) { - const mysqlId = crypto.randomBytes(4).toString('hex'); - const mysqlBase = process.env.database__connection__database; - process.env.database__connection__database = mysqlBase - ? `${mysqlBase}_${mysqlId}` - : `ghost_testing_${mysqlId}`; -} else { - const poolSlot = parseInt(process.env.VITEST_POOL_ID || '', 10); - const sqliteId = Number.isInteger(poolSlot) - ? `pool_${poolSlot}` - : crypto.randomBytes(4).toString('hex'); - const sqliteBase = process.env.database__connection__filename; - process.env.database__connection__filename = sqliteBase - ? `${sqliteBase.replace(/\.db$/i, '')}-${sqliteId}.db` - : `/tmp/ghost-test-${sqliteId}.db`; +// Each worker gets its own random database under the run prefix established by +// globalSetup. The prefix lets global teardown discover and remove every schema +// after the workers exit, including locally where the MySQL volume persists. +const mysqlBase = process.env.GHOST_TEST_DB_BASE; +const mysqlRunId = process.env.GHOST_TEST_DB_RUN_ID; +if (!mysqlBase || !mysqlRunId) { + throw new Error('DB test setup requires vitest-global-db-setup.ts'); } - -// Delete this slot's leftover sqlite file (+ sidecars) before Ghost loads, so a -// reused pool name boots from a clean slate — see the note above. SQLITE LEG ONLY: -// on the mysql leg (NODE_ENV testing-mysql) this derived filename is never ours — -// it belongs to a concurrent sqlite run on the same machine, and deleting it out -// from under that run destroys its database mid-write (SQLITE_READONLY). force:true -// makes the sqlite delete a no-op on a slot's first use. -if (!process.env.NODE_ENV.includes('mysql')) { - for (const suffix of ['', '-journal', '-wal', '-shm', '-orig']) { - try { - require('fs').rmSync(process.env.database__connection__filename + suffix, { force: true }); - } catch (e) { - // best effort — a fresh boot recreates it - } - } +if (!process.env.GHOST_TEST_DB_WORKER_DATABASE) { + const mysqlId = crypto.randomBytes(4).toString('hex'); + process.env.GHOST_TEST_DB_WORKER_DATABASE = `${mysqlBase}_${mysqlRunId}_${mysqlId}`; } +process.env.database__connection__database = process.env.GHOST_TEST_DB_WORKER_DATABASE; // Flush this worker's V8 coverage after every file. The external c8 collector // reads NODE_V8_COVERAGE, which Node writes only on a clean process exit — but @@ -109,30 +72,26 @@ if (process.env.NODE_V8_COVERAGE) { }); } -// NOTE: each worker still leaves a DB behind — vitest force-terminates its -// workers, so a process 'exit' handler can't reclaim them. sqlite stays bounded: -// the next worker on a slot deletes the file at boot (see the derivation above) -// and recreates it, so a run reuses at most ~poolSize files in /tmp instead of -// leaving a fresh random one behind every run. mysql names are random per worker -// but ephemeral on CI (the container dies with the job); locally the mysql suite -// is rarely run. - const canonicalTestPort = 2369; // The per-fork port must be unique among forks running concurrently. Each test // file boots a real HTTP server on this port (e2e-api tests hit it via // supertest.agent(config.get('url'))); if two concurrent forks land on the same // port, one Ghost ends up serving the other's requests — or boots unready — and // every request 404s with an HTML body (e.g. the whole invites suite failing -// intermittently). vitest gives each concurrent fork a distinct VITEST_POOL_ID -// (1..poolSize); a recycled slot's port is reused only after its previous fork -// has exited and freed it, so base+poolId never collides among live forks. The -// old `Math.random()` port in a 7630-wide range collided often enough across ~90 -// parallel boots to flake. (The DB name already uses a 2^32 sessionId, which is -// collision-resistant; only the port was under-spread.) +// intermittently). globalSetup reserves a run-scoped low-port block with a +// MySQL advisory lock, and vitest gives each concurrent fork a distinct +// VITEST_POOL_ID within that block. A recycled slot's port is reused only after +// its previous fork has exited and freed it. const poolId = parseInt(process.env.VITEST_POOL_ID || '', 10); -const derivedPort = Number.isInteger(poolId) - ? 2370 + poolId - : 2370 + Math.floor(Math.random() * 7630); +const portBase = parseInt(process.env.GHOST_TEST_PORT_BASE || '', 10); +const portBlockSize = parseInt(process.env.GHOST_TEST_PORT_BLOCK_SIZE || '', 10); +if (!Number.isInteger(portBase) || !Number.isInteger(portBlockSize)) { + throw new Error('DB test setup requires a reserved port block'); +} +if (Number.isInteger(poolId) && (poolId < 1 || poolId >= portBlockSize)) { + throw new Error(`VITEST_POOL_ID ${poolId} is outside the reserved port block`); +} +const derivedPort = portBase + (Number.isInteger(poolId) ? poolId : 0); process.env.server__port = process.env.server__port || String(derivedPort); process.env.url = process.env.url || `http://127.0.0.1:${process.env.server__port}`; const sessionPort = parseInt(process.env.server__port, 10); @@ -189,10 +148,9 @@ const mockManager = require('./e2e-framework-mock-manager'); // // NOTE: vitest runs setup-file hooks per *file*, not once per run like mocha's // root hooks. That's fine for these (disableNetwork is idempotent; the snapshot -// hooks are per-file aware). DB teardown (drop database / remove the sqlite -// file) is deliberately NOT done here for that reason — it would run after every -// file and tear the shared connection down mid-run. The worker is terminated at -// the end of the run instead; the per-session sqlite file lives in /tmp. +// hooks are per-file aware). DB teardown is deliberately NOT done here for that +// reason — it would run after every file and tear the shared connection down +// mid-run. The worker is terminated at the end of the run instead. beforeAll(async () => { if (mochaHooks?.beforeAll) { await mochaHooks.beforeAll(); diff --git a/ghost/core/vitest.config.db.ts b/ghost/core/vitest.config.db.ts index 0e264e96669..062586dbca4 100644 --- a/ghost/core/vitest.config.db.ts +++ b/ghost/core/vitest.config.db.ts @@ -60,13 +60,12 @@ const sharedDbConfig = { sequence: { shuffle: { files: !!process.env.CI } }, setupFiles: ['./test/utils/vitest-setup-db.ts'], resolveSnapshotPath, - // Keep the testing env (CI sets `testing-mysql` on the MySQL leg; default to - // sqlite `testing` locally). Must reject vitest's own `NODE_ENV='test'` - // default — Ghost has no config.test.json, so `test` yields no DB config and - // bookshelf throws "Invalid knex instance". Resolved here in the main - // process, where CI sets the leg's NODE_ENV. + // DB-backed suites run against MySQL in CI and locally. Set the environment + // explicitly to reject vitest's own `NODE_ENV='test'` default — Ghost has no + // config.test.json, so `test` yields no DB config and bookshelf throws + // "Invalid knex instance". env: { - NODE_ENV: process.env.NODE_ENV?.startsWith('testing') ? process.env.NODE_ENV : 'testing', + NODE_ENV: 'testing-mysql', WEBHOOK_SECRET: process.env.WEBHOOK_SECRET || 'TEST_STRIPE_WEBHOOK_SECRET', // Bree runs jobs in worker_threads that inherit this NODE_OPTIONS; tsx lets // them require() Ghost's .ts sources (job files pull in e.g. @@ -132,8 +131,7 @@ export default defineConfig({ // exposes — e.g. migration.test.js can leave a rolled-back // schema that a co-located file then inherits. Per-file // isolation removes it by construction. The e2e project keeps - // isolate:false (it has no such pollution and is fastest that - // way); sqlite per-file init is cheap so the cost here is small. + // isolate:false (it has no such pollution and is fastest that way). isolate: true, include: ['test/integration/**/*.test.{js,ts}'], exclude: ['**/node_modules/**'], diff --git a/nx.json b/nx.json index ef15c1c1b17..bceb8446cbb 100644 --- a/nx.json +++ b/nx.json @@ -43,16 +43,7 @@ }, "test:ci:*": { "cache": true, - "inputs": [ - "default", - "^default", - { - "env": "DB" - }, - { - "env": "NODE_ENV" - } - ] + "inputs": ["default", "^default"] }, "dev": { "dependsOn": ["^dev"], diff --git a/scripts/release.js b/scripts/release.js index c167ff77b50..acdaacce220 100644 --- a/scripts/release.js +++ b/scripts/release.js @@ -1,6 +1,6 @@ import { existsSync } from 'node:fs'; import { join, relative } from 'node:path'; -import { execSync } from 'node:child_process'; +import { execFileSync, execSync } from 'node:child_process'; import { parseArgs as baseParseArgs } from 'node:util'; import semver from 'semver'; import camelcaseKeys from 'camelcase-keys'; @@ -40,6 +40,13 @@ function parseArgs() { }, }); + // `branch` is a workflow_dispatch input and reaches git as an argument. Git + // accepts far more than this (`git check-ref-format` passes shell payloads), + // so hold it to a plain ref name that can't be read as an option either. + if (!/^[A-Za-z0-9][A-Za-z0-9._/-]*$/.test(values.branch)) { + throw new Error(`Invalid branch name: ${values.branch}`); + } + return camelcaseKeys(values); } @@ -50,6 +57,11 @@ function run(cmd, opts = {}) { return result.trim(); } +// No shell, so refs reach git as literal argv entries rather than syntax. +function git(...args) { + return execFileSync('git', args, { cwd: ROOT_DIR, encoding: 'utf8' }).trim(); +} + // Single-quote a path for the shell. function quoteArg(path) { return `'${path.replace(/'/g, `'\\''`)}'`; @@ -186,7 +198,7 @@ async function fetchRequiredCheck(commit, token) { } function remoteHead(branch) { - const output = run(`git ls-remote origin refs/heads/${branch}`); + const output = git('ls-remote', 'origin', `refs/heads/${branch}`); return output.split(/\s/)[0] || null; } @@ -194,13 +206,13 @@ function remoteHead(branch) { // means the branch was rewritten or we have local commits, and silently // resetting would drop work. function fastForwardTo(branch, sha) { - run(`git fetch origin ${branch}`); + git('fetch', 'origin', branch); try { - run(`git merge-base --is-ancestor HEAD ${sha}`); + git('merge-base', '--is-ancestor', 'HEAD', sha); } catch { return false; } - run(`git reset --hard ${sha}`); + git('reset', '--hard', sha); return true; }