Skip to content

Conversation

@cce
Copy link
Contributor

@cce cce commented Oct 29, 2025

Summary

While working on #6466, I discovered a bug where error-handling wasn't working as I intended because I was misusing errors.As on a pointer error-type (that implemented the error interface using value receiver methods).

This errortype linter would have caught the bug, added here to our custom warning linter.

It also caught a similar bug handling sqlite3.Error in util/db/initialize.go, fixed here.

Test Plan

Existing tests should pass.

@cce cce added the Bug-Fix label Oct 29, 2025
@cce cce changed the title CI: add errortype linter and fix errors db: fix Initialize sqlite3 error handling, add errortype linter Oct 29, 2025
@cce cce changed the title db: fix Initialize sqlite3 error handling, add errortype linter db: fix db.Initialize() sqlite3 error handling, add errortype linter Oct 29, 2025
@cce cce changed the title db: fix db.Initialize() sqlite3 error handling, add errortype linter db: fix db.Initialize() sqlite3 error checking, add errortype linter Oct 29, 2025
@cce cce requested a review from Copilot October 29, 2025 16:39
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a bug in SQLite error handling and adds the errortype linter to prevent similar issues in the future. The bug involved incorrectly using errors.As with a pointer type for sqlite3.Error, which doesn't implement the error interface via pointer receiver methods.

  • Fixed sqlite3.Error type declaration in util/db/initialize.go from pointer to value type
  • Added errortype linter configuration to catch error handling bugs
  • Updated TransactionInLedgerError.Error() to use pointer receiver for consistency
  • Added compile-time checks verifying error interface implementations

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
util/db/initialize.go Changed sqlite3.Error from pointer to value type to fix errors.As usage
ledger/ledgercore/error.go Added error interface verification checks and changed TransactionInLedgerError.Error() to pointer receiver
.golangci-warnings.yml Added errortype linter configuration with appropriate settings
.custom-gcl.yml Added errortype plugin dependency configuration

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codecov
Copy link

codecov bot commented Oct 29, 2025

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 47.28%. Comparing base (9dc5156) to head (cffe002).
⚠️ Report is 5 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
ledger/ledgercore/error.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6477      +/-   ##
==========================================
+ Coverage   47.05%   47.28%   +0.22%     
==========================================
  Files         667      659       -8     
  Lines       88831    88225     -606     
==========================================
- Hits        41802    41713      -89     
+ Misses      44284    43745     -539     
- Partials     2745     2767      +22     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

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

Good catch!

@cce cce requested review from gmalouf and jannotti October 31, 2025 03:15
@cce cce merged commit 7dbdb38 into algorand:master Oct 31, 2025
40 checks passed
@cce cce deleted the errortype branch October 31, 2025 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants