Fix to use only needed columns from tables to get repository git paths #3870
Conversation
lafriks
added
type/bug
type/enhancement
Codecov Report
@@ Coverage Diff @@
## master #3870 +/- ##
==========================================
+ Coverage 20.13% 20.18% +0.04%
==========================================
Files 145 145
Lines 29124 29151 +27
==========================================
+ Hits 5864 5883 +19
- Misses 22368 22374 +6
- Partials 892 894 +2
Continue to review full report at Codecov.
|
bkcsoft
added
the
lgtm/need 2
models/repo.go
Outdated
| @@ -479,6 +488,41 @@ func (repo *Repository) mustOwner(e Engine) *User { | |||
| return repo.Owner | |||
| } | |||
|
|
|||
| func (repo *Repository) getOwnerName(e Engine) error { | |||
| if repo.OwnerName != "" { | |||
| func (repo *Repository) mustOwnerName(e Engine) string { | ||
| if err := repo.getOwnerName(e); err != nil { | ||
| log.Error(4, "Error loading repository owner name: %v", err) | ||
| return "error" |
There was a problem hiding this comment.
So error is now a reserved username/organization name? Then it should be added to said list 🙂
There was a problem hiding this comment.
It already returns owner name error in mustOwner, I just keep previous behaviour, this should be fixed in other PR
| } | ||
|
|
||
| u := new(User) | ||
| has, err := e.ID(repo.OwnerID).Cols("name").Get(u) |
There was a problem hiding this comment.
Other than a slight increase in memory usage, I don't see any point in only loading the name. The database still has to find and fetch the entire row 🤔 I would change this to just mustOwner()
There was a problem hiding this comment.
That's needed because it is used in migrations and can be that struct has more fields than database has so there will be error in select that database does not contain such column
bkcsoft
added
lgtm/need 1
bkcsoft
added
lgtm/done
| } | ||
|
|
||
| u := new(User) | ||
| has, err := e.ID(repo.OwnerID).Cols("name").Get(u) |
There was a problem hiding this comment.
has, err := e.Table("repository").Select("owner_name").Where("id=?", repo.OwnerID).Get(&repo.OwnerName)
There was a problem hiding this comment.
@lunny do you want me to submit PR to change this to master first?
There was a problem hiding this comment.
Do you mean something like:
has, err := e.Table("user").Select("name").Where("id=?", repo.OwnerID).Get(&repo.OwnerName)
There was a problem hiding this comment.
@lafriks the currently code is OK. I only want to say there is another way to do that.
There was a problem hiding this comment.
ok, thanks, I will keep in mind that
Fixes #3864