Skip to content

Conversation

@jbrockopp
Copy link
Contributor

@jbrockopp jbrockopp commented Jun 22, 2022

Based off of #574

This change continues the refactor effort initially introduced in the above PR.

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

This contains a UserService interface declaring all functions necessary for user based interactions with the database:

// UserService represents the Vela interface for user
// functions with the supported Database backends.
//
// nolint: revive // ignore name stutter
type UserService interface {
// User Data Definition Language Functions
//
// https://en.wikipedia.org/wiki/Data_definition_language
// CreateUserIndexes defines a function that creates the indexes for the users table.
CreateUserIndexes() error
// CreateUserTable defines a function that creates the users table.
CreateUserTable(string) error
// User Data Manipulation Language Functions
//
// https://en.wikipedia.org/wiki/Data_manipulation_language
// CountUsers defines a function that gets the count of all users.
CountUsers() (int64, error)
// CreateUser defines a function that creates a new user.
CreateUser(*library.User) error
// DeleteUser defines a function that deletes an existing user.
DeleteUser(*library.User) error
// GetUser defines a function that gets a user by ID.
GetUser(int64) (*library.User, error)
// GetUserForName defines a function that gets a user by name.
GetUserForName(string) (*library.User, error)
// ListUsers defines a function that gets a list of all users.
ListUsers() ([]*library.User, error)
// ListLiteUsers defines a function that gets a lite list of users.
ListLiteUsers(int, int) ([]*library.User, int64, error)
// UpdateUser defines a function that updates an existing user.
UpdateUser(*library.User) error
}

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

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

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

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

@jbrockopp jbrockopp self-assigned this Jun 22, 2022
@codecov
Copy link

codecov bot commented Jun 22, 2022

Codecov Report

Merging #663 (9445022) into master (4c3147f) will increase coverage by 0.20%.
The diff coverage is 78.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #663      +/-   ##
==========================================
+ Coverage   54.94%   55.14%   +0.20%     
==========================================
  Files         195      201       +6     
  Lines       15962    15882      -80     
==========================================
- Hits         8770     8758      -12     
+ Misses       6818     6748      -70     
- Partials      374      376       +2     
Impacted Files Coverage Δ
api/authenticate.go 0.00% <0.00%> (ø)
api/metrics.go 0.00% <0.00%> (ø)
api/user.go 0.00% <0.00%> (ø)
database/user/user.go 51.72% <51.72%> (ø)
database/sqlite/sqlite.go 60.75% <66.66%> (+1.43%) ⬆️
database/postgres/postgres.go 60.33% <72.72%> (+1.75%) ⬆️
database/user/create.go 76.00% <76.00%> (ø)
database/user/update.go 76.00% <76.00%> (ø)
database/user/list_lite.go 76.31% <76.31%> (ø)
database/user/list.go 78.04% <78.04%> (ø)
... and 12 more

@jbrockopp jbrockopp changed the title refactor(database): refactor(database): move user logic into separate package Jun 22, 2022
@jbrockopp jbrockopp added the enhancement Indicates an improvement to a feature label Jun 22, 2022
@jbrockopp jbrockopp marked this pull request as ready for review June 28, 2022 20:21
@jbrockopp jbrockopp requested a review from a team as a code owner June 28, 2022 20:21
Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

Maybe point out in the docstrings ListUsers and ListLiteUsers differ (what makes it "Lite").
It looks like Lite just slips decrypting fields, right? Anything else?

Other than that, it looks good to me.

@jbrockopp
Copy link
Contributor Author

jbrockopp commented Jun 29, 2022

@cognifloyd the ListLiteUsers() function only returns the id and name fields (Select):

err = e.client.
Table(constants.TableUser).
Select("id", "name").
Limit(perPage).
Offset(offset).
Find(&u).
Error

This function is called when someone sends a GET request to the /api/v1/users endpoint.

The ListUsers() function returns everything:

err = e.client.
Table(constants.TableUser).
Find(&u).
Error

This function is called when someone sends a GET request to the /api/v1/admin/users endpoint.

NOTE: The ListUsers() function can only be used by platform admins.

Do you have thoughts on what kind of changes you'd like to see regarding the function comments?

Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

Maybe something like this?

Copy link
Contributor

@kneal kneal left a comment

Choose a reason for hiding this comment

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

LGTM 🐬

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 - really like this new pattern

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.

6 participants