Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix to use only needed columns from tables to get repository git paths #3870

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 47 additions & 3 deletions models/repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,7 @@ func NewRepoContext() {
type Repository struct {
ID int64 `xorm:"pk autoincr"`
OwnerID int64 `xorm:"UNIQUE(s)"`
OwnerName string `xorm:"-"`
Owner *User `xorm:"-"`
LowerName string `xorm:"UNIQUE(s) INDEX NOT NULL"`
Name string `xorm:"INDEX NOT NULL"`
Expand Down Expand Up @@ -225,9 +226,17 @@ func (repo *Repository) MustOwner() *User {
return repo.mustOwner(x)
}

// MustOwnerName always returns valid owner name to avoid
// conceptually impossible error handling.
// It returns "error" and logs error details when error
// occurs.
func (repo *Repository) MustOwnerName() string {
return repo.mustOwnerName(x)
}

// FullName returns the repository full name
func (repo *Repository) FullName() string {
return repo.MustOwner().Name + "/" + repo.Name
return repo.MustOwnerName() + "/" + repo.Name
}

// HTMLURL returns the repository HTML URL
Expand Down Expand Up @@ -479,6 +488,41 @@ func (repo *Repository) mustOwner(e Engine) *User {
return repo.Owner
}

func (repo *Repository) getOwnerName(e Engine) error {
if len(repo.OwnerName) > 0 {
return nil
}

if repo.Owner != nil {
repo.OwnerName = repo.Owner.Name
return nil
}

u := new(User)
has, err := e.ID(repo.OwnerID).Cols("name").Get(u)
Copy link
Member

Choose a reason for hiding this comment

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

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()

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

has, err := e.Table("repository").Select("owner_name").Where("id=?", repo.OwnerID).Get(&repo.OwnerName)

Copy link
Member Author

Choose a reason for hiding this comment

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

@lunny do you want me to submit PR to change this to master first?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean something like:

has, err := e.Table("user").Select("name").Where("id=?", repo.OwnerID).Get(&repo.OwnerName)

Copy link
Member

Choose a reason for hiding this comment

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

@lafriks the currently code is OK. I only want to say there is another way to do that.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, thanks, I will keep in mind that

if err != nil {
return err
} else if !has {
return ErrUserNotExist{repo.OwnerID, "", 0}
}
repo.OwnerName = u.Name
return nil
}

// GetOwnerName returns the repository owner name
func (repo *Repository) GetOwnerName() error {
return repo.getOwnerName(x)
}

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"
Copy link
Member

Choose a reason for hiding this comment

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

So error is now a reserved username/organization name? Then it should be added to said list 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

It already returns owner name error in mustOwner, I just keep previous behaviour, this should be fixed in other PR

}

return repo.OwnerName
}

// ComposeMetas composes a map of metas for rendering external issue tracker URL.
func (repo *Repository) ComposeMetas() map[string]string {
unit, err := repo.GetUnit(UnitTypeExternalTracker)
Expand Down Expand Up @@ -590,7 +634,7 @@ func (repo *Repository) GetBaseRepo() (err error) {
}

func (repo *Repository) repoPath(e Engine) string {
return RepoPath(repo.mustOwner(e).Name, repo.Name)
return RepoPath(repo.mustOwnerName(e), repo.Name)
}

// RepoPath returns the repository path
Expand Down Expand Up @@ -2141,7 +2185,7 @@ func ReinitMissingRepositories() error {
// SyncRepositoryHooks rewrites all repositories' pre-receive, update and post-receive hooks
// to make sure the binary and custom conf path are up-to-date.
func SyncRepositoryHooks() error {
return x.Where("id > 0").Iterate(new(Repository),
return x.Cols("owner_id", "name").Where("id > 0").Iterate(new(Repository),
func(idx int, bean interface{}) error {
if err := createDelegateHooks(bean.(*Repository).RepoPath()); err != nil {
return fmt.Errorf("SyncRepositoryHook: %v", err)
Expand Down
2 changes: 1 addition & 1 deletion models/wiki.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ func WikiPath(userName, repoName string) string {

// WikiPath returns wiki data path for given repository.
func (repo *Repository) WikiPath() string {
return WikiPath(repo.MustOwner().Name, repo.Name)
return WikiPath(repo.MustOwnerName(), repo.Name)
}

// HasWiki returns true if repository has wiki.
Expand Down