Skip to content

Adds get flows by product#65

Open
jlpdeveloper wants to merge 11 commits into
mainfrom
flows/get-by-product
Open

Adds get flows by product#65
jlpdeveloper wants to merge 11 commits into
mainfrom
flows/get-by-product

Conversation

@jlpdeveloper
Copy link
Copy Markdown
Contributor

@jlpdeveloper jlpdeveloper commented May 17, 2026

Description

Code Rabbit Summary

Summary by CodeRabbit

  • New Features

    • Added API endpoint to list flows for a given product and a corresponding HTTP flow definition for testing.
  • Improvements

    • Consistent JSON response helper used across endpoints; improved HTTP status handling (400, 404, 500).
    • Router updated to register the new product-flows route.
  • Tests

    • Added unit and handler tests covering listing flows by product, response encoding, and not-found behavior.

Review Change Stack

Fixes

Closes #36

Post Deployment Tasks?

- Implement `WriteJSONResponse` utility to encode data to JSON and write HTTP responses with improved reliability.
- Add unit tests to cover success, error, and content-type validation scenarios.
- Define HTTP GET request to fetch flows for a specific product using its ID.
- Include request structure, example response, and response validation.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 17, 2026

Warning

Rate limit exceeded

@jlpdeveloper has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 47 minutes and 11 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: ad16430f-0ace-4478-a40c-d588cd19ce7c

📥 Commits

Reviewing files that changed from the base of the PR and between c1088a8 and dbb6786.

📒 Files selected for processing (10)
  • internal/flow/db/db.gen.go
  • internal/flow/db/flows.sql.gen.go
  • internal/flow/db/models.gen.go
  • internal/flow/db/querier.gen.go
  • internal/flow/handler.go
  • internal/flow/handler_test.go
  • internal/flow/request.go
  • internal/flow/service.go
  • internal/flow/service_test.go
  • sqlc.yml
📝 Walkthrough

Walkthrough

Adds GET /api/products/{id}/flows: introduces WriteJSONResponse and NotFoundError, refactors handlers to use the helper, adds a flow service abstraction, updates SQL/gen code and the Querier, wires the router, and adds handler/service tests plus Bruno documentation.

Changes

Get Flows by Product Endpoint Implementation

Layer / File(s) Summary
Response standardization helper
internal/response.go, internal/response_test.go
Adds WriteJSONResponse and NotFoundError; tests verify JSON encoding success (200/201), encoding failure (500 error envelope), and errors.Is for NotFound.
Refactor platform/product handlers to helper
internal/platform/handler.go, internal/product/handler.go
Replaces manual bytes.Buffer/json.Encoder logic with internal.WriteJSONResponse and removes unused bytes imports.
Flow handler interface extension
internal/flow/interface.go
Adds GetFlowsByProduct(w http.ResponseWriter, r *http.Request) to the flow Handler interface.
SQL and generated query updates
internal/flow/flows.sql, internal/flow/flows.sql.gen.go, internal/flow/querier.gen.go
GetFlowsByProduct now selects product_id and returns []Flow; adds GetProductById SQL and generated method; updates Querier signatures.
Flow service implementation
internal/flow/service.go
New service with CreateFlow, GetFlowById, and GetFlowsByProduct delegating to queries, mapping pgx.ErrNoRows to internal.NewNotFoundError, and wrapping other errors.
Flow handler wiring and endpoints
internal/flow/handler.go
Refactors handler to use flowService, implements GetFlowsByProduct, and updates CreateFlow/GetFlowById to use WriteJSONResponse and internal.NotFoundError mapping.
Flow handler tests
internal/flow/handler_test.go
Updates mocks to return []Flow, adapts existing tests to new flowService wiring, and adds TestGetFlowsByProduct covering success, invalid id, not-found, and DB error cases; success decodes JSON into []Flow.
Service unit tests
internal/flow/service_test.go
Adds table-driven tests for the service methods validating success, not-found mapping, and error wrapping.
Route registration
router/router.go
Registers GET /api/products/{id}/flows to flowHandler.GetFlowsByProduct.
Bruno flow documentation
_bruno/Flows/Get Flows by Product.yml
Bruno HTTP flow documenting path param, 200 assertion, request options, and example JSON array response schema.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested labels

enhancement

Poem

🐰 A hop, a patch, a tiny test delight,
Flows now gather under product light,
Responses tidy, errors named just so,
Routes and docs aligned — the garden grows. 🌱

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 8.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Adds get flows by product' directly and clearly summarizes the main change: implementation of the GET flows by product endpoint functionality.
Linked Issues check ✅ Passed The pull request fully implements the requirements from issue #36: the GET /products/{id}/flows endpoint [router/router.go], product existence verification [internal/flow/service.go, internal/flow/handler.go], flows query by product_id [internal/flow/flows.sql, internal/flow/flows.sql.gen.go], and returns 200 OK with the required JSON array structure [internal/flow/handler.go, handler_test.go].
Out of Scope Changes check ✅ Passed All changes are directly related to implementing the GET /products/{id}/flows endpoint. Refactoring of handler wiring, error handling abstractions, and shared response helpers are supporting infrastructure necessary for consistent endpoint implementation.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch flows/get-by-product

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@jlpdeveloper
Copy link
Copy Markdown
Contributor Author

@CodeRabbit review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 17, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 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 `@_bruno/Flows/Get` Flows by Product.yml:
- Around line 53-58: The example response for the "Get Flows by Product" flow is
missing the product_id field; update the sample JSON object (the Flow example
with keys id, name, description, created_at, updated_at) to include the
product_id property with an appropriate sample value so the documented response
shape matches the endpoint contract.

In `@internal/flow/handler_test.go`:
- Around line 315-323: The test case named "No Flows Found" in
internal/flow/handler_test.go is asserting a 404 but the handler should return
200 with an empty array when the product exists but has no flows; update that
case so mockFlowQuerier.getFlowsByProductFunc returns (nil, nil) and
expectedStatus is http.StatusOK and assert an empty JSON array response. Also
add a new case (e.g., "Product Not Found") where
mockFlowQuerier.getFlowsByProductFunc returns (nil, pgx.ErrNoRows) and
expectedStatus is http.StatusNotFound to cover the missing-product contract.

In `@internal/flow/handler.go`:
- Around line 88-90: The handler currently treats pgx.ErrNoRows as "product not
found" and returns 404; instead distinguish the two cases by changing the data
layer/query and the handler: have the data function (e.g., GetFlowsByProduct)
return an explicit sentinel error ErrProductNotFound when the product row itself
is missing, and return an empty slice (and nil error) when the product exists
but has zero flows; then update the handler to check for errors.Is(err,
ErrProductNotFound) to call internal.HandleHttpError(..., http.StatusNotFound)
and otherwise treat a nil error with an empty slice as a 200 + [] response,
removing the current errors.Is(err, pgx.ErrNoRows) 404 branch.
🪄 Autofix (Beta)

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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 3e0b6e2a-e50a-43cb-8bd5-6784ddb77e7c

📥 Commits

Reviewing files that changed from the base of the PR and between b594d7b and 8570a86.

📒 Files selected for processing (9)
  • _bruno/Flows/Get Flows by Product.yml
  • internal/flow/handler.go
  • internal/flow/handler_test.go
  • internal/flow/interface.go
  • internal/platform/handler.go
  • internal/product/handler.go
  • internal/response.go
  • internal/response_test.go
  • router/router.go

Comment on lines +53 to +58
"id": 1,
"name": "Flow 1",
"description": "flow 1 desc",
"created_at": "2026-05-15T19:11:09.35732-05:00",
"updated_at": "2026-05-15T19:11:09.35732-05:00"
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Include product_id in the example response payload.

The sample response omits product_id, but the endpoint contract includes it. Please add it to keep docs aligned with actual response shape.

Suggested doc patch
           [
             {
               "id": 1,
+              "product_id": 2,
               "name": "Flow 1",
               "description": "flow 1 desc",
               "created_at": "2026-05-15T19:11:09.35732-05:00",
               "updated_at": "2026-05-15T19:11:09.35732-05:00"
             }
           ]
📝 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.

Suggested change
"id": 1,
"name": "Flow 1",
"description": "flow 1 desc",
"created_at": "2026-05-15T19:11:09.35732-05:00",
"updated_at": "2026-05-15T19:11:09.35732-05:00"
}
"id": 1,
"product_id": 2,
"name": "Flow 1",
"description": "flow 1 desc",
"created_at": "2026-05-15T19:11:09.35732-05:00",
"updated_at": "2026-05-15T19:11:09.35732-05:00"
}
🤖 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 `@_bruno/Flows/Get` Flows by Product.yml around lines 53 - 58, The example
response for the "Get Flows by Product" flow is missing the product_id field;
update the sample JSON object (the Flow example with keys id, name, description,
created_at, updated_at) to include the product_id property with an appropriate
sample value so the documented response shape matches the endpoint contract.

Comment on lines +315 to +323
name: "No Flows Found",
pathID: "2",
mockSetup: func(m *mockFlowQuerier) {
m.getFlowsByProductFunc = func(ctx context.Context, productID int) ([]GetFlowsByProductRow, error) {
return nil, pgx.ErrNoRows
}
},
expectedStatus: http.StatusNotFound,
},
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Update test expectations for empty-flow behavior and add the missing contract case.

The "No Flows Found" case expects 404, which conflicts with the endpoint requirement to return 200 with an empty array when the product exists but has no flows. Please add/adjust cases to explicitly cover:

  1. existing product + zero flows => 200 and [], and
  2. missing product => 404.

As per coding guidelines, **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request".

🤖 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 `@internal/flow/handler_test.go` around lines 315 - 323, The test case named
"No Flows Found" in internal/flow/handler_test.go is asserting a 404 but the
handler should return 200 with an empty array when the product exists but has no
flows; update that case so mockFlowQuerier.getFlowsByProductFunc returns (nil,
nil) and expectedStatus is http.StatusOK and assert an empty JSON array
response. Also add a new case (e.g., "Product Not Found") where
mockFlowQuerier.getFlowsByProductFunc returns (nil, pgx.ErrNoRows) and
expectedStatus is http.StatusNotFound to cover the missing-product contract.

Comment thread internal/flow/handler.go Outdated
…andling

- Introduce `service` struct in `flow` package for business logic: `CreateFlow`, `GetFlowById`, and `GetFlowsByProduct`.
- Update handlers to delegate flow operations to `service` and improve separation of concerns.
- Add `NotFoundError` for standardized error handling in case of missing flows or products.
- Update SQLC query for `GetFlowsByProduct` to include `product_id` in the result.
- Add `GetProductById` query to validate product existence before fetching related flows.
- Added tests for `CreateFlow`, `GetFlowById`, and `GetFlowsByProduct` methods in the `service` layer.
- Updated `CreateFlow` and `GetFlowById` handlers to handle `NotFoundError` and validation errors consistently.
- Enhanced mock implementation to accommodate new service methods and error scenarios.
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (2)
internal/flow/handler_test.go (1)

304-311: ⚡ Quick win

Assert product_id in the success payload.

The success case only checks len(flows), so it would still pass if product_id disappeared from the response. That field is part of the endpoint contract and one of the key query changes in this PR.

Suggested test tightening
 				m.getFlowsByProductFunc = func(ctx context.Context, productID int) ([]Flow, error) {
 					if productID != 1 {
 						return nil, errors.New("unexpected product id")
 					}
 					return []Flow{
-						{ID: 1, Name: "Flow 1"},
-						{ID: 2, Name: "Flow 2"},
+						{ID: 1, ProductID: 1, Name: "Flow 1"},
+						{ID: 2, ProductID: 1, Name: "Flow 2"},
 					}, nil
 				}
@@
 			if tt.expectedStatus == http.StatusOK {
 				var flows []Flow
 				if err := json.NewDecoder(rr.Body).Decode(&flows); err != nil {
 					t.Fatalf("failed to decode response: %v", err)
 				}
 				if len(flows) != 2 {
 					t.Errorf("expected 2 flows, got %v", len(flows))
 				}
+				for _, flow := range flows {
+					if flow.ProductID != 1 {
+						t.Errorf("expected ProductID 1, got %v", flow.ProductID)
+					}
+				}
 				if rr.Header().Get("Content-Type") != "application/json" {
 					t.Errorf("expected Content-Type application/json, got %v", rr.Header().Get("Content-Type"))
 				}
 			}

As per coding guidelines, **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request".

Also applies to: 365-372

🤖 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 `@internal/flow/handler_test.go` around lines 304 - 311, The test's success
assertions only check len(flows) and omit verifying the response includes the
product_id; update the tests that use m.getFlowsByProductFunc (the success cases
around the existing blocks returning []Flow) to decode the handler response JSON
and assert that the top-level "product_id" equals the requested product (e.g.,
1). Locate the test cases referencing m.getFlowsByProductFunc and Flow, parse
the response body into the expected response struct or map, and add an assertion
for product_id in both success scenarios (the blocks around the current
getFlowsByProductFunc stubs, including the other case around lines 365-372).
internal/flow/service_test.go (1)

92-150: ⚡ Quick win

Add test cases for untested service branches introduced in this PR.

Current tests miss: (1) generic DB error in GetFlowById, (2) generic DB error from GetProductById, (3) generic DB error from GetFlowsByProduct, and (4) product exists with no flows (expect empty slice contract).

As per coding guidelines, "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request".

Also applies to: 152-213

🤖 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 `@internal/flow/service_test.go` around lines 92 - 150, The tests for the
service are missing coverage for several error and edge branches: add unit tests
that use the mockFlowQuerier to simulate (1) a generic DB error returned from
GetFlowById (not pgx.ErrNoRows) and assert the propagated error, (2) a generic
DB error from GetProductById and assert propagation when calling the service
method that depends on product lookup, (3) a generic DB error from
GetFlowsByProduct and assert the service surfaces that error, and (4) a case
where a product exists but GetFlowsByProduct returns an empty slice — assert the
service returns an empty slice (not nil) to preserve the contract; implement
these by adding table-driven test cases similar to TestService_GetFlowById that
set mockFlowQuerier.getFlowFunc/getProductFunc/getFlowsByProductFunc to return
the desired errors or empty slice and verify expected error messages or empty
slice results.
🤖 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 `@internal/flow/service.go`:
- Around line 47-51: The GetFlowsByProduct return can propagate a nil []Flow
which JSON-encodes to null; after calling s.queries.GetFlowsByProduct(ctx, id)
and before returning, ensure the returned flows slice is normalized to an empty
slice when nil (e.g., if flows == nil then set flows = make([]Flow, 0)) so the
method (in service.go) always returns an empty array instead of null while
preserving the existing error handling around s.queries.GetFlowsByProduct.
- Around line 20-23: The current error branch checks errors.Is(err,
pgx.ErrNoRows) but INSERT ... RETURNING foreign-key failures surface as
*pgconn.PgError with SQLSTATE "23503", so replace the pgx.ErrNoRows check with
logic that type-asserts err to *pgconn.PgError and, when pgErr.Code == "23503"
(optionally also checking pgErr.Constraint or Detail for the product FK name),
return Flow{}, internal.NewNotFoundError(id, "Product"); otherwise fall back to
the existing fmt.Errorf("failed to create flow: %w", err) handling. Ensure you
import github.com/jackc/pgconn if not already.

In `@internal/response_test.go`:
- Around line 76-79: The test branch for encoding failures currently only checks
status and Content-Type; update the case where tt.expectedStatus ==
http.StatusInternalServerError to also decode the response body (w.Body.Bytes())
as JSON and assert the problem+json payload contains a "detail" field with the
expected error message (or at least that it contains text indicating an encoding
failure) so the contract produced by HandleHttpError is validated; use the same
test table entry's expected message or a concrete substring and reference w,
tt.expectedStatus, and HandleHttpError/response writer usage to locate where to
add the JSON decode and assertion.

---

Nitpick comments:
In `@internal/flow/handler_test.go`:
- Around line 304-311: The test's success assertions only check len(flows) and
omit verifying the response includes the product_id; update the tests that use
m.getFlowsByProductFunc (the success cases around the existing blocks returning
[]Flow) to decode the handler response JSON and assert that the top-level
"product_id" equals the requested product (e.g., 1). Locate the test cases
referencing m.getFlowsByProductFunc and Flow, parse the response body into the
expected response struct or map, and add an assertion for product_id in both
success scenarios (the blocks around the current getFlowsByProductFunc stubs,
including the other case around lines 365-372).

In `@internal/flow/service_test.go`:
- Around line 92-150: The tests for the service are missing coverage for several
error and edge branches: add unit tests that use the mockFlowQuerier to simulate
(1) a generic DB error returned from GetFlowById (not pgx.ErrNoRows) and assert
the propagated error, (2) a generic DB error from GetProductById and assert
propagation when calling the service method that depends on product lookup, (3)
a generic DB error from GetFlowsByProduct and assert the service surfaces that
error, and (4) a case where a product exists but GetFlowsByProduct returns an
empty slice — assert the service returns an empty slice (not nil) to preserve
the contract; implement these by adding table-driven test cases similar to
TestService_GetFlowById that set
mockFlowQuerier.getFlowFunc/getProductFunc/getFlowsByProductFunc to return the
desired errors or empty slice and verify expected error messages or empty slice
results.
🪄 Autofix (Beta)

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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 499f24c7-e7b6-48e8-af0b-6e0375822ff4

📥 Commits

Reviewing files that changed from the base of the PR and between 8570a86 and c1088a8.

📒 Files selected for processing (9)
  • internal/flow/flows.sql
  • internal/flow/flows.sql.gen.go
  • internal/flow/handler.go
  • internal/flow/handler_test.go
  • internal/flow/querier.gen.go
  • internal/flow/service.go
  • internal/flow/service_test.go
  • internal/response.go
  • internal/response_test.go

Comment thread internal/flow/service.go Outdated
Comment on lines +20 to +23
if errors.Is(err, pgx.ErrNoRows) {
return Flow{}, internal.NewNotFoundError(id, "Product")
}
return Flow{}, fmt.Errorf("failed to create flow: %w", err)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

In pgx v5 with PostgreSQL, for INSERT ... RETURNINGthat violates a foreign key, is the errorpgx.ErrNoRowsor*pgconn.PgError (SQLSTATE 23503)?

💡 Result:

In pgx v5, an INSERT... RETURNING statement that violates a foreign key constraint will return a *pgconn.PgError (with SQLSTATE 23503), not pgx.ErrNoRows [1][2]. pgx.ErrNoRows is a specific error returned only when a query that is expected to return at least one row (such as QueryRow) finds no rows in the result set [3][4]. A foreign key violation is a database-level constraint error that occurs during the execution of the statement itself, which pgx surfaces as a *pgconn.PgError [2]. To handle this, you should use errors.As to check for the *pgconn.PgError type and inspect the Code field [2]: var pgErr *pgconn.PgError if errors.As(err, &pgErr) { if pgErr.Code == "23503" { // Handle foreign key violation } }

Citations:


🏁 Script executed:

find . -name "service.go" -type f | grep -E "flow|service" | head -20

Repository: service-atlas/products

Length of output: 93


🏁 Script executed:

cat -n ./internal/flow/service.go | head -35

Repository: service-atlas/products

Length of output: 1109


Handle missing-product DB errors using FK violation mapping, not pgx.ErrNoRows.

Line 20 currently checks pgx.ErrNoRows, but for INSERT ... RETURNING with a missing referenced product, PostgreSQL returns a foreign key violation error (SQLSTATE 23503), which pgx surfaces as *pgconn.PgError, not pgx.ErrNoRows. This causes the error to incorrectly fall through to the generic error handler, returning a 500 instead of a 404 not-found response.

Proposed fix
 import (
 	"context"
 	"errors"
 	"fmt"
 	"products/internal"
 
+	"github.com/jackc/pgx/v5/pgconn"
 	"github.com/jackc/pgx/v5"
 )
@@
 	flow, err := s.queries.CreateFlow(ctx, req.ToParams(id))
 	if err != nil {
-		if errors.Is(err, pgx.ErrNoRows) {
+		var pgErr *pgconn.PgError
+		if errors.As(err, &pgErr) && pgErr.Code == "23503" {
 			return Flow{}, internal.NewNotFoundError(id, "Product")
 		}
 		return Flow{}, fmt.Errorf("failed to create flow: %w", err)
 	}
📝 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.

Suggested change
if errors.Is(err, pgx.ErrNoRows) {
return Flow{}, internal.NewNotFoundError(id, "Product")
}
return Flow{}, fmt.Errorf("failed to create flow: %w", err)
var pgErr *pgconn.PgError
if errors.As(err, &pgErr) && pgErr.Code == "23503" {
return Flow{}, internal.NewNotFoundError(id, "Product")
}
return Flow{}, fmt.Errorf("failed to create flow: %w", err)
🤖 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 `@internal/flow/service.go` around lines 20 - 23, The current error branch
checks errors.Is(err, pgx.ErrNoRows) but INSERT ... RETURNING foreign-key
failures surface as *pgconn.PgError with SQLSTATE "23503", so replace the
pgx.ErrNoRows check with logic that type-asserts err to *pgconn.PgError and,
when pgErr.Code == "23503" (optionally also checking pgErr.Constraint or Detail
for the product FK name), return Flow{}, internal.NewNotFoundError(id,
"Product"); otherwise fall back to the existing fmt.Errorf("failed to create
flow: %w", err) handling. Ensure you import github.com/jackc/pgconn if not
already.

Comment thread internal/flow/service.go
Comment on lines +47 to +51
flows, err := s.queries.GetFlowsByProduct(ctx, id)
if err != nil {
return nil, fmt.Errorf("failed to fetch flows: %w", err)
}
return flows, nil
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Normalize no-result flow lists to an empty slice.

Line 51 can return a nil []Flow when a product exists but has no flows; JSON encoding then becomes null, while the endpoint contract requires an empty array.

Proposed fix
 	flows, err := s.queries.GetFlowsByProduct(ctx, id)
 	if err != nil {
 		return nil, fmt.Errorf("failed to fetch flows: %w", err)
 	}
+	if flows == nil {
+		return []Flow{}, nil
+	}
 	return flows, nil
 }
📝 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.

Suggested change
flows, err := s.queries.GetFlowsByProduct(ctx, id)
if err != nil {
return nil, fmt.Errorf("failed to fetch flows: %w", err)
}
return flows, nil
flows, err := s.queries.GetFlowsByProduct(ctx, id)
if err != nil {
return nil, fmt.Errorf("failed to fetch flows: %w", err)
}
if flows == nil {
return []Flow{}, nil
}
return flows, nil
🤖 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 `@internal/flow/service.go` around lines 47 - 51, The GetFlowsByProduct return
can propagate a nil []Flow which JSON-encodes to null; after calling
s.queries.GetFlowsByProduct(ctx, id) and before returning, ensure the returned
flows slice is normalized to an empty slice when nil (e.g., if flows == nil then
set flows = make([]Flow, 0)) so the method (in service.go) always returns an
empty array instead of null while preserving the existing error handling around
s.queries.GetFlowsByProduct.

Comment thread internal/response_test.go
Comment on lines +76 to +79
} else if tt.expectedStatus == http.StatusInternalServerError {
if w.Header().Get("Content-Type") != "application/problem+json" {
t.Errorf("expected Content-Type application/problem+json, got %s", w.Header().Get("Content-Type"))
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Assert the error payload in the encoding-failure case.

The test only verifies status and Content-Type for encoding errors; it should also assert the error body contract (detail) so regressions in HandleHttpError payload shape/message are caught.

Suggested test assertion
 			} else if tt.expectedStatus == http.StatusInternalServerError {
 				if w.Header().Get("Content-Type") != "application/problem+json" {
 					t.Errorf("expected Content-Type application/problem+json, got %s", w.Header().Get("Content-Type"))
 				}
+				var got map[string]any
+				if err := json.Unmarshal(w.Body.Bytes(), &got); err != nil {
+					t.Fatalf("failed to unmarshal error body: %v", err)
+				}
+				if got["detail"] != "Failed to encode response" {
+					t.Errorf("expected detail %q, got %v", "Failed to encode response", got["detail"])
+				}
 			}

As per coding guidelines, "/*_test.go: Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request".

📝 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.

Suggested change
} else if tt.expectedStatus == http.StatusInternalServerError {
if w.Header().Get("Content-Type") != "application/problem+json" {
t.Errorf("expected Content-Type application/problem+json, got %s", w.Header().Get("Content-Type"))
}
} else if tt.expectedStatus == http.StatusInternalServerError {
if w.Header().Get("Content-Type") != "application/problem+json" {
t.Errorf("expected Content-Type application/problem+json, got %s", w.Header().Get("Content-Type"))
}
var got map[string]any
if err := json.Unmarshal(w.Body.Bytes(), &got); err != nil {
t.Fatalf("failed to unmarshal error body: %v", err)
}
if got["detail"] != "Failed to encode response" {
t.Errorf("expected detail %q, got %v", "Failed to encode response", got["detail"])
}
}
🤖 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 `@internal/response_test.go` around lines 76 - 79, The test branch for encoding
failures currently only checks status and Content-Type; update the case where
tt.expectedStatus == http.StatusInternalServerError to also decode the response
body (w.Body.Bytes()) as JSON and assert the problem+json payload contains a
"detail" field with the expected error message (or at least that it contains
text indicating an encoding failure) so the contract produced by HandleHttpError
is validated; use the same test table entry's expected message or a concrete
substring and reference w, tt.expectedStatus, and HandleHttpError/response
writer usage to locate where to add the JSON decode and assertion.

- Move generated SQLC files from `internal/flow` to `internal/flow/db` for better organization.
- Update imports and type references in the `flow` service, handlers, and tests to use `db` package.
- Adjust mock implementations and test cases to match new package structure.
- Change SQLC output path from `internal/flow` to `internal/flow/db` for better organization and alignment with package structure.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GET /products/{id}/flows — List all flows under a product

1 participant