googleapis / googleapis/ruby-spanner-activerecord

upsert_all / insert_all! (mutation path) corrupt non-integer composite primary key columns by casting them with BigInteger

Aperta
#407 0 commenti 0 reazioni 1 assegnatario Rivendicata da @olavloite Vedi su GitHub
api: spanner priority: p2 type: bug
Lingua principale
Ruby
Stelle
105
Fork
37
Merge medio
10h 2m
PR unite (30g)
5

Descrizione

#### Environment details

- Programming language: Ruby
- OS: Linux (also reproduced on Spanner emulator and a real Spanner database)
- Language runtime version: Ruby 3.4.8
- Package version: `activerecord-spanner-adapter` 2.5.0 (also present on `main` — see `_set_composite_primary_key_value`)
- Rails: 8.0.4
- Spanner dialect: GoogleSQL

#### Steps to reproduce

1. Create a table with a **composite primary key that includes STRING columns** (minimal fictional schema below).
2. Outside a DML transaction (default mutation / `InsertOrUpdate` path), call `Model.upsert_all([row])` or `Model.insert_all!([row])` with a hash of plain Ruby values.
3. Read the row back and inspect the primary key columns.

```ruby
create_table :demo_items, id: false, force: true do |t|
t.integer :account_id, null: false
t.string :resource_uuid, null: false # UUID string
t.string :kind, null: false # e.g. "email"
t.string :value, null: false # e.g. "alice@example.com"
t.string :resource_type, null: false # non-PK column
t.datetime :created_at, null: false
end

execute <<~SQL
ALTER TABLE demo_items
ADD PRIMARY KEY (account_id, resource_uuid, kind, value)
SQL

class DemoItem < ActiveRecord::Base
self.table_name = "demo_items"
self.primary_key = %w[account_id resource_uuid kind value]
end

account_id = 4242
resource_uuid = "89ff73aa-c1cb-475e-bb27-89281c50c5b8"
created_at = Time.utc(2026, 1, 1, 12, 0, 0)

DemoItem.where(account_id: account_id).delete_all

row = {
"account_id" => account_id,
"resource_uuid" => resource_uuid,
"kind" => "email",
"value" => "alice@example.com",
"resource_type" => "house",
"created_at" => created_at,
}

# Mutation path (default when outside a DML transaction)
DemoItem.upsert_all([row])
# DemoItem.insert_all!([row]) # same corruption on the mutation path

puts DemoItem.where(account_id: account_id).map(&:attributes)
```

### Actual result

```ruby
[{
"account_id" => 4242,
"resource_uuid" => "89", # truncated via BigInteger.cast
"kind" => "0", # "email" => 0
"value" => "0", # "alice@example.com" => 0
"resource_type" => "house", # non-PK: OK
"created_at" => 2026-01-01 12:00:00 UTC, # non-PK: OK
}]
```

Lookup by the intended PK returns `nil`:

```ruby
DemoItem.find_by(
account_id: account_id,
resource_uuid: resource_uuid,
kind: "email",
value: "alice@example.com"
)
# => nil
```

### Expected result

Row persisted with the original STRING PK values (`resource_uuid`, `kind`, `value` unchanged).

## Root cause

In [`lib/activerecord_spanner_adapter/base.rb`](https://github.com/googleapis/ruby-spanner-activerecord/blob/main/lib/activerecord_spanner_adapter/base.rb), `_set_composite_primary_key_value` always defaults the type to `BigInteger` unless the caller already wrapped the value in `ActiveModel::Attribute`:

```ruby
def self._set_composite_primary_key_value primary_key, values, is_mutation
value = values[primary_key]
type = ActiveModel::Type::BigInteger.new # <-- always, for every composite PK column

if value.is_a? ActiveModel::Attribute
type = value.type
value = value.value
end
# ...
values[primary_key] =
ActiveModel::Attribute.from_database primary_key, value, type
end
```

Cast examples:

| Input | `BigInteger.cast` |
|---|---|
| `"89ff73aa-c1cb-..."` | `89` |
| `"email"` | `0` |
| `"alice@example.com"` | `0` |
| `4242` | `4242` (integer PK column survives) |

This path is used by `_buffer_record` for mutation writes (`insert` / `insert_or_update`), which is what `upsert_all` and `insert_all!` use by default when there is no open DML transaction (or when isolation is `:buffered_mutations`).

Inside a default ActiveRecord DML transaction, the same call goes through SQL DML and does **not** hit this cast — so the bug is mutation-path-specific.

`insert_all` (without bang) uses DML `INSERT OR IGNORE` and is not affected; under `:buffered_mutations` the adapter raises `NotImplementedError` for that method.

## Suggested fix

When rewriting composite PK values, use the **column's actual ActiveRecord type** (e.g. `type_for_attribute(primary_key)` / `columns_hash[primary_key].type`) instead of hard-coding `ActiveModel::Type::BigInteger`, at least when a non-nil client-provided value is present.

Integer / auto-generated PK columns would keep working; STRING (and other non-INT64) composite PK columns would stop being corrupted.

## Workaround (application-side)

Wrap each composite PK column with the real column type **before** `upsert_all` / `insert_all!` on the **mutation** path only:

```ruby
row = {
"account_id" => 4242,
"resource_uuid" => "89ff73aa-c1cb-475e-bb27-89281c50c5b8",
"kind" => "email",
"value" => "alice@example.com",
"resource_type" => "house",
"created_at" => Time.utc(2026, 1, 1, 12, 0, 0),
}

Array(DemoItem.primary_key).each do |column|
row[column] = ActiveModel::Attribute.from_database(
column,
row[column],
DemoItem.type_for_attribute(column)
)
end

DemoItem.upsert_all([row])
# => persists correct PK values on the mutation path
```

**Important:** do **not** apply this wrapping inside a DML transaction. In that path, `ActiveModel::Attribute` wrappers can be persisted as `NULL`. Only type PK columns when there is no open DML transaction (or when isolation is `:buffered_mutations`).

`stringify_keys` alone does **not** fix it; only pre-wrapping with the correct `ActiveModel::Attribute` type does.

## Notes

- Reproduced on Spanner emulator and on a real Spanner instance (so this is not an emulator-only quirk).
- Happy to contribute a failing test + patch if that would help.

Guida per i contributori

Apri la guida per i contributori

Valutazione

Questa issue non è ancora stata valutata.

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.