Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds JSON column support for PostgreSQL (using JSONB) and MySQL (using JSON) databases. The implementation automatically serializes Ruby Arrays and Hashes to JSON strings during bulk insertion, enabling seamless storage and retrieval of structured data in JSON columns.
Changes:
- Modified the
BulkInserter#quotemethod to detect and serialize Array/Hash values to JSON - Updated test database schemas to include
tags(array) andmetadata(hash) JSON columns - Added comprehensive tests for JSON column handling in PostgreSQL and MySQL contexts
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lib/staging_table/bulk_inserter.rb | Enhanced the quote method to automatically serialize Array and Hash values to JSON strings before database insertion |
| spec/support/database_helper.rb | Added JSON column definitions to test tables, using JSONB for PostgreSQL, JSON for MySQL, and TEXT for SQLite |
| spec/staging_table/bulk_inserter_spec.rb | Added test coverage for JSON array and hash handling, correctly scoped to PostgreSQL and MySQL (SQLite excluded due to TEXT column limitations) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| connection.quote(value) | ||
| case value | ||
| when Array, Hash | ||
| connection.quote(value.to_json) |
There was a problem hiding this comment.
The to_json method is being used without explicitly requiring the JSON library. While ActiveSupport provides to_json for Array and Hash, it would be more robust to add require "json" at the top of this file to ensure the method is available, especially if this library is used in contexts where ActiveSupport might not load all core extensions.
spec/support/database_helper.rb
Outdated
| DROP TABLE IF EXISTS test_users | ||
| SQL | ||
|
|
||
| json_type = ActiveRecord::Base.connection.adapter_name.downcase.include?("postgresql") ? "JSONB" : "JSON" |
There was a problem hiding this comment.
The adapter detection uses include?("postgresql") which is inconsistent with the pattern matching approach used in lib/staging_table/adapters/base.rb:24 where regex patterns like /postgresql/ are used. Consider using the same pattern matching approach for consistency: adapter_name.downcase =~ /postgresql/ instead of include?("postgresql").
| it "handles array values for JSON/JSONB columns" do | ||
| records = [ | ||
| {name: "John", email: "john@example.com", tags: %w[admin user]} | ||
| ] | ||
|
|
||
| inserter.insert(records) | ||
|
|
||
| user = staging_model.first | ||
| expect(user.tags).to eq(%w[admin user]) | ||
| end | ||
|
|
||
| it "handles hash values for JSON/JSONB columns" do | ||
| records = [ | ||
| {name: "John", email: "john@example.com", metadata: {role: "admin", level: 5}} | ||
| ] | ||
|
|
||
| inserter.insert(records) | ||
|
|
||
| user = staging_model.first | ||
| expect(user.metadata).to eq({"role" => "admin", "level" => 5}) | ||
| end | ||
| end |
There was a problem hiding this comment.
Consider adding test cases for edge cases with JSON columns, such as: empty arrays ([]), empty hashes ({}), nested structures, and null values for JSON columns. These would help ensure the JSON serialization handles all scenarios correctly.
sncalvo
force-pushed
the
feat/add-json-support
branch
from
0187735 to
aea4e2d
Compare
sncalvo
force-pushed
the
feat/add-json-support
branch
from
aea4e2d to
1d97d0f
Compare
json support was added to psql and mysql
sncalvo
force-pushed
the
feat/add-json-support
branch
from
1d97d0f to
1fafa32
Compare
1 participant
json support was added to psql and mysql