Add link to user profile in markdown mention only if user exists

[yardenshoham]

Previously mentioning a user would link to its profile, regardless of whether the user existed. This change tests if the user exists and only if it does - a link to its profile is added.

• Fixes Mentioning should check if the user exists #3444

Before

After

[yardenshoham]

I think the unit tests are failing because of db context issues. This is my first PR that has database interactions, could a maintainer help me understand these errors?

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x30 pc=0xb01cc5]

goroutine 99 [running]:
code.gitea.io/gitea/models/db.GetEngine({0x0, 0x0})
        /workspace/gitea/models/db/context.go:78 +0x45
code.gitea.io/gitea/models/user.IsUserExist({0x0?, 0x0?}, 0x130be1b?, {0xc001624f63, 0x6})
        /workspace/gitea/models/user/user.go:533 +0x53
code.gitea.io/gitea/modules/markup.mentionProcessor(0xc001a6ab40, 0xc001aab600?)
        /workspace/gitea/modules/markup/html.go:611 +0x20c
code.gitea.io/gitea/modules/markup.textNode(...)
        /workspace/gitea/modules/markup/html.go:434
code.gitea.io/gitea/modules/markup.visitNode(0xc001a6ab40, {0x26a1dc0, 0xb, 0xb}, {0x26a1dc0?, 0x7fb84c27a080?, 0x0?}, 0xc0002f6150)
        /workspace/gitea/modules/markup/html.go:377 +0x18d
code.gitea.io/gitea/modules/markup.visitNode(0xc001a6ab40, {0x26a1dc0, 0xb, 0xb}, {0x26a1dc0?, 0xc000285948?, 0xc0002859b0?}, 0xc0002f60e0)
        /workspace/gitea/modules/markup/html.go:424 +0x1f8
code.gitea.io/gitea/modules/markup.visitNode(0xc001a6ab40, {0x26a1dc0, 0xb, 0xb}, {0x26a1dc0?, 0xc000285b40?, 0x47f545?}, 0xc0002f1f80)
        /workspace/gitea/modules/markup/html.go:424 +0x1f8
code.gitea.io/gitea/modules/markup.visitNode(0xc001a6ab40, {0x26a1dc0, 0xb, 0xb}, {0x26a1dc0?, 0xee17d3?, 0xdf1?}, 0xc0002f1650)
        /workspace/gitea/modules/markup/html.go:424 +0x1f8
code.gitea.io/gitea/modules/markup.visitNode(0xc001a6ab40, {0x26a1dc0, 0xb, 0xb}, {0x26a1dc0?, 0x6?, 0x6?}, 0xc0002f1570)
        /workspace/gitea/modules/markup/html.go:424 +0x1f8
code.gitea.io/gitea/modules/markup.postProcess(0xc001a6ab40, {0x26a1dc0, 0xb, 0xb}, {0x19fb020?, 0xc001a47010?}, {0x19fb040, 0xc001a47028})
        /workspace/gitea/modules/markup/html.go:331 +0x24c
code.gitea.io/gitea/modules/markup.PostProcess(...)
        /workspace/gitea/modules/markup/html.go:186
code.gitea.io/gitea/modules/markup.render.func4()
        /workspace/gitea/modules/markup/renderer.go:238 +0x118
created by code.gitea.io/gitea/modules/markup.render
        /workspace/gitea/modules/markup/renderer.go:236 +0x673

Is there any way to make sure db.DefaultContext is actually initialized? I'm testing with go test -timeout 30s -tags 'sqlite sqlite_unlock_notify' code.gitea.io/gitea/modules/markup/markdown -failfast -v -run ^TestTotal_RenderWiki$. I tried adding assert.NoError(t, unittest.PrepareTestDatabase()) to the beginning of the test but it makes no difference.

[yardenshoham]

I figured out how to initialize the database

[wxiaoguang]

Share my thoughts: although I do not think it's good to make modules/markup depend on modals/db and modals/user, but to make the dependency clear it needs some more (maybe a lot) work to split the packages.

So I am neutral for this approach if other maintainers feel good of it.

[silverwind]

I do not think it's good to make modules/markup depend on modals/db and modals/user

Maybe just move the affected function out into a new module (maybe modules/mention)?

[yardenshoham]

As soon as the new module is imported the dependency would still exist, wouldn't it?

[wxiaoguang]

As soon as the new module is imported the dependency would still exist, wouldn't it?

Something like:

pkg.api.FuncInterface
pkg.impl1.Func1
pkg.impl2.Func2
pkg.markup.RenderWith(pkg.api.FuncInterface) -> call FuncInterface(...)
pkg.web.DoSomthing() -> pkg.markup.RenderWith(pkg.impl1.Func1)

Then there is no dependency between pkg.markup and pkg.impl1/pkg.impl2

That's what I meant "but to make the dependency clear it needs some more (maybe a lot) work to split the packages"

According the suggested dependency relation: https://docs.gitea.io/en-us/guidelines-backend/#package-dependencies , it's better not making modules depend on models

[yardenshoham]

There are many instances where modules depend on models

[wxiaoguang]

There are many instances where modules depend on models

Yup, that's just for historical reasons. The codebase was quite messy before, there are many refactoring PRs for splitting the packages. That's why the guideline came.

The root problem is that Golang's dependency management is quite strict and it can not bear cycle imports, so making the dependency clear would help a lot and make the codebase more healthy.

IIRC the consensus at the moment is "avoid importing models to modules if there is no strong reason", I am not sure how others think about it. So I just keep neutral for this approach.

[silverwind]

The root problem is that Golang's dependency management is quite strict and it can not bear cycle imports

That is certainly a benefit in my eyes that the compiler is so strict. I've dealt with untangling circular dependencies in JS before, it's absolutely no fun :)

[wxiaoguang]

I proposed a refactoring for this PR (some test code are removed because they do not work with the refactoring)

• refactor yardenshoham/gitea#19

It decouples the modules/markup and models/user, and you can use modules/context in services/markup/processorhelper now.

[wxiaoguang]

The root problem is that Golang's dependency management is quite strict and it can not bear cycle imports

That is certainly a benefit in my eyes that the compiler is so strict. I've dealt with untangling circular dependencies in JS before, it's absolutely no fun :)

Too strict is no fun either. It's very difficult to refactor existing code. Even if you only tried to move a single function, the cycle dependency problem occurs.

IMO only lazy language like Go has such problem. JS is still friendly to cycle imports and it's useful for some intermediate refactoring steps. All other modern languages are quite friendly to cycle imports.

[silverwind]

All other modern languages are quite friendly to cycle imports.

Cyclic imports in JS can lead to some nasty bugs depending on which module evaluates first. We forbid them here via linter rule as well, I think it's a net positive to not allow them.

[yardenshoham]

I'm working on fixing the tests

[wxiaoguang]

Some questions about the tests:

• Why a new user user34 is added? I didn't see where it is really used.
• Does "markup/markdown/markdown_test.go" really need to prepare database? Now there is no access to database from the markdown module.
• The same question as above for "api/v1/misc/main_test.go" and "api/v1/misc/markdown_test.go"
• The test in "integration/setting_test.go" seems unrelated, should it be reverted?

[yardenshoham]

Some questions about the tests:

• Why a new user user34 is added? I didn't see where it is really used.
• Does "markup/markdown/markdown_test.go" really need to prepare database? Now there is no access to database from the markdown module.
• The same question as above for "api/v1/misc/main_test.go" and "api/v1/misc/markdown_test.go"

TestTotal_RenderWiki and TestAPI_RenderGFM mention the user @r-lyeh

• The tests in "integration/setting_test.go" seems unrelated, could it be reverted?

They fail because user4 is not on explore's first page anymore

[wxiaoguang]

Some questions about the tests:

• Why a new user user34 is added? I didn't see where it is really used.
• Does "markup/markdown/markdown_test.go" really need to prepare database? Now there is no access to database from the markdown module.
• The same question as above for "api/v1/misc/main_test.go" and "api/v1/misc/markdown_test.go"

TestTotal_RenderWiki and TestAPI_RenderGFM mention the user @r-lyeh

• The tests in "integration/setting_test.go" seems unrelated, could it be reverted?

They fail because user4 is not on explore's first page anymore

How about fix other tests, assume the user r-lyeh doesn't exist, instead of adding more fixtures?

IMO the mention render could be tested separately.

[yardenshoham]

OK, I'm on it

[wxiaoguang]

I added a separate processorhelper_test.go to test IsUsernameMentionable with database.

And make markdown_test.go use mocked IsUsernameMentionable. Then no need to match the usernames during markdown_test.

[yardenshoham]

TestTotal_RenderWiki and TestAPI_RenderGFM pass for you @wxiaoguang?

[wxiaoguang]

No problem, I will take the remaining work.

[wxiaoguang]

CI passes