Skip to content

Conversation

@jbrockopp
Copy link
Contributor

Leftover work for go-vela/community#248

Related to go-gorm/gorm#4416

This is a workaround to a problem found in an upstream library.

This updates the go-vela/server/database/sqlite client to specifically check for the rows returned from queries:

// check if the query returned a record not found error or no rows were returned
if errors.Is(result.Error, gorm.ErrRecordNotFound) || result.RowsAffected == 0 {
return nil, gorm.ErrRecordNotFound
}

You'll note that we are explicitly checking the RowsAffected field is equal to 0 from the result returned by GORM.

This is required due to what initially appears to be a bug in GORM (See above go-gorm/gorm issue for more details).

GORM no longer returns record not found when a query is ran against the database and no rows are returned.

@jbrockopp jbrockopp added the bug Indicates a bug label May 27, 2021
@jbrockopp jbrockopp self-assigned this May 27, 2021
@codecov
Copy link

codecov bot commented May 27, 2021

Codecov Report

Merging #410 (ac75dcd) into master (ba294ee) will increase coverage by 0.34%.
The diff coverage is 88.65%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #410      +/-   ##
==========================================
+ Coverage   85.15%   85.49%   +0.34%     
==========================================
  Files         179      179              
  Lines        5073     5096      +23     
==========================================
+ Hits         4320     4357      +37     
+ Misses        500      493       -7     
+ Partials      253      246       -7     
Impacted Files Coverage Δ
database/sqlite/user.go 53.84% <42.85%> (+3.84%) ⬆️
database/sqlite/log.go 72.60% <85.71%> (+5.47%) ⬆️
database/sqlite/repo.go 71.79% <85.71%> (+5.12%) ⬆️
database/sqlite/build.go 93.22% <100.00%> (+7.76%) ⬆️
database/sqlite/hook.go 89.47% <100.00%> (+6.14%) ⬆️
database/sqlite/secret.go 78.68% <100.00%> (+3.68%) ⬆️
database/sqlite/service.go 86.20% <100.00%> (+1.02%) ⬆️
database/sqlite/step.go 86.20% <100.00%> (+1.02%) ⬆️
database/sqlite/worker.go 89.47% <100.00%> (+1.23%) ⬆️

@jbrockopp jbrockopp marked this pull request as ready for review May 27, 2021 19:48
@jbrockopp jbrockopp requested a review from a team as a code owner May 27, 2021 19:48
Copy link
Contributor

@KellyMerrick KellyMerrick left a comment

Choose a reason for hiding this comment

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

lgtm

@jbrockopp jbrockopp merged commit a8a38d5 into master May 28, 2021
@jbrockopp jbrockopp deleted the fix/database/sqlite/record_not_found branch May 28, 2021 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Indicates a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants