Skip to content

Commit 59d2748

Browse files
NewAlexandriamamhoff
authored andcommitted
Fix PromotionMigrator duplicate codes when promotions share base_code
Constrain the promotion code batch join in copy_promotion_codes by promotion_id and created_at to prevent cross-promotion row multiplication and duplicate value inserts. Adds a regression spec and an audit doc summarizing root cause, release verification, and related issue/PR research.
1 parent aa11fbd commit 59d2748

File tree

3 files changed

+113
-1
lines changed

3 files changed

+113
-1
lines changed
Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
# PromotionMigrator duplicate code fix
2+
3+
## Context
4+
5+
During migration from `solidus_legacy_promotions` to `solidus_promotions`, `PromotionMigrator#copy_promotion_codes`
6+
uses a SQL join from legacy code batches to new code batches.
7+
8+
In current releases (4.4.x, 4.5.x, 4.6.x, and `main` at time of writing), the relevant join condition is:
9+
10+
```sql
11+
LEFT OUTER JOIN solidus_promotions_promotion_code_batches
12+
ON solidus_promotions_promotion_code_batches.base_code = spree_promotion_code_batches.base_code
13+
```
14+
15+
## Problem
16+
17+
`base_code` is not globally unique across promotions. If multiple promotions have code batches with the same
18+
`base_code`, this join can match more than one target row for a single source row.
19+
20+
That multiplies result rows in `INSERT ... SELECT`, which can duplicate `value` insert attempts into
21+
`solidus_promotions_promotion_codes`.
22+
23+
Because `solidus_promotions_promotion_codes.value` is unique, migration can fail with:
24+
25+
```text
26+
PG::UniqueViolation: duplicate key value violates unique constraint
27+
"index_solidus_promotions_promotion_codes_on_value"
28+
DETAIL: Key (value)=(suvie-lgm4gw) already exists.
29+
```
30+
31+
## Change
32+
33+
Constrain the join to the current migrated promotion and the copied batch timestamp:
34+
35+
```sql
36+
LEFT OUTER JOIN solidus_promotions_promotion_code_batches
37+
ON solidus_promotions_promotion_code_batches.base_code = spree_promotion_code_batches.base_code
38+
AND solidus_promotions_promotion_code_batches.promotion_id = #{Integer(new_promotion.id)}
39+
AND solidus_promotions_promotion_code_batches.created_at = spree_promotion_code_batches.created_at
40+
```
41+
42+
This makes each migrated code row resolve to the corresponding batch for the promotion currently being migrated,
43+
preventing cross-promotion row multiplication.
44+
45+
## Validation
46+
47+
A regression spec covers two legacy promotions that both have `PromotionCodeBatch` records with the same
48+
`base_code`. The migration now inserts each code exactly once, without duplicate key violations.
49+
50+
## Related issues and pull requests reviewed
51+
52+
- Searched open and closed issues/PRs in `solidusio/solidus` for:
53+
- `PromotionMigrator`
54+
- `copy_promotion_codes`
55+
- `base_code`
56+
- promotion code migration duplicate-key failures
57+
- No issue or PR appears to cover this specific join condition in `PromotionMigrator#copy_promotion_codes`.
58+
- Historic issue [`#1248`](https://github.com/solidusio/solidus/issues/1248) was reviewed. It concerns an older
59+
migration path (`spree_promotions.code` to `spree_promotion_codes`) and is not the same bug as this
60+
cross-promotion batch join multiplication.

promotions/lib/solidus_promotions/promotion_migrator.rb

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,10 @@ def copy_promotion_codes(new_promotion)
6161
SELECT solidus_promotions_promotions.id AS promotion_id, solidus_promotions_promotion_code_batches.id AS promotion_code_batch_id, value, spree_promotion_codes.created_at, spree_promotion_codes.updated_at
6262
FROM spree_promotion_codes
6363
LEFT OUTER JOIN spree_promotion_code_batches ON spree_promotion_code_batches.id = spree_promotion_codes.promotion_code_batch_id
64-
LEFT OUTER JOIN solidus_promotions_promotion_code_batches ON solidus_promotions_promotion_code_batches.base_code = spree_promotion_code_batches.base_code
64+
LEFT OUTER JOIN solidus_promotions_promotion_code_batches
65+
ON solidus_promotions_promotion_code_batches.base_code = spree_promotion_code_batches.base_code
66+
AND solidus_promotions_promotion_code_batches.promotion_id = #{Integer(new_promotion.id)}
67+
AND solidus_promotions_promotion_code_batches.created_at = spree_promotion_code_batches.created_at
6568
INNER JOIN spree_promotions ON spree_promotion_codes.promotion_id = spree_promotions.id
6669
INNER JOIN solidus_promotions_promotions ON spree_promotions.id = solidus_promotions_promotions.original_promotion_id
6770
WHERE spree_promotion_codes.promotion_id = #{new_promotion.original_promotion_id};

promotions/spec/lib/solidus_promotions/promotion_migrator_spec.rb

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,55 @@
5252
end
5353
end
5454

55+
context "when multiple promotions have batches with the same base_code" do
56+
let(:shared_base_code) { "SUVIE" }
57+
let(:shared_time) { Time.current.change(usec: 0) }
58+
let!(:spree_promotion) { create(:promotion) }
59+
let!(:another_spree_promotion) { create(:promotion) }
60+
let!(:first_batch) do
61+
Spree::PromotionCodeBatch.create!(
62+
promotion: spree_promotion,
63+
base_code: shared_base_code,
64+
number_of_codes: 1,
65+
created_at: shared_time,
66+
updated_at: shared_time
67+
)
68+
end
69+
let!(:second_batch) do
70+
Spree::PromotionCodeBatch.create!(
71+
promotion: another_spree_promotion,
72+
base_code: shared_base_code,
73+
number_of_codes: 1,
74+
created_at: shared_time,
75+
updated_at: shared_time
76+
)
77+
end
78+
let!(:first_code) do
79+
create(
80+
:promotion_code,
81+
promotion: spree_promotion,
82+
promotion_code_batch: first_batch,
83+
value: "suvie-lgm4gw"
84+
)
85+
end
86+
let!(:second_code) do
87+
create(
88+
:promotion_code,
89+
promotion: another_spree_promotion,
90+
promotion_code_batch: second_batch,
91+
value: "suvie-abc123"
92+
)
93+
end
94+
95+
it "copies each code exactly once without raising a duplicate value error" do
96+
expect { subject }.not_to raise_error
97+
expect(SolidusPromotions::PromotionCode.where(value: [first_code.value, second_code.value]).count).to eq(2)
98+
expect(
99+
SolidusPromotions::PromotionCode.where(value: [first_code.value, second_code.value]).group(:value).count.values
100+
).to all(eq(1))
101+
end
102+
end
103+
55104
context "if our rules and actions are missing from the promotion map" do
56105
let(:promotion_map) do
57106
{

0 commit comments

Comments
 (0)