Copilot commented on code in PR #95: URL: https://github.com/apache/incubator-seata-go-samples/pull/95#discussion_r3385098071
########## at/ecommerce/order/main.go: ########## @@ -0,0 +1,65 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package main + +import ( + "database/sql" + "net/http" + "os" + + "github.com/gin-gonic/gin" + "seata.apache.org/seata-go-samples/util" + "seata.apache.org/seata-go/pkg/client" + "seata.apache.org/seata-go/pkg/util/log" +) + +var ( + db *sql.DB + inventoryService = "http://127.0.0.1:18081"; + accountService = "http://127.0.0.1:18082"; +) + +func main() { + client.InitPath("conf/seatago.yml") + if err := util.SetDefaultEnv("MYSQL_DB", "seata_ecommerce_order"); err != nil { + log.Fatalf("set MYSQL_DB default error: %v", err) + } + if value := os.Getenv("INVENTORY_SERVICE_URL"); value != "" { + inventoryService = value + } + if value := os.Getenv("ACCOUNT_SERVICE_URL"); value != "" { + accountService = value + } + db = util.GetAtMySqlDb() + + r := gin.Default() + r.POST("/createOrder", createOrderHandler) + + if err := r.Run(":18080"); err != nil { + log.Fatalf("start order service fatal: %v", err) + } +} + +func createOrderHandler(c *gin.Context) { + log.Infof("receive create order request") + if err := createOrder(c); err != nil { + c.JSON(util.StatusCodeForError(err), util.APIResponse{Error: err.Error()}) + return + } + c.JSON(http.StatusOK, util.APIResponse{Message: "create order ok"}) +} Review Comment: Returning `err.Error()` directly to clients can leak internal details (e.g., SQL driver errors, network errors, stack-y messages) that are useful for attackers and noisy for clients. Suggestion: for non-validation/conflict errors, log the detailed error server-side and return a generic message (while keeping detailed messages for `ValidationError`/`ConflictError` where appropriate). This same pattern appears in inventory/account handlers too. ########## util/http.go: ########## @@ -0,0 +1,87 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package util + +import ( + "errors" + "net/http" +) + +type APIResponse struct { + Message string `json:"message,omitempty"` + Error string `json:"error,omitempty"` +} + +type ValidationError struct { + Message string +} + +func (e *ValidationError) Error() string { + return e.Message +} + +func NewValidationError(message string) error { + return &ValidationError{Message: message} +} + +type ConflictError struct { + Message string +} + +func (e *ConflictError) Error() string { + return e.Message +} + +func NewConflictError(message string) error { + return &ConflictError{Message: message} +} + +type DownstreamError struct { + StatusCode int + Message string +} + +func (e *DownstreamError) Error() string { + return e.Message +} + +func NewDownstreamError(statusCode int, message string) error { + return &DownstreamError{StatusCode: statusCode, Message: message} +} + +func StatusCodeForError(err error) int { + var validationErr *ValidationError + if errors.As(err, &validationErr) { + return http.StatusBadRequest + } + + var conflictErr *ConflictError + if errors.As(err, &conflictErr) { + return http.StatusConflict + } + + var downstreamErr *DownstreamError + if errors.As(err, &downstreamErr) { + if downstreamErr.StatusCode == http.StatusConflict { + return http.StatusConflict + } + return http.StatusBadGateway + } + + return http.StatusInternalServerError +} Review Comment: `DownstreamError` collapses all non-409 downstream HTTP responses into `502 Bad Gateway`. This will misreport client errors (e.g., downstream `400` validation errors from inventory/account) as `502`, making it harder for clients to act correctly. Recommendation: propagate downstream 4xx status codes (and possibly 5xx as 502/504) rather than only special-casing 409. ########## util/env.go: ########## @@ -0,0 +1,27 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package util + +import "os" + +func SetDefaultEnv(key string, value string) error { + if _, exists := os.LookupEnv(key); exists { + return nil + } + return os.Setenv(key, value) +} Review Comment: `os.LookupEnv` treats an explicitly-set empty value (e.g. `MYSQL_HOST=""`) as “exists”, so defaults will *not* be applied and the DSN can become invalid. Consider treating empty values as unset (e.g., check the looked-up value and fall back when it’s empty) so behavior matches the previous `os.Getenv(key) == ""` logic. ########## util/db.go: ########## @@ -45,19 +46,23 @@ func GetXAMySqlDb() *sql.DB { } func defaultEnv() { - if os.Getenv("MYSQL_HOST") == "" { - _ = os.Setenv("MYSQL_HOST", "127.0.0.1") + mustSetDefaultEnv("MYSQL_HOST", "127.0.0.1") + mustSetDefaultEnv("MYSQL_PORT", "3306") + mustSetDefaultEnv("MYSQL_USERNAME", "root") + if _, exists := os.LookupEnv("MYSQL_PASSWORD"); !exists { + if rootPassword, rootExists := os.LookupEnv("MYSQL_ROOT_PASSWORD"); rootExists { + if err := os.Setenv("MYSQL_PASSWORD", rootPassword); err != nil { + panic(fmt.Sprintf("set MYSQL_PASSWORD from MYSQL_ROOT_PASSWORD error: %v", err)) + } + } else if err := os.Setenv("MYSQL_PASSWORD", "123456"); err != nil { + panic(fmt.Sprintf("set MYSQL_PASSWORD default error: %v", err)) + } } Review Comment: Falling back to `MYSQL_PASSWORD=123456` when neither `MYSQL_PASSWORD` nor `MYSQL_ROOT_PASSWORD` is set can lead to confusing runtime failures (docker-compose requires `MYSQL_ROOT_PASSWORD`, but local services may not have it exported and will silently use `123456`). Consider failing fast with a clear panic/error message when neither is provided, or align the default with the documented/required compose flow to reduce “works on my machine” drift. ########## at/ecommerce/README.md: ########## @@ -0,0 +1,134 @@ +<!-- + Licensed to the Apache Software Foundation (ASF) under one or more + contributor license agreements. See the NOTICE file distributed with + this work for additional information regarding copyright ownership. + The ASF licenses this file to You under the Apache License, Version 2.0 + (the "License"); you may not use this file except in compliance with + the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +--> + +# AT E-commerce Sample + +This sample demonstrates an e-commerce order flow in Seata Go AT mode. + +The scenario contains three independent Go services: + +1. `order-service` creates the order record and starts the global transaction +2. `inventory-service` deducts stock +3. `account-service` deducts the user balance + +All three operations must succeed or fail together. If the account balance is insufficient, `account-service` rejects the request and Seata automatically rolls back the order creation and inventory deduction through `undo_log`. + +## Directory Layout + +- `order/`: `order-service` +- `inventory/`: `inventory-service` +- `account/`: `account-service` +- `sql/mysql_ecommerce.sql`: schema and seed data for the three MySQL databases +- `docker-compose.yml`: MySQL and Seata Server + +## Start the Infrastructure + +Configure the MySQL root password first: + +```bash +cd at/ecommerce +cp .env.example .env +``` + +Edit `.env` and set a non-trivial `MYSQL_ROOT_PASSWORD` before starting the sample. The compose file now requires this value explicitly. + +For the local Go services, export either `MYSQL_PASSWORD` or `MYSQL_ROOT_PASSWORD` with the same value. If `MYSQL_PASSWORD` is unset, the sample falls back to `MYSQL_ROOT_PASSWORD` automatically: + +```bash +export MYSQL_ROOT_PASSWORD=your-password +# optional: +export MYSQL_PASSWORD="$MYSQL_ROOT_PASSWORD" +``` + +```bash +cd at/ecommerce +docker-compose up -d +``` + +Default ports: + +- MySQL: `3306` +- Seata Server: `8091` +- order-service: `18080` +- inventory-service: `18081` +- account-service: `18082` + +## Start the Three Services + +From the repository root, open three terminals and run: + +```bash +go run ./at/ecommerce/order +go run ./at/ecommerce/inventory +go run ./at/ecommerce/account +``` + +The services use `conf/seatago.yml`, so run them from the repository root as shown above. + +## Run the Success Scenario + +The account seed balance is `50`, so a `money` value below that limit commits successfully: + +```bash +curl -X POST http://127.0.0.1:18080/createOrder \ + -H 'Content-Type: application/json' \ + -d '{"userId":"U100001","commodityCode":"C100001","count":2,"money":30}' +``` + +Expected result: + +- a new row is inserted into `seata_ecommerce_order.order_tbl` +- `seata_ecommerce_inventory.inventory_tbl.stock` decreases from `100` to `98` +- `seata_ecommerce_account.account_tbl.balance` decreases from `50` to `20` + +## Run the Rollback Scenario + +This request exceeds the seed balance and makes `account-service` reject the deduction: + +```bash +curl -X POST http://127.0.0.1:18080/createOrder \ + -H 'Content-Type: application/json' \ + -d '{"userId":"U100001","commodityCode":"C100001","count":2,"money":100}' +``` + +Expected result: + +- `account-service` returns `insufficient balance for userId U100001` +- the global transaction fails in `order-service` +- `seata_ecommerce_order.order_tbl` does not gain a new committed row +- `seata_ecommerce_inventory.inventory_tbl.stock` remains unchanged from its value before this request +- `seata_ecommerce_account.account_tbl.balance` remains unchanged from its value before this request + +## Reset the Demo Data + +If you want to re-run the checks from the initial database state, recreate the sample containers and volumes: + +```bash +cd at/ecommerce +docker-compose down -v +docker-compose up -d +``` + +## Verify in MySQL + Review Comment: These verification commands hardcode `-p123456`, but the compose setup requires `MYSQL_ROOT_PASSWORD` to be set (and the doc encourages a strong password). To avoid confusion, consider using an env-substituted example (e.g. `-p"$MYSQL_ROOT_PASSWORD"`), or explicitly show a placeholder password consistently with the earlier `.env` instructions. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
