Repository navigation
Conversation
📝 WalkthroughWalkthroughThis change adds product options and variants with variant-backed inventory, introduces use-case and adapter boundaries, moves HTTP and PostgreSQL code into bounded packages, updates runtime composition, and adds integration and architecture tests. ChangesProduct variants and persistence
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ProductHandler
participant ProductService
participant ProductRepository
participant PostgreSQL
Client->>ProductHandler: Create variant request
ProductHandler->>ProductService: Validate detail and option IDs
ProductService->>ProductRepository: Load detail and options
ProductRepository->>PostgreSQL: Query product ownership
PostgreSQL-->>ProductRepository: Validated detail and options
ProductService->>ProductRepository: Create variant
ProductRepository->>PostgreSQL: Insert variant and option associations
PostgreSQL-->>ProductHandler: Variant result
ProductHandler-->>Client: JSON response
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@db/migrations/017_create_product_variants.up.sql`:
- Around line 162-172: The inventory helpers still assume each price has one
inventory, causing incorrect variant selection and duplicate price rows. Update
get_inventory_by_price_id, get_product_price_by_id, and
list_product_prices_by_detail to return variant-aware inventory state or a
single aggregate per price without duplicating price records; preserve the
existing output contracts where possible, and add coverage for two variant
inventories sharing one product price.
- Around line 141-143: Update the update_product_variant function to synchronize
inventories.product_price_id with p_price_id for rows whose product_variant_id
equals p_id, alongside the existing product_variants update. Add an integration
test covering an inventory-backed variant moved to a different price and verify
subsequent reads use the new price.
- Around line 47-51: Update the migration containing the inventories constraints
to avoid blocking writes: add the foreign key as NOT VALID, defer its validation
to a later step, and move unique-index creation to a separate CREATE UNIQUE
INDEX CONCURRENTLY migration. Ensure the concurrent-index step runs outside the
golang-migrate transaction, and handle SET NOT NULL in a non-blocking migration
step.
In `@plans/clean-architecture-migration.md`:
- Around line 42-44: Update the migration verification steps to require running
the Playwright E2E suite after backend verification, including the API/database
tests and available integration tests, and before merge; remove the statement
that intentionally defers E2E.
In `@src/adapter/http/announcement_handler.go`:
- Around line 89-94: Add tests in src/adapter/http/announcement_handler.go:89-94
covering request-context propagation and expected responses for create, list,
get, update, and delete operations, using the handler’s relevant methods. Add
tests in src/adapter/http/middleware.go:35-41 covering valid sessions, missing
sessions, missing members, and request-context member injection; no direct
production-code change is required at either site.
In `@src/adapter/http/gate_middleware.go`:
- Around line 11-14: Update GatewayMiddleware so an empty secret does not bypass
authentication: reject protected /api/ requests instead of returning the next
handler unauthenticated. Also add startup validation for the required gateway
secret, failing startup when it is missing while preserving normal authenticated
request handling for configured secrets.
In `@src/adapter/http/product_handler.go`:
- Around line 407-428: Update the CreatePrice, GetPrice, and UpdatePrice
handlers to resolve the route detail via routeDetail using both productId and
detailId. Use detail.ID when creating prices, and return ErrPriceNotFound
whenever a retrieved price’s ProductDetailID differs from detail.ID. Add
regression tests covering product A routes attempting to create, read, or update
product B prices.
In `@src/adapter/postgres/inventory_repo.go`:
- Around line 55-59: Update CreateInventory to detect pgx.ErrNoRows from the
create_inventory query and return domain.ErrInvalidProductVariant, while
preserving the existing wrapped persistence error for other Scan failures. Add a
repository or API test using a valid but nonexistent variant UUID to verify the
domain error.
In `@src/adapter/postgres/product_repo.go`:
- Around line 281-419: Add integration tests in product_variant_repo_test.go
covering not-found errors for option and variant get/update/delete operations,
empty results from ListOptionsByDetailID and ListVariantsByDetailID, and
verification that GetVariantByID loads OptionIDs correctly after creating or
updating a variant. Reuse the existing repository setup and assertions while
preserving current happy-path and constraint coverage.
In `@src/adapter/session/cache.go`:
- Around line 7-9: Update the test helper newTestCache in cache_test.go to
construct caches through the exported NewCache constructor instead of
NewSessionCache. Retain all existing cache behavior assertions so the new public
API is exercised without changing test coverage.
In `@src/adapter/storage/files.go`:
- Around line 8-15: Update Save to explicitly close the created file and return
any close error when io.Copy succeeds, while preserving copy errors as the
primary failure; avoid relying solely on deferred Close. Add storage tests
covering successful saves, destination/create errors, copy failures, and final
flush/close failures using the project’s existing test patterns.
In `@src/domain/model/product.go`:
- Around line 5-35: The Product.Price and ProductPrice.Amount fields in
src/domain/model/product.go:5-35 currently use float64; replace both with one
exact money representation, such as integer minor units or the project’s
established decimal type. Apply the identical representation to
InventoryItem.Cost in src/domain/model/inventory.go:18-28, preserving the
existing JSON contracts as appropriate.
In `@src/test/api/category_handler_test.go`:
- Around line 16-17: Remove the duplicate backend/src/adapter/http import in
src/test/api/category_handler_test.go:16-17,
src/test/api/event_handler_test.go:12-13,
src/test/api/inventory_handler_test.go:14-15,
src/test/api/member_handler_test.go:16-17,
src/test/api/product_variant_handler_test.go:14-15, and
src/test/api/registration_code_handler_test.go:14-15. Keep the appropriate alias
and update any domain references to use it, or change domain imports to the
actual model package if those references require model types.
In `@src/test/api/product_variant_handler_test.go`:
- Around line 83-84: Update the cross-product creation test around
handler.CreateVariant to populate CreateProductVariantRequest with the second
fixture’s ProductPriceID and OptionIDs, ensuring validation passes and ownership
is checked. Keep the request targeting the first product, then assert the
expected ownership-rejection response rather than a validation-only 400;
preserve the test’s existing fixture and assertion conventions.
In `@src/test/architecture/dependency_test.go`:
- Around line 184-185: The dependency check in the architecture test currently
searches for the literal pattern "domain.*Repository", so repository imports can
bypass it. Update the condition around the HTTP handler validation to detect the
forbidden backend/src/domain/repository import path, while preserving the
existing backend/src/api and backend/src/database checks; add or update tests
covering a repository import and allowed non-repository imports.
In `@src/usecase/member_test.go`:
- Around line 26-36: Update TestMemberUseCaseLoginAndRegister to capture the
session returned by Login, then assert it is non-nil and its member ID matches
the registered member’s ID, while preserving the existing error assertion.
In `@src/usecase/product_test.go`:
- Around line 25-36: Add a successful CreateVariant test alongside
TestProductUseCaseRejectsDuplicateOptions, using valid detail, price, and option
data, then assert fakeVariants.created contains the expected detail ID, price
ID, and option IDs. Also add a mismatched-price test that verifies CreateVariant
rejects a price belonging to a different product detail after ownership
validation is implemented.
In `@src/usecase/product.go`:
- Around line 19-38: Update ProductUseCase.CreateVariant to load priceID via
u.products.GetPriceByID after resolving detail, propagate lookup errors, and
reject prices whose ProductDetailID differs from detail.ID before validating
options or calling CreateVariant.
In `@src/usecase/services.go`:
- Around line 5-17: The service interfaces and constructors currently expose
repository methods directly, allowing handlers to bypass use-case validation.
Replace the repository-embedding contracts and constructors, including
ProductService/NewProductService and the analogous services, with explicit
application-service interfaces backed by the corresponding use-case
implementations; ensure ProductService exposes ProductUseCase.CreateVariant with
productID, priceID, and optionIDs parameters, while keeping repository
interfaces internal to the use-case layer.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3a15269d-7597-494e-bbce-455db612ea83
📒 Files selected for processing (90)
db/migrations/017_create_product_variants.up.sqldecisions/2026-08-09-clean-architecture.mddecisions/2026-08-09-product-variants-review-fix.mddecisions/2026-08-09-product-variants.mddecisions/2026-08-09-production-cutover.mddecisions/2026-08-09-runtime-cutover.mdmain.goplans/clean-architecture-migration.mdplans/product-variants.mdplans/production-runtime-cutover.mdsrc/adapter/http/announcement_handler.gosrc/adapter/http/category_handler.gosrc/adapter/http/chat_handler.gosrc/adapter/http/dto.gosrc/adapter/http/event_handler.gosrc/adapter/http/gate_middleware.gosrc/adapter/http/inventory_handler.gosrc/adapter/http/member_handler.gosrc/adapter/http/middleware.gosrc/adapter/http/product_handler.gosrc/adapter/http/product_test.gosrc/adapter/http/registration_code_handler.gosrc/adapter/http/response.gosrc/adapter/http/router.gosrc/adapter/http/session_cookie.gosrc/adapter/http/session_cookie_test.gosrc/adapter/postgres/announcement_repo.gosrc/adapter/postgres/category_repo.gosrc/adapter/postgres/chat_repo.gosrc/adapter/postgres/event_repo.gosrc/adapter/postgres/inventory_repo.gosrc/adapter/postgres/member_repo.gosrc/adapter/postgres/product_repo.gosrc/adapter/postgres/registration_code_repo.gosrc/adapter/postgres/repositories.gosrc/adapter/session/cache.gosrc/adapter/session/cache_store.gosrc/adapter/session/cache_test.gosrc/adapter/storage/files.gosrc/api/gate_middleware.gosrc/api/product_handler.gosrc/api/session_cookie.gosrc/domain/announcement.gosrc/domain/category.gosrc/domain/errors.gosrc/domain/event.gosrc/domain/inventory.gosrc/domain/member.gosrc/domain/model/announcement.gosrc/domain/model/category.gosrc/domain/model/chat.gosrc/domain/model/errors.gosrc/domain/model/event.gosrc/domain/model/inventory.gosrc/domain/model/member.gosrc/domain/model/product.gosrc/domain/product.gosrc/domain/repository.gosrc/domain/repository/event.gosrc/domain/repository/member.gosrc/domain/repository/product.gosrc/infrastructure/database.gosrc/test/api/category_handler_test.gosrc/test/api/chat_handler_test.gosrc/test/api/composition_test.gosrc/test/api/event_handler_test.gosrc/test/api/gate_middleware_test.gosrc/test/api/handler_test.gosrc/test/api/inventory_handler_test.gosrc/test/api/member_handler_test.gosrc/test/api/product_variant_handler_test.gosrc/test/api/registration_code_handler_test.gosrc/test/architecture/dependency_test.gosrc/test/database/category_repo_test.gosrc/test/database/chat_repo_test.gosrc/test/database/event_repo_test.gosrc/test/database/inventory_repo_test.gosrc/test/database/member_repo_test.gosrc/test/database/product_repo_test.gosrc/test/database/product_variant_repo_test.gosrc/test/seed/seed_test_db.gosrc/usecase/event.gosrc/usecase/event_test.gosrc/usecase/inventory.gosrc/usecase/inventory_test.gosrc/usecase/member.gosrc/usecase/member_test.gosrc/usecase/product.gosrc/usecase/product_test.gosrc/usecase/services.go
💤 Files with no reviewable changes (11)
- src/domain/inventory.go
- src/domain/announcement.go
- src/api/gate_middleware.go
- src/domain/repository.go
- src/api/product_handler.go
- src/api/session_cookie.go
- src/domain/errors.go
- src/domain/event.go
- src/domain/member.go
- src/domain/category.go
- src/domain/product.go
| ALTER TABLE inventories | ||
| ALTER COLUMN product_variant_id SET NOT NULL, | ||
| ADD CONSTRAINT inventories_product_variant_id_fkey | ||
| FOREIGN KEY (product_variant_id) REFERENCES product_variants(id) ON DELETE CASCADE; | ||
| CREATE UNIQUE INDEX idx_inventories_variant_id ON inventories(product_variant_id); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)017_create_product_variants\.up\.sql$|migration|goose|golang-migrate|db' | head -100
echo "== target migration =="
if [ -f db/migrations/017_create_product_variants.up.sql ]; then
cat -n db/migrations/017_create_product_variants.up.sql
fi
echo "== migration runner/config search =="
rg -n "database|db|goose|migrate|migration|migration-|NON_TRANSACTIONAL|non-transaction|SET NOT NULL|CREATE UNIQUE INDEX|ALTER TABLE" -S --glob '!vendor/**' --glob '!node_modules/**' . | head -250Repository: Neelein/Product_Manger_Backend
Length of output: 46196
🌐 Web query:
PostgreSQL ALTER column set not null creates lock blocks writes
💡 Result:
Yes, executing ALTER TABLE ALTER COLUMN SET NOT NULL in PostgreSQL generally creates an ACCESS EXCLUSIVE lock on the table [1][2][3]. This lock is the most restrictive level in PostgreSQL and blocks all other access, including concurrent reads (SELECT) and writes (INSERT, UPDATE, DELETE) [4][5]. By default, the command performs a full table scan to verify that no existing rows contain NULL values [1][2][3]. Because this scan must hold the ACCESS EXCLUSIVE lock to ensure data consistency, it can cause significant downtime on large tables [4][5]. You can avoid this long-held lock by using a safe, multi-step pattern that leverages PostgreSQL's ability to skip the scan if a valid CHECK constraint already proves the column contains no NULLs [1][2][3]. Recommended Zero-Downtime Pattern: 1. Add a CHECK constraint as NOT VALID. This acquires an ACCESS EXCLUSIVE lock, but it is extremely fast because it does not scan the table [6][7]. ALTER TABLE your_table ADD CONSTRAINT your_constraint_name CHECK (column_name IS NOT NULL) NOT VALID; 2. Validate the constraint. This uses a SHARE UPDATE EXCLUSIVE lock, which is non-blocking for reads and writes, allowing you to safely scan the entire table for violations [1][2][6]. ALTER TABLE your_table VALIDATE CONSTRAINT your_constraint_name; 3. Set the column as NOT NULL. Because the database now has a valid CHECK constraint proving there are no NULLs, it skips the table scan and completes almost instantly, requiring only a brief ACCESS EXCLUSIVE lock [1][2][6]. ALTER TABLE your_table ALTER COLUMN column_name SET NOT NULL; 4. Drop the redundant CHECK constraint. ALTER TABLE your_table DROP CONSTRAINT your_constraint_name; It is critical that these steps be performed separately [5]. If you attempt to combine them (e.g., in a single transaction that drops the check constraint immediately before setting the column to NOT NULL), the optimization will be lost, and the table scan will revert to the standard, blocking behavior [5].
Citations:
- 1: https://www.postgresql.org/docs/18/sql-altertable.html
- 2: https://www.postgresql.org/docs/current/sql-altertable.html
- 3: https://www.postgresql.org/docs/19/sql-altertable.html
- 4: https://tomodahinata.com/en/blog/postgresql-zero-downtime-schema-migration-lock-safe-ddl-guide
- 5: https://dev.to/andrewpsy/the-set-not-null-downtime-trap-in-postgresql-1o71
- 6: https://dev.to/mickelsamuel/adding-not-null-constraints-to-existing-postgresql-columns-safely-5fk9
- 7: https://blog.bolshakov.dev/2023/11/16/how-to-introduce-a-not-null-constraint.html
🌐 Web query:
PostgreSQL non-valid constraint foreign key validation locks table
💡 Result:
When you add a foreign key constraint to a PostgreSQL table without using the NOT VALID option, PostgreSQL performs a full table scan to verify that all existing rows satisfy the constraint [1][2]. During this process, the command acquires a SHARE ROW EXCLUSIVE lock, which blocks concurrent data modification (INSERT, UPDATE, DELETE) on both the referencing and referenced tables until the transaction commits [1][3][2]. To avoid this downtime, you can add the constraint using the NOT VALID option [4][5]. This is a fast, metadata-only operation that does not require a full table scan and, consequently, does not block concurrent writes for the duration of the validation [5][1][2]. While the constraint is in this state, it is immediately enforced for all new or updated rows, but the database does not yet guarantee that existing rows satisfy the constraint [6][1][2]. To fully validate the constraint after adding it with NOT VALID, you must execute the VALIDATE CONSTRAINT command in a separate transaction [7][5][3]. This operation performs the necessary table scan to ensure existing data is consistent, but it is specifically designed to be non-blocking for concurrent DML operations [1][2]. Validation requires only a SHARE UPDATE EXCLUSIVE lock on the table being altered (along with a ROW SHARE lock on the referenced table), which allows read and write operations to continue normally while the validation scan proceeds [1][2]. In summary, the recommended process for adding a foreign key to a large table with minimal impact is: 1. Execute ALTER TABLE <table_name> ADD CONSTRAINT <constraint_name> FOREIGN KEY (...) REFERENCES... NOT VALID; in one transaction [5][3]. 2. Execute ALTER TABLE <table_name> VALIDATE CONSTRAINT <constraint_name>; in a subsequent, separate transaction [7][5][3].
Citations:
- 1: https://www.postgresql.org/docs/current/sql-altertable.html
- 2: https://blog.hisham.ws/how-to-add-a-foreign-key-to-a-table-with-zero-downtime/
- 3: https://pg-language-server.com/latest/reference/rules/adding-foreign-key-constraint/
- 4: https://www.postgresql.org/message-id/CADoTbHVcaf7Ns4NUVoHb-cHtvMnLxVWrNTi0sqh%3D3aQTkTvgfA%40mail.gmail.com
- 5: https://runebook.dev/en/docs/postgresql/ddl-alter/id-1.5.4.9.7.2
- 6: https://www.postgresql.org/message-id/7c6d27be-915d-3db1-2e88-4e6b4e906407%40aklaver.com
- 7: https://dba.stackexchange.com/questions/343791/do-you-need-not-valid-when-adding-new-foreign-key-column
🌐 Web query:
PostgreSQL CREATE INDEX CONCURRENTLY requires non transactional golang-migrate
💡 Result:
PostgreSQL CREATE INDEX CONCURRENTLY requires that the command be executed outside an explicit transaction block. PostgreSQL documentation states: “Another difference is that a regular CREATE INDEX command can be performed within a transaction block, but CREATE INDEX CONCURRENTLY cannot.”[1] For golang-migrate (github.com/golang-migrate/migrate): 1) If your migration runner ends up executing the index creation inside a transaction (or via multi-statement execution that wraps statements in a transaction), you will hit the PostgreSQL error “CREATE INDEX CONCURRENTLY cannot run inside a transaction block”.[1][2] 2) The golang-migrate PostgreSQL driver has a multi-statement mode toggle. Its README explains that when multiple SQL statements are run in one Exec, PostgreSQL executes them inside a transaction, and that you should put CREATE INDEX CONCURRENTLY into separate migration files if you want to use it without enabling multi-statement mode.[3] 3) The golang-migrate project has also discussed this restriction: a Postgres driver running migrations in a transaction breaks CREATE INDEX CONCURRENTLY, and the workaround discussed is disabling transactions / splitting work so the concurrent index statements run outside the migration transaction context.[4] So, the practical answer to your query is: - You do not need a special “non-transactional golang-migrate binary”; you need to ensure that the migration framework does not run the CREATE INDEX CONCURRENTLY statement inside an explicit transaction. - With golang-migrate, that typically means: keep CREATE INDEX CONCURRENTLY in its own migration file and ensure your pg driver/mode is not wrapping it in a transaction (e.g., avoid multi-statement execution for that statement).[1][3] Confidence: high (PostgreSQL rule is explicit in official docs; golang-migrate’s behavior/workarounds for this exact restriction are stated in its repo docs and PR discussion).[1][2][3]
Citations:
- 1: https://www.postgresql.org/docs/18/sql-createindex.html
- 2: Support multi-statement execution for PostgreSQL golang-migrate/migrate#495
- 3: https://github.com/golang-migrate/migrate/blob/master/database/postgres/README.md
- 4: Support creating indexes concurrently in postgresql golang-migrate/migrate#137
🌐 Web query:
PostgreSQL ALTER column SET NOT NULL ACCESS EXCLUSIVE table scan existing NULL lock
💡 Result:
In PostgreSQL, executing ALTER TABLE ALTER COLUMN SET NOT NULL on an existing column acquires an ACCESS EXCLUSIVE lock on the target table [1][2]. This lock mode conflicts with all other lock modes, meaning it blocks all concurrent access to the table, including both reads and writes, for the entire duration of the command [3]. Ordinarily, this command also performs a full table scan to verify that no existing rows contain NULL values in the specified column [4][5][1]. Because the ACCESS EXCLUSIVE lock is held throughout this scan, the operation can significantly impact availability on large tables [2][6]. There are a few key nuances regarding this behavior: 1. Scan Optimization: If a valid CHECK constraint already exists on the column (and is not being dropped in the same command) that proves no NULL values can exist, PostgreSQL will skip the full table scan [4][5][1]. 2. Mitigation Strategy: To avoid the long-held ACCESS EXCLUSIVE lock during the table scan, you can use a two-step approach with a CHECK constraint [7][6]: - First, add a CHECK constraint as NOT VALID (e.g., ALTER TABLE table_name ADD CONSTRAINT constraint_name CHECK (column_name IS NOT NULL) NOT VALID;). This operation requires only a brief lock and does not scan the table [4][7][6]. - Second, run ALTER TABLE table_name VALIDATE CONSTRAINT constraint_name;. This step performs the necessary table scan to verify existing rows but only requires a SHARE UPDATE EXCLUSIVE lock, which allows concurrent reads and writes [4][7][6]. - Finally, once validated, you can execute ALTER TABLE table_name ALTER COLUMN column_name SET NOT NULL;. Because the constraint has already been validated, this operation is fast and only requires a short-duration exclusive lock [7]. Note that some newer PostgreSQL versions have introduced or are introducing improvements to how NOT NULL constraints are handled, but the standard ALTER COLUMN SET NOT NULL remains a blocking operation that requires an exclusive lock and typically a scan [4][8][9].
Citations:
- 1: https://www.postgresql.org/docs/17/sql-altertable.html
- 2: https://sql.marcus-belz.de/en/add-not-null-column-existing-table-postgres/
- 3: https://www.postgresql.org/docs/current/explicit-locking.html
- 4: https://www.postgresql.org/docs/current/sql-altertable.html
- 5: https://www.postgresql.org/docs/18/sql-altertable.html
- 6: https://pg-language-server.com/latest/reference/rules/adding-not-null-field/
- 7: https://dba.stackexchange.com/questions/267947/how-can-i-set-a-column-to-not-null-without-locking-the-table-during-a-table-scan
- 8: https://www.postgresql.org/docs/19/sql-altertable.html
- 9: https://www.dbi-services.com/blog/postgresql-18-allow-not-null-constraints-to-be-added-as-not-valid/
🌐 Web query:
PostgreSQL ADD FOREIGN KEY ALTER TABLE default lock SHARE ROW EXCLUSIVE
💡 Result:
In PostgreSQL, adding a foreign key constraint using ALTER TABLE acquires a SHARE ROW EXCLUSIVE lock [1][2][3]. This lock is held on both the table being altered and the referenced (parent) table [1][2][4]. The SHARE ROW EXCLUSIVE lock mode is restrictive: it blocks concurrent data modification commands (INSERT, UPDATE, DELETE) and other DDL operations on those tables, effectively preventing writes while allowing concurrent reads [5][6][3]. Because this command must scan the entire table to verify existing rows, it can hold this lock for a significant amount of time on large tables, causing downtime for applications that rely on writing to these tables [1][4][3]. To mitigate this impact, PostgreSQL provides the NOT VALID option [1][2]. When you use ALTER TABLE... ADD CONSTRAINT... FOREIGN KEY... NOT VALID, the command performs a much faster metadata-only operation, acquiring the lock only briefly to update the catalog [4][3]. You can then execute ALTER TABLE... VALIDATE CONSTRAINT in a separate, later transaction [1][4]. The validation step acquires a less restrictive SHARE UPDATE EXCLUSIVE lock on the table, which allows concurrent writes to continue [1][4].
Citations:
- 1: https://www.postgresql.org/docs/current/sql-altertable.html
- 2: https://www.postgresql.org/docs/19/sql-altertable.html
- 3: https://pgfence.com/blog/postgres-lock-mode-cheat-sheet/
- 4: https://postgres.ai/docs/postgres-howtos/schema-design/constraints/how-to-add-a-foreign-key
- 5: https://www.postgresql.org/docs/current/explicit-locking.html
- 6: https://www.postgresql.org/docs/18/explicit-locking.html
Avoid write-blocking migration steps.
This migration runs in golang-migrate transactions, while SET NOT NULL, the FK scan, and the unique index lock can block inventory writes. Split the migration by using a separate step for CREATE INDEX CONCURRENTLY, adding the FK as NOT VALID, and validating it later. Keep the concurrent index outside the golang-migrate transaction if the Postgres driver would otherwise run statements in one transaction.
🧰 Tools
🪛 Squawk (2.61.0)
[warning] 48-48: Setting a column NOT NULL blocks reads while the table is scanned. Make the field nullable and use a CHECK constraint instead.
(adding-not-nullable-field)
[warning] 49-50: By default new constraints require a table scan and block writes to the table while that scan occurs. Use NOT VALID with a later VALIDATE CONSTRAINT call.
(constraint-missing-not-valid)
[warning] 49-50: Adding a foreign key constraint requires a table scan and a SHARE ROW EXCLUSIVE lock on both tables, which blocks writes to each table. Add NOT VALID to the constraint in one transaction and then VALIDATE the constraint in a separate transaction.
(adding-foreign-key-constraint)
[warning] 51-51: During normal index creation, table updates are blocked, but reads are still allowed. Use concurrently to avoid blocking writes.
(require-concurrent-index-creation)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@db/migrations/017_create_product_variants.up.sql` around lines 47 - 51,
Update the migration containing the inventories constraints to avoid blocking
writes: add the foreign key as NOT VALID, defer its validation to a later step,
and move unique-index creation to a separate CREATE UNIQUE INDEX CONCURRENTLY
migration. Ensure the concurrent-index step runs outside the golang-migrate
transaction, and handle SET NOT NULL in a non-blocking migration step.
Source: Linters/SAST tools
| DELETE FROM product_variant_options WHERE variant_id = p_id; | ||
| INSERT INTO product_variant_options SELECT p_id, unnest(COALESCE(p_option_ids, ARRAY[]::UUID[])); | ||
| UPDATE product_variants AS pv SET product_price_id = p_price_id, sku = NULLIF(p_sku, ''), status = COALESCE(NULLIF(p_status, ''), 'active'), updated_at = now() WHERE pv.id = p_id RETURNING pv.updated_at INTO v_updated_at; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep the inventory price synchronized.
update_product_variant changes product_variants.product_price_id but does not update matching inventories.product_price_id. Inventory reads and price functions still use that inventory column. A variant price update then returns and joins against the old price.
Update inventories for product_variant_id = p_id in the same function. Add an integration test that moves an inventory-backed variant to another price.
Proposed fix
UPDATE product_variants AS pv SET product_price_id = p_price_id, sku = NULLIF(p_sku, ''), status = COALESCE(NULLIF(p_status, ''), 'active'), updated_at = now() WHERE pv.id = p_id RETURNING pv.updated_at INTO v_updated_at;
+ UPDATE inventories
+ SET product_price_id = p_price_id, updated_at = now()
+ WHERE product_variant_id = p_id;
RETURN QUERY SELECT v_updated_at;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@db/migrations/017_create_product_variants.up.sql` around lines 141 - 143,
Update the update_product_variant function to synchronize
inventories.product_price_id with p_price_id for rows whose product_variant_id
equals p_id, alongside the existing product_variants update. Add an integration
test covering an inventory-backed variant moved to a different price and verify
subsequent reads use the new price.
| CREATE OR REPLACE FUNCTION get_inventory_by_price_id(p_price_id UUID) | ||
| RETURNS TABLE(id UUID, product_variant_id UUID, product_price_id UUID, product_detail_id UUID, product_id UUID, name VARCHAR, status VARCHAR, total_quantity BIGINT, sold_quantity BIGINT, created_at TIMESTAMPTZ, updated_at TIMESTAMPTZ) | ||
| LANGUAGE sql AS $$ SELECT * FROM get_inventory_by_id((SELECT i.id FROM inventories i WHERE i.product_price_id=p_price_id LIMIT 1)); $$; | ||
| CREATE OR REPLACE FUNCTION list_inventories() | ||
| RETURNS TABLE(id UUID, product_variant_id UUID, product_price_id UUID, product_detail_id UUID, product_id UUID, name VARCHAR, status VARCHAR, total_quantity BIGINT, sold_quantity BIGINT, created_at TIMESTAMPTZ, updated_at TIMESTAMPTZ) | ||
| LANGUAGE sql AS $$ SELECT x.* FROM inventories i CROSS JOIN LATERAL get_inventory_by_id(i.id) x ORDER BY x.created_at DESC; $$; | ||
|
|
||
| CREATE OR REPLACE FUNCTION get_product_price_by_id(p_id UUID) | ||
| RETURNS TABLE(id UUID, product_detail_id UUID, label VARCHAR, amount NUMERIC, currency VARCHAR, sort_order INT, inventory_id UUID, created_at TIMESTAMPTZ, updated_at TIMESTAMPTZ) LANGUAGE sql AS $$ SELECT pp.id, pp.product_detail_id, pp.label, pp.amount, pp.currency, pp.sort_order, i.id, pp.created_at, pp.updated_at FROM product_prices pp LEFT JOIN inventories i ON i.product_price_id=pp.id WHERE pp.id=p_id; $$; | ||
| CREATE OR REPLACE FUNCTION list_product_prices_by_detail(p_detail_id UUID) | ||
| RETURNS TABLE(id UUID, product_detail_id UUID, label VARCHAR, amount NUMERIC, currency VARCHAR, sort_order INT, inventory_id UUID, created_at TIMESTAMPTZ, updated_at TIMESTAMPTZ) LANGUAGE sql AS $$ SELECT pp.id, pp.product_detail_id, pp.label, pp.amount, pp.currency, pp.sort_order, i.id, pp.created_at, pp.updated_at FROM product_prices pp LEFT JOIN inventories i ON i.product_price_id=pp.id WHERE pp.product_detail_id=p_detail_id ORDER BY pp.sort_order; $$; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 'get_product_price_by_id|list_product_prices_by_detail|get_inventory_by_price_id|InventoryID|ProductVariantID' \
src/adapter/postgres src/domain src/usecaseRepository: Neelein/Product_Manger_Backend
Length of output: 12527
🏁 Script executed:
#!/bin/bash
set -euo pipefail
wc -l db/migrations/017_create_product_variants.up.sql src/adapter/postgres/product_repo.go src/adapter/postgres/inventory_repo.go
printf '\n--- migration relevant schema/functions ---\n'
sed -n '1,240p' db/migrations/017_create_product_variants.up.sql
printf '\n--- product_repo relevant methods ---\n'
sed -n '160,235p' src/adapter/postgres/product_repo.go
printf '\n--- inventory_repo relevant Get methods ---\n'
sed -n '1,140p' src/adapter/postgres/inventory_repo.go
printf '\n--- calls to GetInventoryByPriceID and GetPricesByDetailID ---\n'
rg -n -C 4 'GetInventoryByPriceID|GetInventoryByVariantID|GetPricesByDetailID|ListPricesByProductDetailID|InventoryID|ProductVariantID|ProductPriceID' src/adapter/postgres src/domain src/usecase src/handler src/routerRepository: Neelein/Product_Manger_Backend
Length of output: 36736
Remove the one-inventory-per-price assumption.
inventories now has one row per variant, so get_inventory_by_price_id can return the wrong variant when one price has two inventories. get_product_price_by_id and list_product_prices_by_detail also return the same price multiple times, each with a different inventory_id. Return inventory state by variant, or expose an aggregate of variant inventories for price-level calls. Add a test or E2E case for two variant inventories sharing one price.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@db/migrations/017_create_product_variants.up.sql` around lines 162 - 172, The
inventory helpers still assume each price has one inventory, causing incorrect
variant selection and duplicate price rows. Update get_inventory_by_price_id,
get_product_price_by_id, and list_product_prices_by_detail to return
variant-aware inventory state or a single aggregate per price without
duplicating price records; preserve the existing output contracts where
possible, and add coverage for two variant inventories sharing one product
price.
Source: Coding guidelines
| - Add use case unit tests and dependency architecture tests. | ||
| - Run API/database tests against the HTTP, PostgreSQL, session, model, and repository packages directly. | ||
| - Run `gofmt`, `go test ./...`, and the available database integration tests. E2E is intentionally deferred until backend verification is complete. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Require Playwright E2E before merge.
Line 44 defers E2E but provides no required execution point. Run the Playwright suite after backend verification and before merge.
As per coding guidelines, “Backend changes that alter user-facing behavior … must pass the Playwright E2E test suite before merge.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plans/clean-architecture-migration.md` around lines 42 - 44, Update the
migration verification steps to require running the Playwright E2E suite after
backend verification, including the API/database tests and available integration
tests, and before merge; remove the statement that intentionally defers E2E.
Source: Coding guidelines
| if err := h.repo.Create(r.Context(), &announcement); err != nil { | ||
| writeError(w, http.StatusInternalServerError, err.Error()) | ||
| return | ||
| } | ||
|
|
||
| writeJSON(w, http.StatusCreated, domain.AnnouncementResponse{Announcement: announcement}) | ||
| writeJSON(w, http.StatusCreated, AnnouncementResponse{Announcement: announcement}) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add tests for the migrated adapter-to-service contracts.
These changes alter request-context propagation and service dependencies. No corresponding announcement-handler or authentication-middleware tests are included in the reviewed changes.
src/adapter/http/announcement_handler.go#L89-L94: add tests that verify request-context propagation and responses for create, list, get, update, and delete operations.src/adapter/http/middleware.go#L35-L41: add tests for valid sessions, missing sessions, missing members, and request-context member injection.
As per coding guidelines, “Every code change must include corresponding tests; no code may be merged without tests.”
📍 Affects 2 files
src/adapter/http/announcement_handler.go#L89-L94(this comment)src/adapter/http/middleware.go#L35-L41
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/adapter/http/announcement_handler.go` around lines 89 - 94, Add tests in
src/adapter/http/announcement_handler.go:89-94 covering request-context
propagation and expected responses for create, list, get, update, and delete
operations, using the handler’s relevant methods. Add tests in
src/adapter/http/middleware.go:35-41 covering valid sessions, missing sessions,
missing members, and request-context member injection; no direct production-code
change is required at either site.
Source: Coding guidelines
| if strings.Contains(text, "domain.*Repository") || strings.Contains(text, "backend/src/api") || strings.Contains(text, "backend/src/database") { | ||
| t.Fatalf("HTTP handler %s depends on a forbidden adapter", file) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix the repository dependency check.
strings.Contains treats "domain.*Repository" as literal text. It cannot match domain.ProductRepository or another repository type. An HTTP handler can import backend/src/domain/repository and pass this test.
Check the forbidden import path instead.
Proposed fix
- if strings.Contains(text, "domain.*Repository") || strings.Contains(text, "backend/src/api") || strings.Contains(text, "backend/src/database") {
+ if strings.Contains(text, `"backend/src/domain/repository"`) || strings.Contains(text, "backend/src/api") || strings.Contains(text, "backend/src/database") {As per coding guidelines, “Every code change must include corresponding tests; no code may be merged without tests.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if strings.Contains(text, "domain.*Repository") || strings.Contains(text, "backend/src/api") || strings.Contains(text, "backend/src/database") { | |
| t.Fatalf("HTTP handler %s depends on a forbidden adapter", file) | |
| if strings.Contains(text, `"backend/src/domain/repository"`) || strings.Contains(text, "backend/src/api") || strings.Contains(text, "backend/src/database") { | |
| t.Fatalf("HTTP handler %s depends on a forbidden adapter", file) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/test/architecture/dependency_test.go` around lines 184 - 185, The
dependency check in the architecture test currently searches for the literal
pattern "domain.*Repository", so repository imports can bypass it. Update the
condition around the HTTP handler validation to detect the forbidden
backend/src/domain/repository import path, while preserving the existing
backend/src/api and backend/src/database checks; add or update tests covering a
repository import and allowed non-repository imports.
Source: Coding guidelines
| func TestMemberUseCaseLoginAndRegister(t *testing.T) { | ||
| f := &fakeMembers{} | ||
| u := NewMemberUseCase(f, &fakeSessions{}) | ||
| member := &model.Member{Email: "a@example.com", Name: "A"} | ||
| if err := u.Register(context.Background(), member, "secret", func(_ context.Context, m *model.Member) error { f.member = m; return nil }); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if _, _, err := u.Login(context.Background(), member.Email, "secret"); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the created session.
Line 33 discards the session returned by Login. Capture it and assert that it is non-nil and has the registered member ID. This verifies session creation and the login return contract.
As per coding guidelines, “Every code change must include corresponding tests; no code may be merged without tests.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/usecase/member_test.go` around lines 26 - 36, Update
TestMemberUseCaseLoginAndRegister to capture the session returned by Login, then
assert it is non-nil and its member ID matches the registered member’s ID, while
preserving the existing error assertion.
Source: Coding guidelines
| func TestProductUseCaseRejectsDuplicateOptions(t *testing.T) { | ||
| f := &fakeVariants{detail: &model.ProductDetail{ID: "d"}, options: map[string]*model.ProductOption{"o": {ID: "o", ProductDetailID: "d"}}} | ||
| if err := NewProductUseCase(nil, f).CreateVariant(context.Background(), "p", "price", []string{"o", "o"}); err != model.ErrDuplicateProductVariant { | ||
| t.Fatalf("err = %v", err) | ||
| } | ||
| } | ||
| func TestProductUseCaseRejectsForeignOption(t *testing.T) { | ||
| f := &fakeVariants{detail: &model.ProductDetail{ID: "d"}, options: map[string]*model.ProductOption{"o": {ID: "o", ProductDetailID: "other"}}} | ||
| if err := NewProductUseCase(nil, f).CreateVariant(context.Background(), "p", "price", []string{"o"}); err == nil { | ||
| t.Fatal("expected ownership error") | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add a successful variant creation test.
These tests only verify rejected inputs. Add a test that calls CreateVariant with valid inputs and asserts that fakeVariants.created has the expected detail ID, price ID, and option IDs. Add a mismatched-price test after adding the ownership validation.
As per coding guidelines, “Every code change must include corresponding tests; no code may be merged without tests.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/usecase/product_test.go` around lines 25 - 36, Add a successful
CreateVariant test alongside TestProductUseCaseRejectsDuplicateOptions, using
valid detail, price, and option data, then assert fakeVariants.created contains
the expected detail ID, price ID, and option IDs. Also add a mismatched-price
test that verifies CreateVariant rejects a price belonging to a different
product detail after ownership validation is implemented.
Source: Coding guidelines
| func (u *ProductUseCase) CreateVariant(ctx context.Context, productID, priceID string, optionIDs []string) error { | ||
| detail, err := u.variants.GetDetailByProductID(ctx, productID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| seen := make(map[string]struct{}, len(optionIDs)) | ||
| for _, id := range optionIDs { | ||
| if _, ok := seen[id]; ok { | ||
| return model.ErrDuplicateProductVariant | ||
| } | ||
| seen[id] = struct{}{} | ||
| o, err := u.variants.GetOptionByID(ctx, id) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if o.ProductDetailID != detail.ID { | ||
| return fmt.Errorf("option %s does not belong to product detail", id) | ||
| } | ||
| } | ||
| return u.variants.CreateVariant(ctx, &model.ProductVariant{ProductDetailID: detail.ID, ProductPriceID: priceID, OptionIDs: optionIDs}) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate that priceID belongs to the requested product.
Line 20 validates the product detail, but Lines 24-38 never resolve priceID. A caller can create a variant for one product detail with a price from another product detail because the database schema has separate foreign keys, not a paired ownership constraint.
Load the price through u.products.GetPriceByID. Reject the request when price.ProductDetailID != detail.ID. Add this validation before persistence.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/usecase/product.go` around lines 19 - 38, Update
ProductUseCase.CreateVariant to load priceID via u.products.GetPriceByID after
resolving detail, propagate lookup errors, and reject prices whose
ProductDetailID differs from detail.ID before validating options or calling
CreateVariant.
| type ProductService interface{ repository.Product } | ||
| type InventoryService interface{ repository.Inventory } | ||
| type MemberService interface{ repository.Member } | ||
| type SessionService interface{ repository.Session } | ||
| type RegistrationCodeService interface{ repository.RegistrationCode } | ||
| type CategoryService interface{ repository.Category } | ||
| type AnnouncementService interface{ repository.Announcement } | ||
| type ChatService interface{ repository.ChatRoom } | ||
| type EventService interface{ repository.EventOperations } | ||
|
|
||
| type productService struct{ repository.Product } | ||
|
|
||
| func NewProductService(repo repository.Product) ProductService { return &productService{repo} } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not expose repository methods as application services.
Lines 5-55 only wrap repository interfaces. ProductService exposes the raw CreateVariant(context.Context, *model.ProductVariant) method. It cannot expose ProductUseCase.CreateVariant(context.Context, productID, priceID string, optionIDs []string). HTTP handlers can therefore bypass duplicate-option and ownership validation.
Define explicit application-service methods. Construct ProductService from ProductUseCase. Keep repository interfaces internal to the use case layer.
Also applies to: 19-55
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/usecase/services.go` around lines 5 - 17, The service interfaces and
constructors currently expose repository methods directly, allowing handlers to
bypass use-case validation. Replace the repository-embedding contracts and
constructors, including ProductService/NewProductService and the analogous
services, with explicit application-service interfaces backed by the
corresponding use-case implementations; ensure ProductService exposes
ProductUseCase.CreateVariant with productID, priceID, and optionIDs parameters,
while keeping repository interfaces internal to the use-case layer.
1 participant
Summary by CodeRabbit
New Features
Architecture
Bug Fixes
Tests