best practices in examples #2117
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2117 +/- ##
==========================================
- Coverage 75.16% 74.93% -0.23%
==========================================
Files 427 427
Lines 36974 36974
==========================================
- Hits 27792 27708 -84
- Misses 7993 8066 +73
- Partials 1189 1200 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
summaryInferred base version: v3.135.11 |
There was a problem hiding this comment.
Pull request overview
This PR updates the pagination example to surface context cancellation/timeouts when a query produces no result set / rows, instead of silently treating it as end-of-data.
Changes:
- Added an explicit
ctx.Err()check in the “no more rows” branch ofselectPaging().
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
@copilot find the same problems in other examples and fix |
…rl_shortener, webserver) Agent-Logs-Url: https://github.com/ydb-platform/ydb-go-sdk/sessions/7f8e6c7c-194a-4622-aead-567f90cfd21a Co-authored-by: asmyasnikov <14202262+asmyasnikov@users.noreply.github.com>
…ies.go; update changelog Agent-Logs-Url: https://github.com/ydb-platform/ydb-go-sdk/sessions/7f8e6c7c-194a-4622-aead-567f90cfd21a Co-authored-by: asmyasnikov <14202262+asmyasnikov@users.noreply.github.com>
Found and fixed the same context cancellation issue in three additional examples (commit cb32275):
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot migrate all examples from db,Table() usage to db.Query() usage |
Migrate the following example files from using db.Table() to db.Query(): - examples/ddl: executeQuery uses c.Exec() with ImplicitTxControl - examples/pagination: createTable, fillTableWithData, selectPaging use query.Client - examples/ttl/series.go: createTables, addDocument, readDocument, deleteExpired use query.Client; readExpiredBatchTransaction returns ClosableResultSet - examples/decimal/main.go: use db.Query().Exec() for DDL and DoTx for read/write - examples/serverless/url_shortener/service.go: createTable, insertShort, selectLong - examples/serverless/healthcheck/service.go: createTableIfNotExists, upsertRows - examples/topic/cdc-cache-bus-freeseats/database.go: DROP TABLE via Query.Exec - examples/topic/cdc-fill-and-read/tables.go: dropTableIfExists, createTable, fillTable, removeFromTable use query.Client and query.TxActor - examples/opensource_night2024/main.go: fillTableWeather uses Query.Exec with UPSERT INTO ... AS_TABLE instead of BulkUpsert Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: asmyasnikov <14202262+asmyasnikov@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: asmyasnikov <14202262+asmyasnikov@users.noreply.github.com>
Agent-Logs-Url: https://github.com/ydb-platform/ydb-go-sdk/sessions/62288c34-9005-4e86-aeda-5b3a6c9eba7e Co-authored-by: asmyasnikov <14202262+asmyasnikov@users.noreply.github.com>
Agent-Logs-Url: https://github.com/ydb-platform/ydb-go-sdk/sessions/62288c34-9005-4e86-aeda-5b3a6c9eba7e Co-authored-by: asmyasnikov <14202262+asmyasnikov@users.noreply.github.com>
Done in commit f8a1f3a. All applicable examples have been migrated from
Examples kept as-is (use Table-specific APIs with no Query equivalent): Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
There was a problem hiding this comment.
Severity: Major
Confidence: Medium
In readExpiredBatchTransaction, the query.ClosableResultSet is created via s.QueryResultSet() inside a c.Do() closure (lines 54–68) and then returned to the caller (line 74). When the closure returns, the session is returned to the pool and may be reused or closed. Unlike the old table.Session.Execute() which fully materialized results in memory, the new query.Session.QueryResultSet() is backed by a streaming gRPC connection (streamResult) tied to the session.
The caller deleteExpired (line 115) then attempts to iterate rs.Rows(ctx) on a result set whose underlying session has already been released. This can lead to reading stale/invalid data or a closed-stream error.
The safe pattern (already used in readDocument at lines 172–222) is to consume the result set inside the c.Do() closure while the session is still held.
Suggested fix: move the row iteration into the c.Do() closure, or use c.QueryResultSet() at the client level (which manages the session internally).
There was a problem hiding this comment.
AI Review Summary
Verdict: ❌ 1 critical issue(s) found
Critical issues
- Major | Medium:
readExpiredBatchTransactionreturns a streamingClosableResultSetafter its session has been returned to the pool —examples/ttl/series.go:53-74
Other findings
- Minor | Medium:
selectLonglost the explicit "hash not found" error; callers now see a rawquery.ErrNoRowsinstead —examples/serverless/url_shortener/service.go:207-234 - Minor | Medium:
selectPagingscans into pointer types (*string,*uint32) for NOT NULL columns and dereferences without nil checks —examples/pagination/cities.go:77-91 - Minor | Low:
createTableslostWithUniformPartitionspartitioning for thedocumentstable;docTablePartitionCountis now dead code —examples/ttl/series.go:264-275 - Nit | Low: Multiple
CREATE TABLEstatements changed column types from Optional (nullable) to non-nullable, which is a schema compatibility change for pre-existing tables
This review was generated automatically. Critical issues require attention; other findings are advisory.
If this comment was useful, please give it a 👍 — it helps us improve the review bot.
|
Severity: Major | Confidence: Medium
Unlike the old The correct pattern (already used in Suggested fix: either move the row iteration into the |
|
Severity: Minor | Confidence: Medium The old code returned a descriptive error The caller Suggested fix: check for if errors.Is(err, query.ErrNoRows) {
return "", fmt.Errorf("hash '%s' is not found", hash)
} |
|
Severity: Minor | Confidence: Medium
If the table already exists from a previous run using the old nullable schema, or if the schema is changed in the future, these lines will panic on nil dereference. Suggested fix: either use non-pointer types ( |
|
Severity: Minor | Confidence: Low The old The If the partitioning was intentional for performance, consider adding |
Related ticket: https://nda.ya.ru/t/bsuUPpPR7bPRvr
Pull request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Other information