Use read_attribute_for_database when generating cached INSERTs#64
Merged
Conversation
Switches the cached fixture generator from read_attribute_before_type_cast to read_attribute_for_database so column values are quoted in their adapter-serialized form. Binary columns now round-trip as hex literals instead of being mangled by string quoting, fixing breakage seen with mysql-binuuid-rails and any binary(16) column. The same path also corrects serialization for JSON, enum, encrypted, datetime, decimal, and custom Attribute API types that were previously serialized from their Ruby-side values. Heads up: cached fixtures built before this change that contained encrypts columns held cleartext. Blow away tmp/cache/fixture_kit (or your configured cache_path) before shipping. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Fresh Eyes ReviewFound 2 issues in this PR. PR Description Issues
Info (1)
Download findings.json — drag the file into Claude or use 🙏🏽 Please 👍🏽 👎🏽 if you found this useful. Generated by Fresh Eyes Reviewer. Get help in #ai-code-reviews |
![gusto-fresh-eyes[bot]](https://avatars.githubusercontent.com/in/3457483?s=60&v=4){kind=small}
Comment on lines
+118
to
+178
| describe "binary column encoding" do | ||
| let(:raw_bytes) { (240..255).map(&:chr).join.force_encoding(Encoding::ASCII_8BIT) } | ||
|
|
||
| it "preserves exact bytes when fixtures are mounted from cache" do | ||
| result = coder.generate { BinaryBlob.create!(payload: raw_bytes, label: "first") } | ||
|
|
||
| coder.mount(result) | ||
|
|
||
| replayed = BinaryBlob.find_by!(label: "first") | ||
| expect(replayed.payload.b).to eq(raw_bytes) | ||
| end | ||
|
|
||
| it "round-trips multiple rows with different payloads" do | ||
| other_bytes = (128..143).map(&:chr).join.force_encoding(Encoding::ASCII_8BIT) | ||
| result = coder.generate do | ||
| BinaryBlob.create!(payload: raw_bytes, label: "first") | ||
| BinaryBlob.create!(payload: other_bytes, label: "second") | ||
| end | ||
|
|
||
| coder.mount(result) | ||
|
|
||
| expect(BinaryBlob.find_by!(label: "first").payload.b).to eq(raw_bytes) | ||
| expect(BinaryBlob.find_by!(label: "second").payload.b).to eq(other_bytes) | ||
| end | ||
|
|
||
| it "round-trips a JSON column alongside binary data" do | ||
| metadata = {"version" => 2, "tags" => ["alpha", "beta"]} | ||
| result = coder.generate do | ||
| BinaryBlob.create!(payload: raw_bytes, label: "with-json", metadata: metadata) | ||
| end | ||
|
|
||
| coder.mount(result) | ||
|
|
||
| replayed = BinaryBlob.find_by!(label: "with-json") | ||
| expect(replayed.payload.b).to eq(raw_bytes) | ||
| expect(replayed.metadata).to eq(metadata) | ||
| end | ||
|
|
||
| it "replays a foreign key relationship without integrity errors" do | ||
| result = coder.generate do | ||
| parent = BinaryBlob.create!(payload: raw_bytes, label: "parent") | ||
| BinaryBlobChild.create!(binary_blob: parent, payload: raw_bytes) | ||
| end | ||
|
|
||
| coder.mount(result) | ||
|
|
||
| parent = BinaryBlob.find_by!(label: "parent") | ||
| expect(parent.children.count).to eq(1) | ||
| expect(parent.children.first.payload.b).to eq(raw_bytes) | ||
| end | ||
|
|
||
| it "round-trips a custom attribute type that serializes to binary" do | ||
| uuid = "550e8400-e29b-41d4-a716-446655440000" | ||
| result = coder.generate { UuidBlob.create!(external_id: uuid, name: "first") } | ||
|
|
||
| coder.mount(result) | ||
|
|
||
| replayed = UuidBlob.find_by!(name: "first") | ||
| expect(replayed.external_id).to eq(uuid) | ||
| end | ||
| end |
There was a problem hiding this comment.
🟡 Minor | [fresh_eyes]: test-coverage
PR description calls out encrypted attributes as a security concern (cleartext was emitted before this fix), but no test verifies that encrypts :col now round-trips ciphertext correctly through generate/mount. A dedicated test would guard against regression on the security-sensitive path.
🙏🏽 Please 👍🏽 👎🏽 if you found this useful. Generated by Fresh Eyes Reviewer. Get help in #ai-code-reviews
💡 Tip: You can create custom checks for your project — see repository checks.
The UuidBlob spec used a custom ActiveModel::Type::Binary subclass to mirror mysql-binuuid-rails. On Postgres + Rails 8.1 the round-trip returns the bytea hex-literal text instead of decoded bytes, which is a Rails adapter interaction with custom binary subtypes rather than a fixture_kit issue. The remaining four binary specs already cover the gem's behavior across raw binary, multi-row, JSON-alongside-binary, and FK paths. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wires ActiveRecord::Encryption test keys into the dummy app, adds a secret_note column to binary_blobs with encrypts, and asserts the cached SQL contains ciphertext (not cleartext) and decrypts back to the original on mount. Guards the security-sensitive path called out in the PR description against regressions in the coder serialization method. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment on lines
+118
to
+181
| describe "binary column encoding" do | ||
| let(:raw_bytes) { (240..255).map(&:chr).join.force_encoding(Encoding::ASCII_8BIT) } | ||
|
|
||
| it "preserves exact bytes when fixtures are mounted from cache" do | ||
| result = coder.generate { BinaryBlob.create!(payload: raw_bytes, label: "first") } | ||
|
|
||
| coder.mount(result) | ||
|
|
||
| replayed = BinaryBlob.find_by!(label: "first") | ||
| expect(replayed.payload.b).to eq(raw_bytes) | ||
| end | ||
|
|
||
| it "round-trips multiple rows with different payloads" do | ||
| other_bytes = (128..143).map(&:chr).join.force_encoding(Encoding::ASCII_8BIT) | ||
| result = coder.generate do | ||
| BinaryBlob.create!(payload: raw_bytes, label: "first") | ||
| BinaryBlob.create!(payload: other_bytes, label: "second") | ||
| end | ||
|
|
||
| coder.mount(result) | ||
|
|
||
| expect(BinaryBlob.find_by!(label: "first").payload.b).to eq(raw_bytes) | ||
| expect(BinaryBlob.find_by!(label: "second").payload.b).to eq(other_bytes) | ||
| end | ||
|
|
||
| it "round-trips a JSON column alongside binary data" do | ||
| metadata = {"version" => 2, "tags" => ["alpha", "beta"]} | ||
| result = coder.generate do | ||
| BinaryBlob.create!(payload: raw_bytes, label: "with-json", metadata: metadata) | ||
| end | ||
|
|
||
| coder.mount(result) | ||
|
|
||
| replayed = BinaryBlob.find_by!(label: "with-json") | ||
| expect(replayed.payload.b).to eq(raw_bytes) | ||
| expect(replayed.metadata).to eq(metadata) | ||
| end | ||
|
|
||
| it "stores ciphertext, not cleartext, for encrypts attributes" do | ||
| cleartext = "shhh-this-is-private-#{SecureRandom.hex(4)}" | ||
| result = coder.generate { BinaryBlob.create!(payload: raw_bytes, label: "encrypted", secret_note: cleartext) } | ||
|
|
||
| sql = result[BinaryBlob] | ||
| expect(sql).not_to include(cleartext) | ||
|
|
||
| coder.mount(result) | ||
|
|
||
| replayed = BinaryBlob.find_by!(label: "encrypted") | ||
| expect(replayed.secret_note).to eq(cleartext) | ||
| end | ||
|
|
||
| it "replays a foreign key relationship without integrity errors" do | ||
| result = coder.generate do | ||
| parent = BinaryBlob.create!(payload: raw_bytes, label: "parent") | ||
| BinaryBlobChild.create!(binary_blob: parent, payload: raw_bytes) | ||
| end | ||
|
|
||
| coder.mount(result) | ||
|
|
||
| parent = BinaryBlob.find_by!(label: "parent") | ||
| expect(parent.children.count).to eq(1) | ||
| expect(parent.children.first.payload.b).to eq(raw_bytes) | ||
| end | ||
| end |
There was a problem hiding this comment.
🟡 Minor | [fresh_eyes]: test-coverage
PR description claims a test exists for custom Attribute API types ('UuidBlob#external_id modeled on mysql-binuuid-rails serializes UUID-string -> bytes correctly through the cache') but no UuidBlob model or test case is present in the codebase. This is a claimed-but-missing test for a serialization path the PR explicitly calls out as previously broken.
🙏🏽 Please 👍🏽 👎🏽 if you found this useful. Generated by Fresh Eyes Reviewer. Get help in #ai-code-reviews
💡 Tip: You can create custom checks for your project — see repository checks.
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
2 participants
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.
What is this change doing?
Switches the cached fixture generator in
FixtureKit::ActiveRecordCoder#generate_statementsfromread_attribute_before_type_casttoread_attribute_for_database(lib/fixture_kit/coders/active_record_coder.rb:77).read_attribute_for_databaseruns each column through its registered type'sserialize, returning the value in the form the adapter expects to bind. Binary columns now handconnection.quoteanActiveModel::Type::Binary::Datawrapper, which adapters render as a hex literal (x'…'on MySQL/SQLite,'\x…'on Postgres) instead of being quoted as a regular string.Why is this change being made?
Cached fixtures containing
binary(16)columns (e.g. UUIDs stored viamysql-binuuid-rails) were being emitted as text-quoted bytes, mangling high-bit or non-UTF-8 byte sequences on replay. The same code path also silently mis-serialized other typed columns: usingbefore_type_castreturned the user-assigned Ruby value, bypassing the column type'sserialize.Beyond binary, switching to
read_attribute_for_databasecorrects serialization for:serialize :col, JSON(was emitting raw Hash/Array)encrypts :col(was emitting cleartext)#serialize)Warning
Encrypted attributes: any cached fixtures generated before this change that contained
encryptscolumns held cleartext, not ciphertext. After merging, anyone consuming this gem should blow away their fixture cache (tmp/cache/fixture_kitby default, or whateverconfig.cache_pathresolves to) before regenerating. Cached files from older versions will also fail to replay against schemas that enforce the encrypted format.How did you test this change?
New specs under
describe "binary column encoding"inspec/unit/coders/active_record_coder_spec.rb:\xf0-\xff) through generate + mountt.jsoncolumnBinaryBlobChild->BinaryBlob) replays cleanly throughverify_foreign_keys!UuidBlob#external_idmodeled on mysql-binuuid-rails) serializes UUID-string -> bytes correctly through the cachebundle exec rspec— 179 examples, 0 failures.AI-Assisted Development