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.

Sign up for GitHub

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

Jump to bottom

models should not depend on macaron #6927

Closed
zeripath opened this issue May 13, 2019 · 12 comments · Fixed by #6931, #6933, #6940 or #10050
Closed

models should not depend on macaron #6927

zeripath opened this issue May 13, 2019 · 12 comments · Fixed by #6931, #6933, #6940 or #10050
Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile

Comments

@zeripath
Copy link
Contributor

The models package dependency tree is rather complex. In particular it currently relies on and imports macaron.

This causes macaron's init to run even in gitea commands that will not run a webserver - it also prevents early mitigation of macaron's init.

We should move methods that require macaron out of models and in to routers or into modules and consider if there are other places in modules where we can avoid importing macaron.
@zeripath
Copy link
Contributor Author

This affects most of models/mail.go:

gitea/models/mail.go

Lines 47 to 119 in 6dbd261

// SendUserMail sends a mail to the user
func SendUserMail(c *macaron.Context, u *User, tpl base.TplName, code, subject, info string) {
data := map[string]interface{}{
"DisplayName": u.DisplayName(),
"ActiveCodeLives": base.MinutesToFriendly(setting.Service.ActiveCodeLives, c.Locale.Language()),
"ResetPwdCodeLives": base.MinutesToFriendly(setting.Service.ResetPwdCodeLives, c.Locale.Language()),
"Code": code,
}
var content bytes.Buffer
if err := templates.ExecuteTemplate(&content, string(tpl), data); err != nil {
log.Error("Template: %v", err)
return
}
msg := mailer.NewMessage([]string{u.Email}, subject, content.String())
msg.Info = fmt.Sprintf("UID: %d, %s", u.ID, info)
mailer.SendAsync(msg)
}
// SendActivateAccountMail sends an activation mail to the user (new user registration)
func SendActivateAccountMail(c *macaron.Context, u *User) {
SendUserMail(c, u, mailAuthActivate, u.GenerateActivateCode(), c.Tr("mail.activate_account"), "activate account")
}
// SendResetPasswordMail sends a password reset mail to the user
func SendResetPasswordMail(c *macaron.Context, u *User) {
SendUserMail(c, u, mailAuthResetPassword, u.GenerateActivateCode(), c.Tr("mail.reset_password"), "recover account")
}
// SendActivateEmailMail sends confirmation email to confirm new email address
func SendActivateEmailMail(c *macaron.Context, u *User, email *EmailAddress) {
data := map[string]interface{}{
"DisplayName": u.DisplayName(),
"ActiveCodeLives": base.MinutesToFriendly(setting.Service.ActiveCodeLives, c.Locale.Language()),
"Code": u.GenerateEmailActivateCode(email.Email),
"Email": email.Email,
}
var content bytes.Buffer
if err := templates.ExecuteTemplate(&content, string(mailAuthActivateEmail), data); err != nil {
log.Error("Template: %v", err)
return
}
msg := mailer.NewMessage([]string{email.Email}, c.Tr("mail.activate_email"), content.String())
msg.Info = fmt.Sprintf("UID: %d, activate email", u.ID)
mailer.SendAsync(msg)
}
// SendRegisterNotifyMail triggers a notify e-mail by admin created a account.
func SendRegisterNotifyMail(c *macaron.Context, u *User) {
data := map[string]interface{}{
"DisplayName": u.DisplayName(),
"Username": u.Name,
}
var content bytes.Buffer
if err := templates.ExecuteTemplate(&content, string(mailAuthRegisterNotify), data); err != nil {
log.Error("Template: %v", err)
return
}
msg := mailer.NewMessage([]string{u.Email}, c.Tr("mail.register_notify"), content.String())
msg.Info = fmt.Sprintf("UID: %d, registration notify", u.ID)
mailer.SendAsync(msg)
}

In fact models/mail.go probably doesn't belong in models at all.

There is one place in models/login_source.go that depends on macaron/binding:

gitea/models/login_source.go

Line 411 in ef2a343

if binding.AlphaDashDotPattern.MatchString(sr.Username) {

I suspect that this isn't the correct function and we have a different more specific check for validity of usernames.

@zeripath
Copy link
Contributor Author

We should probably also look carefully at where we can dissect out macaron from modules - eg. modules/log I think depends on macaron because of the router log but this could/should be cleaned in such a way that it's moved out.

In general we should try to decouple as much of models as possible, and separate modules in to two sections - those that depend on models and those that models depends on.

@lunny lunny added the topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile label May 13, 2019
@lunny lunny mentioned this issue May 13, 2019
Merged
@lunny
Copy link
Member

lunny commented May 13, 2019

If #6931 merged, then we only left

gitea/models/login_source.go

Line 411 in ef2a343

if binding.AlphaDashDotPattern.MatchString(sr.Username) {
.

@lunny lunny mentioned this issue May 13, 2019
Merged
@lunny lunny reopened this May 14, 2019
@lunny lunny mentioned this issue May 14, 2019
Merged
@lunny
Copy link
Member

lunny commented May 14, 2019 •
edited
Loading

@zeripath I would like to split modules as two parties, one will keep modules will be as basic packages dependent by models and other packages, another named service will depend on models and will be invoked by routers.

@zeripath
Copy link
Contributor Author

zeripath commented May 20, 2019 •
edited
Loading

So unfortunately models depends on modules/auth which still depends on macaron. This is not to blame see below...

@zeripath zeripath reopened this May 20, 2019
@zeripath
Copy link
Contributor Author

OK so I'm wrong above. The issue appears to be resulting from dependencies on:

:; for i in $(go list -f '{{ join .Imports  "\n"}}' code.gitea.io/gitea/models); do go list -f '{{ join .Deps "\n" }}' $i | grep "code.gitea.io/gitea/vendor/gopkg.in/macaron.v1" 1> /dev/null && echo $i; done
code.gitea.io/gitea/modules/auth/oauth2
code.gitea.io/gitea/modules/avatar
code.gitea.io/gitea/modules/base
code.gitea.io/gitea/modules/cache
code.gitea.io/gitea/modules/highlight
code.gitea.io/gitea/modules/indexer
code.gitea.io/gitea/modules/mailer
code.gitea.io/gitea/modules/markup
code.gitea.io/gitea/modules/markup/markdown
code.gitea.io/gitea/modules/options
code.gitea.io/gitea/modules/setting
code.gitea.io/gitea/modules/util

@zeripath
Copy link
Contributor Author

Which when looked at with a bit of a more complex script:

for i in $(go list -f '{{ join .Imports  "\n"}}' code.gitea.io/gitea/models); do                        [09:35:43]
    go list -f '{{ join .Deps "\n" }}' $i |
        grep "code.gitea.io/gitea/vendor/gopkg.in/macaron.v1" 1> /dev/null &&
        echo "Checking: $i" 1>&2 &&
        (for j in $(go list -f '{{ join .Imports  "\n"}}' $i); do go list -f '{{ join .Deps "\n" }}' $j | grep "code.gitea.io/gitea/vendor/gopkg.in/macaron.v1" 1> /dev/null && echo "  $j"; done)
    done

Results in:

Checking: code.gitea.io/gitea/modules/auth/oauth2
  code.gitea.io/gitea/modules/setting
Checking: code.gitea.io/gitea/modules/avatar
  code.gitea.io/gitea/modules/setting
Checking: code.gitea.io/gitea/modules/base
  code.gitea.io/gitea/modules/setting
  code.gitea.io/gitea/modules/util
Checking: code.gitea.io/gitea/modules/cache
  code.gitea.io/gitea/modules/setting
  code.gitea.io/gitea/vendor/github.com/go-macaron/cache
Checking: code.gitea.io/gitea/modules/highlight
  code.gitea.io/gitea/modules/setting
Checking: code.gitea.io/gitea/modules/indexer
  code.gitea.io/gitea/modules/setting
Checking: code.gitea.io/gitea/modules/mailer
  code.gitea.io/gitea/modules/base
  code.gitea.io/gitea/modules/setting
Checking: code.gitea.io/gitea/modules/markup
  code.gitea.io/gitea/modules/base
  code.gitea.io/gitea/modules/setting
  code.gitea.io/gitea/modules/util
Checking: code.gitea.io/gitea/modules/markup/markdown
  code.gitea.io/gitea/modules/markup
  code.gitea.io/gitea/modules/setting
  code.gitea.io/gitea/modules/util
Checking: code.gitea.io/gitea/modules/options
  code.gitea.io/gitea/modules/setting
Checking: code.gitea.io/gitea/modules/setting
  code.gitea.io/gitea/modules/session
  code.gitea.io/gitea/vendor/github.com/go-macaron/cache/memcache
  code.gitea.io/gitea/vendor/github.com/go-macaron/cache/redis
  code.gitea.io/gitea/vendor/github.com/go-macaron/cors
  code.gitea.io/gitea/vendor/github.com/go-macaron/session
  code.gitea.io/gitea/vendor/github.com/go-macaron/session/couchbase
  code.gitea.io/gitea/vendor/github.com/go-macaron/session/memcache
  code.gitea.io/gitea/vendor/github.com/go-macaron/session/mysql
  code.gitea.io/gitea/vendor/github.com/go-macaron/session/nodb
  code.gitea.io/gitea/vendor/github.com/go-macaron/session/postgres
  code.gitea.io/gitea/vendor/github.com/go-macaron/session/redis
Checking: code.gitea.io/gitea/modules/util
  code.gitea.io/gitea/modules/setting

Revealing that the naughty packages that independently depend on macaron are as follows:

code.gitea.io/gitea/modules/cache

Through its dependency on: code.gitea.io/gitea/vendor/github.com/go-macaron/cache

code.gitea.io/gitea/modules/setting

Through multiple dependencies:

code.gitea.io/gitea/modules/session
code.gitea.io/gitea/vendor/github.com/go-macaron/cache/memcache
code.gitea.io/gitea/vendor/github.com/go-macaron/cache/redis
code.gitea.io/gitea/vendor/github.com/go-macaron/cors
code.gitea.io/gitea/vendor/github.com/go-macaron/session
code.gitea.io/gitea/vendor/github.com/go-macaron/session/couchbase
code.gitea.io/gitea/vendor/github.com/go-macaron/session/memcache
code.gitea.io/gitea/vendor/github.com/go-macaron/session/mysql
code.gitea.io/gitea/vendor/github.com/go-macaron/session/nodb
code.gitea.io/gitea/vendor/github.com/go-macaron/session/postgres
code.gitea.io/gitea/vendor/github.com/go-macaron/session/redis

@stale
Copy link

stale bot commented Aug 16, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 weeks. Thank you for your contributions.

@stale stale bot added the issue/stale label Aug 16, 2019
@lunny lunny added the issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented label Aug 16, 2019
@stale stale bot removed the issue/stale label Aug 16, 2019
@lunny
Copy link
Member

lunny commented Jan 4, 2020

@zeripath I think this could be closed.

@lunny
Copy link
Member

lunny commented Jan 28, 2020 •
edited
Loading

Closed as when go list -f '{{ join .Imports "\n"}}' code.gitea.io/gitea/models on commit 28216bd :

bufio
bytes
code.gitea.io/gitea/modules/auth/ldap
code.gitea.io/gitea/modules/auth/oauth2
code.gitea.io/gitea/modules/auth/openid
code.gitea.io/gitea/modules/auth/pam
code.gitea.io/gitea/modules/avatar
code.gitea.io/gitea/modules/base
code.gitea.io/gitea/modules/generate
code.gitea.io/gitea/modules/git
code.gitea.io/gitea/modules/log
code.gitea.io/gitea/modules/markup
code.gitea.io/gitea/modules/markup/markdown
code.gitea.io/gitea/modules/options
code.gitea.io/gitea/modules/process
code.gitea.io/gitea/modules/references
code.gitea.io/gitea/modules/secret
code.gitea.io/gitea/modules/setting
code.gitea.io/gitea/modules/structs
code.gitea.io/gitea/modules/timeutil
code.gitea.io/gitea/modules/util
container/list
context
crypto
crypto/aes
crypto/cipher
crypto/md5
crypto/rand
crypto/rsa
crypto/sha1
crypto/sha256
crypto/subtle
crypto/tls
crypto/x509
database/sql
encoding/asn1
encoding/base64
encoding/binary
encoding/hex
encoding/json
encoding/pem
errors
fmt
github.com/denisenkom/go-mssqldb
github.com/dgrijalva/jwt-go
github.com/go-sql-driver/mysql
github.com/gobwas/glob
github.com/keybase/go-crypto/openpgp
github.com/keybase/go-crypto/openpgp/armor
github.com/keybase/go-crypto/openpgp/packet
github.com/lib/pq
github.com/markbates/goth
github.com/pquerna/otp/totp
github.com/satori/go.uuid
github.com/stretchr/testify/assert
github.com/tstranex/u2f
github.com/unknwon/com
golang.org/x/crypto/argon2
golang.org/x/crypto/bcrypt
golang.org/x/crypto/pbkdf2
golang.org/x/crypto/scrypt
golang.org/x/crypto/ssh
gopkg.in/testfixtures.v2
hash
html/template
image/jpeg
image/png
io
io/ioutil
math
math/big
mime/multipart
net/smtp
net/textproto
net/url
os
path
path/filepath
reflect
regexp
sort
strconv
strings
sync
testing
time
unicode/utf8
xorm.io/builder
xorm.io/core
xorm.io/xorm

@lunny lunny closed this as completed Jan 28, 2020
@zeripath zeripath reopened this Jan 28, 2020
@zeripath
Copy link
Contributor Author

It still does...

go list -f '{{ join .Deps "\n"}}' code.gitea.io/gitea/models  | grep macaron

gitea.com/macaron/cache
gitea.com/macaron/cache/memcache
gitea.com/macaron/cache/redis
gitea.com/macaron/cors
gitea.com/macaron/inject
gitea.com/macaron/macaron
gitea.com/macaron/session

I think it's through modules/setting which makes it a bit difficult to remove.

@zeripath
Copy link
Contributor Author

yup it's through setting. which means I guess we need to remove the dependency on macaron from setting.

@lunny lunny mentioned this issue Jan 29, 2020
Merged
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile
Projects
None yet
Milestone
No milestone
2 participants
@lunny @zeripath