Skip to content

Conversation

@jbrockopp
Copy link
Contributor

@jbrockopp jbrockopp commented Aug 27, 2022

Based off of #574 and #663

This change continues the refactor efforts initially introduced in the above PRs.

This adds a new repo package to the github.com/go-vela/server/database package.

This contains a RepoService interface declaring all functions necessary for repo based interactions with the database:

// RepoService represents the Vela interface for repo
// functions with the supported Database backends.
//
//nolint:revive // ignore name stutter
type RepoService interface {
// Repo Data Definition Language Functions
//
// https://en.wikipedia.org/wiki/Data_definition_language
// CreateRepoIndexes defines a function that creates the indexes for the repos table.
CreateRepoIndexes() error
// CreateRepoTable defines a function that creates the repos table.
CreateRepoTable(string) error
// Repo Data Manipulation Language Functions
//
// https://en.wikipedia.org/wiki/Data_manipulation_language
// CountRepos defines a function that gets the count of all repos.
CountRepos() (int64, error)
// CountReposForOrg defines a function that gets the count of repos by org name.
CountReposForOrg(string, map[string]interface{}) (int64, error)
// CountReposForUser defines a function that gets the count of repos by user ID.
CountReposForUser(*library.User, map[string]interface{}) (int64, error)
// CreateRepo defines a function that creates a new repo.
CreateRepo(*library.Repo) error
// DeleteRepo defines a function that deletes an existing repo.
DeleteRepo(*library.Repo) error
// GetRepo defines a function that gets a repo by ID.
GetRepo(int64) (*library.Repo, error)
// GetRepoForOrg defines a function that gets a repo by org and repo name.
GetRepoForOrg(string, string) (*library.Repo, error)
// ListRepos defines a function that gets a list of all repos.
ListRepos() ([]*library.Repo, error)
// ListReposForOrg defines a function that gets a list of repos by org name.
ListReposForOrg(string, string, map[string]interface{}, int, int) ([]*library.Repo, int64, error)
// ListReposForUser defines a function that gets a list of repos by user ID.
ListReposForUser(*library.User, string, map[string]interface{}, int, int) ([]*library.Repo, int64, error)
// UpdateRepo defines a function that updates an existing repo.
UpdateRepo(*library.Repo) error
}

This package also contains the engine which implements the above service interface:

// engine represents the repo functionality that implements the RepoService interface.
engine struct {
// engine configuration settings used in repo functions
config *config
// gorm.io/gorm database client used in repo functions
//
// https://pkg.go.dev/gorm.io/gorm#DB
client *gorm.DB
// sirupsen/logrus logger used in repo functions
//
// https://pkg.go.dev/github.com/sirupsen/logrus#Entry
logger *logrus.Entry
}

This engine contains no raw SQL queries for integrating with the repos table.

Instead, we leverage our DB library's (https://gorm.io/) agnostic abstraction for integrating with that table.

@jbrockopp jbrockopp self-assigned this Aug 27, 2022
@codecov
Copy link

codecov bot commented Aug 27, 2022

Codecov Report

Merging #687 (b0b3f00) into main (49e720d) will decrease coverage by 0.00%.
The diff coverage is 79.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #687      +/-   ##
==========================================
- Coverage   55.09%   55.09%   -0.01%     
==========================================
  Files         201      210       +9     
  Lines       15959    15912      -47     
==========================================
- Hits         8793     8767      -26     
+ Misses       6788     6768      -20     
+ Partials      378      377       -1     
Impacted Files Coverage Δ
api/metrics.go 0.00% <0.00%> (ø)
api/repo.go 0.00% <0.00%> (ø)
api/scm.go 0.00% <0.00%> (ø)
api/user.go 0.00% <0.00%> (ø)
api/webhook.go 0.00% <0.00%> (ø)
database/repo/repo.go 51.72% <51.72%> (ø)
database/sqlite/sqlite.go 62.18% <66.66%> (+1.42%) ⬆️
database/postgres/postgres.go 62.04% <72.72%> (+1.71%) ⬆️
database/repo/create.go 76.92% <76.92%> (ø)
database/repo/update.go 76.92% <76.92%> (ø)
... and 18 more

@jbrockopp jbrockopp requested a review from a team as a code owner September 10, 2022 20:54
@jbrockopp jbrockopp added the enhancement Indicates an improvement to a feature label Sep 11, 2022
Copy link
Collaborator

@wass3r wass3r left a comment

Choose a reason for hiding this comment

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

i see you replaced .Scan() with .Take() in a few places. based on GORM docs (https://gorm.io/docs/query.html#Retrieving-a-single-object), it looks like Take already does LIMIT 1? Can we remove the preceding .Limit(1) in those instances? clearly it passed unit testing so it must account for it somehow?

@jbrockopp
Copy link
Contributor Author

jbrockopp commented Sep 16, 2022

@wass3r good catch! 👍

To provide some context in case anyone is curious, this is the reason why we're using Take():

https://gorm.io/docs/v2_release_note.html#ErrRecordNotFound

In short, for several of our DB queries, we expect an error if the record doesn't exist.

However, gorm.io changed their behavior from V1 -> V2 regarding when that error message is returned:

go-gorm/gorm#4416

In order to account for this today, we have extra code across our DB package that does this:

// 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
}

Links for reference:

Copy link
Contributor

@ecrupper ecrupper left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Indicates an improvement to a feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants