jamesfredley commented on PR #523: URL: https://github.com/apache/grails-static-website/pull/523#issuecomment-4984765941
Thanks @sanjana2505006 - this is a strong contribution. I reviewed the guide prose against the companion sample app in full: built it via CI, cross-checked every prose↔snippet include, verified the plugin commands/config against the Apache Grails source, and did a comprehensiveness pass on the migration coverage. The core arc is solid and everything is factually correct about the plugin. **Setup done on my side:** I created the `grails8` branch on `grails-guides/grails-database-migration` (seeded from your sample app, your commits preserved), so `sampleRef` now resolves and CI is green there (`initial` + `complete` unit tests, `complete` integration tests all pass). > [!IMPORTANT] > **Overlapping changes need a companion PR.** Several items below change files the guide `include::`s as snippets (domains, migrations, tests, config). Those live in the **sample app** repo, so please mirror any such change in a **companion PR against the `grails8` branch of [`grails-guides/grails-database-migration`](https://github.com/grails-guides/grails-database-migration)**, so the published guide and the runnable sample never drift. Items tagged **[overlapping]** below belong in that companion PR (and this one). --- ### 1. Sample-app correctness - **[overlapping]** (companion PR on grails-guides repo) - **[MAJOR] Rollback is advertised but breaks.** The guide shows `dbm-rollback-count 1`, but the `redesignTables` phase (raw-SQL data copy + three `dropColumn` changesets) has no `rollback {}` blocks, so `dbm-rollback` fails on `drop-person-zip-1` and would lose data even if it ran. Either add explicit `rollback {}` blocks (re-add columns + restore from `address`) or reframe as expand-migrate-contract and scope the rollback claim to the reversible steps. Good chance to teach designed rollback (see §2). - **[MAJOR] `Address` ownership contradiction.** `create-address-table.groovy` makes `address.person_id` `NOT NULL`, but `AddressSpec` asserts `new Address().validate()` is `true` (no person). An address validates yet can never persist. Pick one contract - recommended: mandatory ownership (`person nullable: false`, keep `person_id NOT NULL`, and change `AddressSpec` to expect a validation error). - **[MAJOR] Integration test never proves the headline feature.** `DatabaseMigrationIntegrationSpec` only checks the final DDL on a clean DB. It never seeds a legacy `person` row before `migrate-address-data-1`, so it doesn't verify the data-migration SQL actually preserves `street_name/city/zip`. Add a migration-path test: apply through `add-address-fields-to-person`, seed legacy data, apply the rest, then assert the `address` row matches and the legacy columns are gone. - **[MINOR]** `initial/src/test/resources/application-test.yml` sets `updateOnStart: true` pointing at a `changelog.groovy` that doesn't exist in `initial/` yet - a trap for anyone adding an `@Integration` test there. Set `updateOnStart: false` until the guide generates the baseline. - **[MINOR]** `initial/` `ApplicationSpec` only asserts `true`; the advertised `./gradlew test` proves nothing. Replace with `DomainUnitTest<Person>` checks (name + age required). ### 2. Comprehensiveness - the part we care most about - **[overlapping]** Current coverage is good (GORM baseline, nullable via `dbm-gorm-diff`, add columns, table redesign + data SQL, basic rollback, unit + integration tests). To make this the definitive database-migration guide it should be, these are the **must-adds** (each touches both the prose and the sample app, so → companion PR too): - **Release runbook: inspect → preview → apply.** `dbm-status`, then `dbm-update-sql pending.sql` to preview, then `dbm-update`. Note `updateOnStart` is fine for local/single-instance, but production should run migrations once before rolling out multiple app instances. - **Rollback as a designed capability.** `rollback {}` blocks, tag a known-good release with `dbm-tag`, preview with `dbm-rollback-sql <tag>`, execute `dbm-rollback <tag>`. (This also resolves the §1 rollback issue.) - **Legacy / existing-DB onboarding.** Contrast `dbm-generate-gorm-changelog` (from domains) with `dbm-generate-changelog` (snapshot a live schema), then `dbm-changelog-sync` to mark it applied without DDL. Covers the very common "prod DB already exists" case. - **Changelog discipline / immutability.** Append-only includes in release order, one changeset per file with unique author+id, never edit an applied changeset (add a corrective one), and what `dbm-clear-checksums` is/isn't for. - **Preconditions.** Guard the destructive redesign with `preConditions(onFail: 'HALT') { tableExists(...); columnExists(...) }` so drift/wrong ordering fails loudly instead of damaging data. Nice follow-ons if you want to go further: indexes/unique constraints via migrations, contexts for env-specific changesets (`--contexts`), and multiple-datasource migrations. (All command names above verified against the current plugin reference.) ### 3. Guide prose / metadata - this PR only - **[MINOR]** Command style is inconsistent: the prose uses `./gradlew runCommand "-Pargs=dbm-*"` while the sample README leads with `./grailsw dbm-*`. Both are valid - please standardize on one (suggest `./grailsw dbm-*` as primary, the Gradle form as the CI/automation fallback). - **[MINOR]** You authored the guide but aren't in `guides.yml` `authors:` (only Nirav Assar + Sergio del Amo are listed on the parent). Please add yourself to the parent `authors:` list. - **FYI - no change needed:** the `requirements: What you will need` bare-scalar TOC key is valid (`YamlTocStrategy` has a string overload; `grails-spring-security` v8 uses the same form). ### 4. Verified correct - no action (FYI) Checked against Apache Grails source: plugin coordinate `org.apache.grails:grails-data-hibernate5-dbmigration` ✅, `./gradlew runCommand "-Pargs=dbm-*"` is a real task ✅, all `dbm-*` commands exist ✅, `updateOnStart` / `updateOnStartFileName` / `dbCreate: none` correct ✅, Groovy changelog DSL (`dropNotNullConstraint`, `addForeignKeyConstraint`, etc.) valid ✅. All 12 TOC keys map 1:1 to the guide files, every snippet include resolves, and `GET /api/persons` is wired up. --- Happy to help get the companion sample-app PR up against `grails8` if that's easier. Nice work overall - the bones here are very good. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
