Skip to content

Pick jsonb on PostgreSQL under either of its adapter names (1.1.1) - #46

Merged
rameerez merged 1 commit into
mainfrom
fix/postgis-jsonb
Oct 18, 2026
Merged

rameerez merged 1 commit into
mainfrom
fix/postgis-jsonb

Conversation

@rameerez

Copy link
Copy Markdown
Owner

The install and upgrade migrations, and the demo app's, chose jsonb for JSON columns only when the adapter was called "PostgreSQL"; activerecord-postgis-adapter reports "PostGIS", and such an app got plain json columns. Same fix as rameerez/wallets#14 (found while moving CarHey to wallets 0.4.2). A template test pins the column type per adapter name across all four migrations. CHANGELOG entry under 1.1.1.

🤖 Generated with Claude Code

https://claude.ai/code/session_01FKwnuouKrCTFGCidHgwSuS

The install and upgrade migrations, and the demo app's, chose jsonb for
JSON columns only when the adapter was called "PostgreSQL";
activerecord-postgis-adapter reports "PostGIS", and such an app got
plain json columns. Same fix as wallets 0.4.3; a template test pins the
column type per adapter name across all four migrations.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FKwnuouKrCTFGCidHgwSuS
@claude

claude Bot commented Oct 18, 2026

Copy link
Copy Markdown

Review

Small, focused fix, and it looks correct. /postg/i matches PostgreSQL and PostGIS, and doesn't match Mysql2, Trilogy or SQLite. It's applied consistently across both generator templates and both dummy migrations. I checked pay_charge_extension.rb: it already uses include?("postgres"), so PostGIS already takes the @> path there and needs no change.

Suggestions (minor)

  • Orphaned comment in migration_templates_test.rb: the new test sits between the existing MySQL/Trilogy comment block ("MySQL refuses a default on a JSON column…") and the MySQL test it describes. That comment now reads as part of the PostGIS block. Move the new test above that comment, or below the MySQL test.
  • Looser regex than the old check: /postg/i is a bit broader than the old check. That's fine for now, and pay_charge_extension.rb uses the same prefix idea. If you want it tighter, /postgres|postgis/i is explicit and matches the intent in the comment. This is optional.
  • Duplicated adapter detection: the adapter-name logic is copied into four migration copies plus the extension. That's inherent to migration templates, but a note in the 1.1.1 changelog or a shared test, like the one you added, is what keeps them in sync. The new test covers that well.

Tests: the test covers all four migrations across five adapter names, including the new PostGIS case, and uses stubs, so it needs no real database. Good coverage for the change.

Security/performance: none.

LGTM aside from the comment placement.

@rameerez
rameerez merged commit 0969d01 into main Oct 18, 2026
24 checks passed
@rameerez
rameerez deleted the fix/postgis-jsonb branch October 18, 2026 23:30
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