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

目录直接相关的 API #234

Merged
merged 28 commits into from
Oct 24, 2022
Merged

目录直接相关的 API #234

merged 28 commits into from
Oct 24, 2022

Conversation

RanKKI
Copy link
Member

@RanKKI RanKKI commented Oct 8, 2022

v0.Post("/indices", req.JSON, h.NeedLogin, h.NewIndex)
v0.Put("/indices/:id", req.JSON, h.NeedLogin, h.UpdateIndex)
// v0.Delete("/indices/:id", h.NeedLogin, h.DeleteIndex) 该 PR 不实现该方法,相关代码已删除

v0.Post("/indices/:id/subjects", req.JSON, h.NeedLogin, h.AddIndexSubject)
v0.Put("/indices/:id/subjects/:subject_id", req.JSON, h.NeedLogin, h.UpdateIndexSubject)
v0.Delete("/indices/:id/subjects/:subject_id", h.NeedLogin, h.RemoveIndexSubject) 

@netlify
Copy link

netlify bot commented Oct 8, 2022

Deploy Preview for bangumi-org-server ready!

Name Link
🔨 Latest commit 6ce4fa6
🔍 Latest deploy log https://app.netlify.com/sites/bangumi-org-server/deploys/635676879337b400086976dd
😎 Deploy Preview https://deploy-preview-234--bangumi-org-server.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@RanKKI RanKKI marked this pull request as ready for review October 8, 2022 12:28
@codecov
Copy link

codecov bot commented Oct 8, 2022

Codecov Report

Base: 49.55% // Head: 51.07% // Increases project coverage by +1.52% 🎉

Coverage data is based on head (6ce4fa6) compared to base (4f2558d).
Patch coverage: 72.95% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #234      +/-   ##
==========================================
+ Coverage   49.55%   51.07%   +1.52%     
==========================================
  Files         198      204       +6     
  Lines        7804     8153     +349     
==========================================
+ Hits         3867     4164     +297     
- Misses       3480     3507      +27     
- Partials      457      482      +25     
Impacted Files Coverage Δ
internal/model/subject.go 44.44% <ø> (ø)
internal/model/timeline.go 32.35% <ø> (+32.35%) ⬆️
internal/web/handler/comments.go 11.04% <0.00%> (ø)
main.go 0.00% <ø> (ø)
internal/ctrl/get_index.go 34.48% <34.48%> (ø)
internal/pkg/generic/conv.go 40.00% <40.00%> (ø)
internal/web/handler/index/subject.go 53.57% <53.57%> (ø)
internal/web/handler/index/index.go 43.06% <68.91%> (ø)
internal/index/mysql_repository.go 70.00% <76.36%> (+15.58%) ⬆️
internal/ctrl/update_subject_collection.go 68.14% <82.50%> (+6.04%) ⬆️
... and 17 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@trim21
Copy link
Contributor

trim21 commented Oct 12, 2022

这个PR是可以review了吗?改一下openapi文件?

internal/web/res/index.go Outdated Show resolved Hide resolved
@RanKKI
Copy link
Member Author

RanKKI commented Oct 12, 2022

这个PR是可以review了吗?改一下openapi文件?

不好意思,这两天在忙别的, 先 review 代码吧, openapi 我今天晚点补上

internal/model/index.go Outdated Show resolved Hide resolved
internal/web/handler.go Outdated Show resolved Hide resolved
@trim21
Copy link
Contributor

trim21 commented Oct 15, 2022

看下slack私聊

openapi/v0.yaml Outdated Show resolved Hide resolved
internal/web/handler/index.go Outdated Show resolved Hide resolved
internal/web/handler/index.go Show resolved Hide resolved
internal/web/handler/index.go Outdated Show resolved Hide resolved
internal/web/handler/index.go Outdated Show resolved Hide resolved
internal/web/handler/index.go Outdated Show resolved Hide resolved
internal/web/handler/index.go Outdated Show resolved Hide resolved
Comment: s.Comment,
Subject: sub,
}
}

return results, nil
}

func (r mysqlRepo) AddOrUpdateIndexSubject(
Copy link
Contributor

Choose a reason for hiding this comment

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

这个是不是应该包在一个事务里

Copy link
Member Author

Choose a reason for hiding this comment

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

不需要吧,前面都是查询, 后面要么改或者增,没必要用事务

但我觉得这要加锁,避免创建两份目录

Copy link
Contributor

Choose a reason for hiding this comment

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

不需要吧,前面都是查询, 后面要么改或者增,没必要用事务

但我觉得这要加锁,避免创建两份目录

同时发两个请求的话有可能添加两个条目进目录吧,或者因为有unique key(没看sql表怎么定义的)而出错

Copy link
Member Author

Choose a reason for hiding this comment

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

嗯, 用事务也没法解决。
感觉对于这种创建资源的 还是应该是一定时间内阻止一样的请求 #233

Copy link
Member Author

Choose a reason for hiding this comment

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

这个要不我写个中间件 drop 请求吧.. 没啥别的好的想法解决这个问题了

@trim21
Copy link
Contributor

trim21 commented Oct 15, 2022

subject.ConvertDao 本来是没想导出的,ListSubjects应该在controller上面做的。

(在我其他地方是这么写的,但不是很确定是不是最好的办法,会不会太过繁琐了一点。)

@trim21
Copy link
Contributor

trim21 commented Oct 15, 2022

咨询一下,

CREATE TABLE
  IF NOT EXISTS `chii_index_related` (
    `idx_rlt_cat` TINYINT(3) NOT NULL,
    `idx_rlt_type` SMALLINT(6) UNSIGNED NOT NULL COMMENT '关联条目类型',
  ) ENGINE = MyISAM DEFAULT CHARSET = utf8 COMMENT = '目录关联表';

这里的 cat 我没理解错应该是 category,是目录里筛选条目类型的东西,但 type 是干这事的.. 是弃用的字段么

这个type是条目类型,cat不知道是什么,可能只是我们没用到。

@RanKKI
Copy link
Member Author

RanKKI commented Oct 16, 2022

这个type是条目类型,cat不知道是什么,可能只是我们没用到。

我看了下代码,之前是用 cat 做的分类,没有见到过 type 字段。

昨天还发现一点问题, ListSubjects 接口的 type 不支持传入 personcharacter 两个值

@trim21
Copy link
Contributor

trim21 commented Oct 16, 2022

这个type是条目类型,cat不知道是什么,可能只是我们没用到。

我看了下代码,之前是用 cat 做的分类,没有见到过 type 字段。

昨天还发现一点问题, ListSubjects 接口的 type 不支持传入 personcharacter 两个值

我突然发现忘了做角色和人物的相关功能了,你有兴趣补一下吗(

@RanKKI
Copy link
Member Author

RanKKI commented Oct 17, 2022

我先把目录的补完吧,
期末了要准备 Final 了,时间不是很多,结束如果没有人搞 我可以来整。

@trim21
Copy link
Contributor

trim21 commented Oct 17, 2022

我先把目录的补完吧, 期末了要准备 Final 了,时间不是很多,结束如果没有人搞 我可以来整。

OK

@trim21
Copy link
Contributor

trim21 commented Oct 17, 2022

合并一下主线(

@trim21
Copy link
Contributor

trim21 commented Oct 21, 2022

lint没过

@RanKKI
Copy link
Member Author

RanKKI commented Oct 21, 2022 via email

@trim21
Copy link
Contributor

trim21 commented Oct 21, 2022

需要把 func (ctrl Ctrl) GetIndexWithCache(c context.Context, id uint32) (res.Index, bool, error) { 改成 func (ctl Ctrl) GetIndexWithCache(c context.Context, id uint32) (res.Index, bool, error) {

@trim21
Copy link
Contributor

trim21 commented Oct 21, 2022

报错是报在了其他文件,但是是这个PR引起的

@RanKKI
Copy link
Member Author

RanKKI commented Oct 22, 2022

报错是报在了其他文件,但是是这个PR引起的

发现了.. 我这报错在 internal/ctrl/count_episode_for_subject.go:30:17: 但是是因为 index.go...
改好了。

@trim21
Copy link
Contributor

trim21 commented Oct 24, 2022

sai老板还在摸,再等等(

@trim21
Copy link
Contributor

trim21 commented Oct 24, 2022

sai老板给权限了(

和了一个别的PR,有个冲突


indexSubject, err := r.q.IndexSubject.WithContext(ctx).
Where(r.q.IndexSubject.IndexID.Eq(id), r.q.IndexSubject.SubjectID.Eq(uint32(subjectID))).
FirstOrInit()
Copy link
Contributor

Choose a reason for hiding this comment

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

FirstOrInit还会返回gorm.ErrRecordNotFound吗?

Copy link
Contributor

@trim21 trim21 Oct 24, 2022

Choose a reason for hiding this comment

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

而且这里Init了,indexSubject.ID还是0吗。

Copy link
Member Author

Choose a reason for hiding this comment

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

FirstOrInit还会返回gorm.ErrRecordNotFound吗?

不会,确实判断的有点多余了

而且这里Init了,indexSubject.ID还是0吗。

这里主要是初始化对象,使 indexSubject 不会是 nil, 然后里面的数据都是默认值, Int 默认就是 0

Copy link
Contributor

Choose a reason for hiding this comment

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

那下面convertDao失败的话岂不是会留一条错误纪录在里面...

Copy link
Member Author

Choose a reason for hiding this comment

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

咦,convertDao 在 firstOrInit 上面呀。

如果 convert 失败了, 错误就直接出去了。

Copy link
Contributor

Choose a reason for hiding this comment

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

啊 确实,看错了

@trim21 trim21 merged commit c1ee878 into bangumi:master Oct 24, 2022
@RanKKI RanKKI deleted the feat/indices_part_a branch November 10, 2022 11:11
@RanKKI RanKKI mentioned this pull request Nov 10, 2022
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants