Fix store_accessor on MariaDB by declaring the JSON attribute type - #3038
Open
a-abdellatif98 wants to merge 1 commit into
Open
Fix store_accessor on MariaDB by declaring the JSON attribute type#3038a-abdellatif98 wants to merge 1 commit into
a-abdellatif98 wants to merge 1 commit into
Conversation
MariaDB implements JSON as an alias for LONGTEXT with a CHECK (json_valid(...)) constraint, so ActiveRecord introspects filters.fields and events.particulars as Type::Text. store_accessor rejects that type, and Filter.from_params raised ActiveRecord::ConfigurationError on every request. Declaring the attribute as :json overrides the introspected type on MariaDB and is a no-op on MySQL and SQLite, where the column already resolves to Type::Json. Unlike `store ..., coder: JSON` it adds no Type::Serialized layer and keeps StringKeyedHashAccessor, so the existing string-keyed hash semantics are unchanged. These are the only two JSON columns in the schema. The remaining serialized columns (webhooks.subscribed_actions, webhook_deliveries.request/response, passkeys.transports) are text columns with explicit coders and resolve to Type::Serialized on every adapter, so they are unaffected. Fixes basecamp#2402
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR ensures store_accessor works correctly with MariaDB JSON columns by explicitly declaring affected columns as :json, and adds regression tests to verify type casting and persistence.
Changes:
- Declare
Filter#fieldsandEvent#particularsas:jsonattributes to supportstore_accessorwhen MariaDB reports JSON asLONGTEXT. - Add tests asserting JSON typing and confirming store accessor values round-trip through the DB.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| test/models/filter_test.rb | Adds regression tests for fields JSON typing and store_accessor persistence. |
| test/models/event_test.rb | Adds regression tests for particulars JSON typing and store_accessor persistence. |
| app/models/filter/fields.rb | Declares fields as :json to make store_accessor work with MariaDB. |
| app/models/event/particulars.rb | Declares particulars as :json to make store_accessor work with MariaDB. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| end | ||
|
|
||
| test "fields is a JSON attribute however the adapter reports the column" do | ||
| assert_instance_of ActiveRecord::Type::Json, Filter.type_for_attribute(:fields) |
| end | ||
|
|
||
| test "particulars is a JSON attribute however the adapter reports the column" do | ||
| assert_instance_of ActiveRecord::Type::Json, Event.type_for_attribute(:particulars) |
Comment on lines
+33
to
+35
| # MariaDB reports JSON columns as LONGTEXT, so declare the type store_accessor needs. | ||
| attribute :fields, :json | ||
|
|
Comment on lines
+5
to
+6
| # MariaDB reports JSON columns as LONGTEXT, so declare the type store_accessor needs. | ||
| attribute :particulars, :json |
| assert_not_includes filter.board_titles, "Private board" | ||
| end | ||
|
|
||
| test "fields is a JSON attribute however the adapter reports the column" do |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2402.
MariaDB implements
JSONas an alias forLONGTEXTwith aCHECK (json_valid(...))constraint, so ActiveRecord introspectsfilters.fieldsandevents.particularsasType::Text.store_accessorrejects that type andFilter.from_paramsraisesActiveRecord::ConfigurationErroron every request.Verified against MariaDB 11.8.8, where the column comes back as:
Declaring
attribute :fields, :jsonoverrides the introspected type on MariaDB and is a no-op on MySQL and SQLite, where the column already resolves toType::Json. Unlikestore ..., coder: JSONit adds noType::Serializedlayer and keepsStringKeyedHashAccessor, so the existing string-keyed hash semantics are unchanged.These are the only two JSON columns in the schema. The other serialized columns (
webhooks.subscribed_actions,webhook_deliveries.request/response,passkeys.transports) are text columns with explicit coders and resolve toType::Serializedon every adapter, so they are unaffected.Why this does not also quote the fixtures
#2769 fixes the same bug with the same
attributedeclaration, but it additionally quotes the JSON fixture values. That part is a silent data-loss regression on MySQL and SQLite, so this PR leaves fixtures untouched.build_fixture_sqlserializes through the database column type, not the model's declared attribute type, soattribute :fields, :jsonnever reaches fixtures at all:The double-encoded value deserializes back to a
Stringrather than aHash, so the store accessors find no keys and the data quietly disappears. On SQLite with #2769's quoting applied,bin/rails test test/models/event/description_test.rbgives 5 failures of the form:Reverting only the quoting: 18 tests, 62 assertions, 0 failures.
Neither a YAML String nor a YAML Hash is correct on all three adapters, so there is no stock-Rails fixture value that works everywhere. That means this PR unblocks MariaDB at runtime but does not make the test suite runnable on MariaDB, and it deliberately does not add MariaDB to CI. Closing that gap needs either an upstream Rails change or an adapter-level patch mapping MariaDB's
json_validlongtext columns toType::Json, which seemed out of scope here.Testing
Added four tests: a type assertion per model that fails on MariaDB before this change, and a create/reload round trip through the store accessors.
Not verified:
bin/rails test:systemon any adapter, and the full suite on MariaDB, which is blocked by the pre-existing fixture issue described above rather than by this change.