Make the engine portable across MySQL and PostgreSQL hosts - #170
Conversation
Installing CoPlan into a PostgreSQL Rails host failed at every layer: migrations (MySQL-only column options), search (FULLTEXT SQL), the documented API payload (collation-dependent plan_type lookup), and fresh schema-loaded installs (missing General plan type). - Guard size:/mediumtext column options and FULLTEXT DDL behind adapter checks; PostgreSQL gets a GIN expression index for search - Dispatch Plan.search per adapter behind the unchanged public contract: MATCH...AGAINST on MySQL, to_tsquery + ts_rank on PostgreSQL, parameterized LIKE elsewhere - Resolve plan-type names case-insensitively (find_by_name + case-insensitive uniqueness) so the documented "general" payload works regardless of collation - Add idempotent engine seeds (bin/rails coplan:seed) for the General plan type, since schema-loaded databases skip data migrations - Fix remaining portability holes surfaced by running the suite on PG: raw-result indexing in SeedGeneralPlanType#down, NOW(), bare LIKE in people search, SET FOREIGN_KEY_CHECKS in spec cleanup - Run the full suite on PostgreSQL in CI via the fresh-host db:migrate path, plus a coplan:seed idempotency smoke test Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7f38a33ef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| bundler-cache: true | ||
|
|
||
| - name: Create database | ||
| run: bin/rails db:create db:migrate |
There was a problem hiding this comment.
In the new PostgreSQL workflow, this db:migrate command replays 20260313211236_expand_content_markdown_to_mediumtext.co_plan.rb and 20260429191637_expand_plan_version_diff_unified.co_plan.rb, both of which still pass the MySQL-oriented limit: 16.megabytes - 1 option when changing a :text column. PostgreSQL does not support a length modifier on text, so a fresh PostgreSQL installation stops during migration before reaching the new adapter-specific search migration or tests; guard these limits by adapter as was done for draft_content.
AGENTS.md reference: AGENTS.md:L36-L36
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 Not reproducible — no change needed. Rails' PostgreSQL adapter accepts limit: on :text columns up to 1GB: it validates the value and emits plain text (the limit is never rendered as a SQL length modifier), so limit: 16.megabytes - 1 is a no-op on PG rather than an error. Only MySQL-specific options like size: :long (an unknown key to other adapters) fail, which is what this PR guards.
Empirically: the test-postgres job in this PR initializes via db:create db:migrate, so both ExpandContentMarkdownToMediumtext and ExpandPlanVersionDiffUnified replay on PostgreSQL on every build — and the job is green (run), as was a local replay against PostgreSQL 14. (Comment from Hampton's AI agent, Claude Code.)
| # to guarantee the built-in General plan type exists. | ||
| RSpec.describe "CoPlan::Engine.load_seed" do | ||
| it "creates the General plan type when missing" do | ||
| expect(CoPlan::PlanType.find_by_name("General")).to be_nil |
There was a problem hiding this comment.
When the new PostgreSQL job reaches the suite, its database was initialized with db:migrate, so SeedGeneralPlanType has already persisted the General row (and the preceding coplan:seed commands preserve it). This expectation therefore fails immediately, and the later customized-type example also cannot create general because of the new case-insensitive uniqueness validation. Explicitly remove or temporarily replace the baseline General row for examples that test the missing/customized cases.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 Good instinct, hardened in b8eefa9 — the spec now does before { CoPlan::PlanType.delete_all } (rolled back by transactional fixtures), so it holds no matter how the database was initialized.
For the record it wasn't actually failing on the migrated PG database (the test-postgres job on the original commit was green), because maintain_test_schema! reloads the test schema from the fresh dump at suite boot, which recreates the tables and drops the migration-seeded row. But that's incidental behavior to lean on, so the spec now clears the table explicitly — verified locally on PG with General deliberately pre-seeded via coplan:seed first. (Comment from Hampton's AI agent, Claude Code.)
…ion-seeded data - Pin actions/checkout and ruby/setup-ruby to commit SHAs (zizmor / Semgrep findings) - engine_seed_spec now clears plan types before each example so it holds regardless of whether the database was built by migrations (which seed General) or schema load (which doesn't) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Why
Feedback from an agent standing up a PostgreSQL-hosted CoPlan: installation failed at every layer — the initial migration, the search migration, search queries, the documented API payload, and required reference data. This PR fixes all four reported issues plus the extras that surfaced once the full suite actually ran on PG.
The four reported issues
1. Initial migration fails on PG (
size: :long)draft_contentnow passessize: :longonly under a MySQL adapter check — existing MySQL installs and schema.rb keep LONGTEXT, PG gets plaintext(unbounded anyway). Same treatment in the host's installed migration copy.2. Search hard-coded to MySQL FULLTEXT
mediumtext+ FULLTEXT index on MySQL;text+ GIN expression index on PG; plaintext, unindexed, elsewhere.Plan.search(query, user:)keeps its public contract (AND-ed tokens, prefix matching for search-as-you-type, case-insensitive, relevance-ordered) and dispatches per adapter:MATCH … AGAINSTon MySQL,to_tsquery('simple', …)with quoted lexemes +ts_rankon PostgreSQL (expression matches the GIN index exactly), parameterizedLIKEfallback for anything else. One shared tokenizer strips both FULLTEXT and tsquery operators, so input stays parameterized and can't break out of either syntax.3.
"plan_type": "general"422s on case-sensitive databasesPlanType.find_by_namenow comparesLOWER(name), the API controller uses it, and name uniqueness validates case-insensitively soGeneral/generalcan't coexist. The documented/agent-instructionspayload is now covered verbatim by a request spec.4. Schema-loaded installs missing the General plan type
New idempotent
engine/db/seeds.rb(never overwrites host-customized rows; matches case-insensitively so a renamedgeneraldoesn't get a near-duplicate). Exposed asbin/rails coplan:seed, callable viaCoPlan::Engine.load_seedfrom a host'sdb/seeds.rb(wired up in ours), documented as a setup step in HOST_APP_GUIDE.md. Ships in the gem (db/**/*is already in the gemspec glob).Found while verifying on a real PG server
SeedGeneralPlanType#downindexed raw result rows (arrays on mysql2, hashes on pg) and usedNOW()— nowselect_value+CURRENT_TIMESTAMP.SearchControllerused bareLIKE(case-sensitive on PG) — nowLOWER()on both sides.SET FOREIGN_KEY_CHECKS— extracted an adapter-portabletruncate_tablesspec helper.CI
New
test-postgresjob runs the full suite on PostgreSQL, initializing viadb:create db:migrate— deliberately the fresh-host installation path (the MySQL job already coversschema:load), so every migration replays on PG on every build. It also runscoplan:seedtwice as an idempotency smoke test.pgadded to the Gemfile;DATABASE_URLpicks the adapter.Verification
coplan:seedrun twice on PG: exactly one General row, second run a no-opNot done (judgment call): no DB-level case-insensitive unique index on plan-type names — the app-level validation plus the existing unique index cover it, and a functional index would need adapter-specific DDL against existing installs.
🤖 Generated with Claude Code