Skip to content

Make the settings seed non-destructive on re-run (seed idempotency)#266

Open
fabiodalez-dev wants to merge 3 commits into
mainfrom
fix/seed-idempotency-preserve-settings
Open

Make the settings seed non-destructive on re-run (seed idempotency)#266
fabiodalez-dev wants to merge 3 commits into
mainfrom
fix/seed-idempotency-preserve-settings

Conversation

@fabiodalez-dev

@fabiodalez-dev fabiodalez-dev commented Jul 18, 2026

Copy link
Copy Markdown
Owner

Every system_settings INSERT in the installer's data_{it,en,de,fr}.sql used

ON DUPLICATE KEY UPDATE setting_value = VALUES(setting_value), description = VALUES(description), updated_at = NOW()

which forces setting_value back to the (usually empty) seed value on any re-run — so a re-seed wiped admin-configured values: social links (incl. the new Telegram one), the sharing-providers list, loan limits, etc. This is the root of the #260 re-seed finding (which CodeRabbit withdrew as fresh-install-only given the .installed guard — but a non-destructive seed is strictly safer, and it's the behaviour @fabiodalez-dev asked for).

Change: dropped setting_value = VALUES(setting_value) from every such clause. A fresh install still INSERTs the seed value (rows don't exist yet); a re-seed now preserves whatever the admin set, only refreshing description/updated_at. Matches the project's own INSERT-IGNORE seeding principle. The languages table's ON DUPLICATE is untouched — its columns are ships-with metadata that should refresh.

Verified by loading schema + the modified seed into a scratch DB (0 errors, six socials seeded) and re-seeding after configuring social_telegram — the value survives. New tests/seed-idempotency.unit.php locks the contract: static (no destructive setting_value upsert in any locale) + behavioural (re-seed preserves an admin value; a fresh key still takes the seed default). 12 checks.

Summary by CodeRabbit

  • Correzioni

    • I riesegui dell’installer preservano i valori di configurazione già personalizzati.
    • Le descrizioni delle impostazioni vengono aggiornate senza sovrascrivere le configurazioni esistenti.
    • Il comportamento è stato uniformato per i seed in tedesco, inglese, francese e italiano.
  • Test

    • Aggiunti controlli automatici per verificare l’idempotenza dei seed e la corretta gestione delle nuove impostazioni.

Every system_settings INSERT in the installer's data_{it,en,de,fr}.sql used
ON DUPLICATE KEY UPDATE setting_value = VALUES(setting_value), ..., which forces
setting_value back to the (usually empty) seed value on any re-run — so a
re-seed wiped admin-configured values (social links incl. Telegram, sharing
providers, loan limits). Dropped setting_value from every such ON DUPLICATE
clause: a fresh install still INSERTs the seed value, but a re-seed now
PRESERVES whatever the admin set, only refreshing the description/updated_at
metadata. This matches the project's own INSERT-IGNORE seeding principle
(never overwrite admin-customised rows). The languages table's ON DUPLICATE is
untouched — its columns are ships-with metadata that should refresh.

Verified by loading schema + the modified seed into a scratch DB (0 errors, six
socials seeded) and re-seeding after configuring social_telegram — the value
survives. New tests/seed-idempotency.unit.php locks the contract (static: no
destructive setting_value upsert in any locale; behavioural: re-seed preserves
an admin value while a fresh key still takes the seed default). 12 checks.

Addresses the CodeRabbit #260 re-seed finding at the root (which CodeRabbit
withdrew as fresh-install-only, but a non-destructive seed is strictly safer).
@coderabbitai

coderabbitai Bot commented Jul 18, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

An error occurred during the review process. Please try again later.

📝 Walkthrough

Walkthrough

I seed SQL localizzati non sovrascrivono più setting_value sulle chiavi già presenti. Un nuovo test verifica staticamente le clausole e comportamentalmente l’inserimento, la preservazione dei valori configurati e l’aggiornamento dei metadati.

Changes

Idempotenza dei seed

Layer / File(s) Summary
Upsert dei seed senza sovrascrittura
installer/database/data_*.sql
Le clausole ON DUPLICATE KEY UPDATE aggiornano description e updated_at, preservando setting_value per le impostazioni dei prestiti, del catalogo e della condivisione.
Verifica statica dei seed
tests/seed-idempotency.unit.php
Il test controlla i seed localizzati e fallisce se ricompare l’assegnazione setting_value = VALUES(setting_value).
Verifica comportamentale dell’idempotenza
tests/seed-idempotency.unit.php
Il test MySQL verifica inserimenti, preservazione del valore esistente, aggiornamento della descrizione, nuove chiavi e codice di uscita finale.

Estimated code review effort: 3 (Moderate) | ~20 minuti

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed Il titolo descrive correttamente la modifica principale: rendere non distruttivo il seed delle impostazioni ai rerun.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/seed-idempotency-preserve-settings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/seed-idempotency.unit.php`:
- Around line 41-44: Update the seed-idempotency validation around the existing
system_settings SQL check to parse and inspect every system_settings INSERT
statement, not merely search for one ON DUPLICATE clause. Require each statement
to include non-destructive duplicate-key handling, including the statements in
both locale seed files, so execution cannot stop on an unhandled duplicate key.
- Line 21: Replace the direct PHP invocation for the seed-idempotency test with
execution through the mandatory run-e2e.sh wrapper, passing --workers=1 to
enforce serial execution. Update the affected test instructions and command path
consistently, while preserving the existing pass/fail behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 9dc66390-ce55-4a6f-b2a1-7a7638bcd2b9

📥 Commits

Reviewing files that changed from the base of the PR and between 221f018 and 97415dc.

📒 Files selected for processing (5)
  • installer/database/data_de_DE.sql
  • installer/database/data_en_US.sql
  • installer/database/data_fr_FR.sql
  • installer/database/data_it_IT.sql
  • tests/seed-idempotency.unit.php

Comment thread tests/seed-idempotency.unit.php
Comment thread tests/seed-idempotency.unit.php
…ructive

The events_page_enabled and catalogue_mode seed rows shipped as plain
INSERTs without an ON DUPLICATE handler, so a re-seed aborted on the
duplicate key before reaching the settings whose upsert this branch had
already made non-destructive. Add ON DUPLICATE KEY UPDATE (refresh
description/updated_at only, never setting_value) to both rows in
data_it_IT.sql and data_en_US.sql so the whole settings seed is fully
idempotent: a fresh install still lands the default, a re-seed preserves
whatever the admin configured, and no statement errors on a second run.

Strengthen tests/seed-idempotency.unit.php to parse every
system_settings INSERT and assert each one carries a non-destructive
duplicate handler, instead of only checking that some ON DUPLICATE
clause exists.
The admin-only advanced.custom_header_css value is rendered verbatim
inside an inline <style> block on every frontend page. Content inside
<style> is HTML raw text, so the only escape into script execution is
the literal </style end tag. The previous sanitiser
(ContentSanitizer::normalizeExternalAssets) only normalised Google-Fonts
URLs, so a value such as </style><script>...</script> broke out of the
style context and executed arbitrary JavaScript for every visitor — a
stored XSS gated behind admin write on a field labelled CSS-only.

Add ContentSanitizer::sanitizeCustomCss(), which runs the existing
font normalisation and then strips every <style>/<script> tag boundary
(plus HTML comment / CDATA markers) — sequences that never appear in
valid CSS. Apply it at the render boundary (frontend/layout.php, so
already-stored values are neutralised too) and at both save boundaries
in SettingsController, and harden ThemeController's per-theme custom CSS
save to use it instead of a partial <script>-pair strip.

Tests: custom-css-injection.unit.php (42 assertions — 16 breakout
payloads neutralised, valid CSS preserved byte-for-byte) and
custom-css-injection.spec.js (real admin form -> public frontend
pipeline: payload never executes, legit CSS still round-trips).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant