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

Enhance service operation #139

Closed

Conversation

AbigailJixiangyuyu
Copy link
Contributor

@AbigailJixiangyuyu AbigailJixiangyuyu commented Mar 11, 2024

Link to an issue

What

  • 解决了 CommentDomain 没有 初始化 InitSnowFlake 导致的空指针
  • 解决了 Favorite 没有初始化 errs 导致的空指针
  • 移除了 api.proto 中无用的 token 参数
  • 返回前端的 int64 类型转为 string

How

Screenshots

How to test

Checklist

  • Link to an issue if it really related.
  • At least describe what this PR does.
  • Use rebase to confirm that current branch doesn't conflict with main branch.
  • Unit tests. At least do not reduce the unit test coverage.
  • Checked and updated guidelines.md which used to describe how to build, deploy and use DouTok.

# resolve some problem caused by nil value
# remove some useless param especially token
# convert int64 to string when returning to frontend
Copy link
Owner

@TremblingV5 TremblingV5 left a comment

Choose a reason for hiding this comment

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

Comment Domain重构问题

Comment Domain是我之前已经重构过的一个模块,建议这部分改动可以提issue或者discussion说一下具体想法,然后再看怎样落实

Token相关

当前DouTok还没有独立的前端,所以希望还是能保持和字节提供的客户端的兼容性

user_id解析

建议依然保留从token中解析user_id的方式,并在设置user_id到上下文中时直接设置成int64类型,毕竟go的代码里的处理均是针对int64的

其他

还是建议不同的功能尽可能分到多个PR里去

proto/api.proto Outdated
User user =2; // 评论用户信息
string content = 3; // 评论内容
string create_date = 4; // 评论发布日期,格式 mm-dd
int64 like_count = 5; // 该评论点赞数
int64 tease_count = 6; // 该评论diss数
int64 like_count = 5 [(api.body) = "id,string"]; // 该评论点赞数
Copy link
Owner

Choose a reason for hiding this comment

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

这里api.body中标明的字段名应该是不正确的吧,应该不是id

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我的错

proto/api.proto Outdated
int64 like_count = 5; // 该评论点赞数
int64 tease_count = 6; // 该评论diss数
int64 like_count = 5 [(api.body) = "id,string"]; // 该评论点赞数
int64 tease_count = 6 [(api.body) = "id,string"]; // 该评论diss数
Copy link
Owner

Choose a reason for hiding this comment

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

同上

proto/api.proto Outdated
@@ -289,8 +286,7 @@ message DouyinCommentActionResponse {
}

message DouyinCommentListRequest {
string token = 1; // 用户鉴权token
Copy link
Owner

Choose a reason for hiding this comment

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

这里的token参数建议依然保留,DouTok现在还没有完全脱离字节提供的手机客户端,尽量还是维持能有一个客户端正常使用吧

proto/api.proto Outdated
int64 video_id = 1; // 视频id
int32 action_type = 2; // 1-发布评论,2-删除评论
string comment_text = 3; // 用户填写的评论内容,在action_type=1的时候使用
int64 comment_id = 4; // 要删除的评论id,在action_type=2的时候使用
Copy link
Owner

Choose a reason for hiding this comment

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

这里貌似也需要类似[(api.body) = "id,string"]的标注

Copy link
Owner

Choose a reason for hiding this comment

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

@AbigailJixiangyuyu 这里需要确认

proto/api.proto Outdated
@@ -81,7 +81,7 @@ message DouyinRelationActionResponse {
}

message DouyinRelationFollowListRequest {
int64 user_id = 1; // 用户id
int64 user_id = 1 [(api.body) = "id,string"]; // 用户id
Copy link
Owner

Choose a reason for hiding this comment

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

这里api.body中标明的字段名应该是不正确的吧,应该不是id

commentCountCache CommentCntCache // key: video id, value: modification if count of comments for a video
commentTotalCountCache CommentTotalCountCache // key: video id, value: count of comments for a video
snowflakeHandle *utils.SnowflakeHandle
type CommentDomainUtil struct {
Copy link
Owner

Choose a reason for hiding this comment

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

请问这里为何要重命名为Util,如果没有什么特殊原因建议保持原来的命名

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个是因为之前的命名会导致两个不同的结构体使用相同的名字,然后使用上就出错了。就是上面提到的snakeflake的空指针异常

Copy link
Owner

Choose a reason for hiding this comment

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

如果相同名称的结构体不在同一个命名空间下,根据命名空间控制就可以了

commentTotalCountCache CommentTotalCountCache // key: video id, value: count of comments for a video
snowflakeHandle *utils.SnowflakeHandle
type CommentDomainUtil struct {
CommentRepository commentRepository
Copy link
Owner

Choose a reason for hiding this comment

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

这里所有的成员应该都只是Service内部去调用,应该不需要全部对外暴漏

@@ -14,42 +14,43 @@ import (

var (
CommentNotBelongToUser = errors.New("comment not belong to user")
DomainUtil *CommentDomainUtil
Copy link
Owner

Choose a reason for hiding this comment

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

这里之前ServiceHandler的一个成员,这里建议继续维持在Handler里比较好吧,稍微OOP一些

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这部分就是因为OOP导致业务逻辑都出错了,得仔细看看是怎样的逻辑

Copy link
Owner

Choose a reason for hiding this comment

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

业务逻辑出错是指snowflake的空指针吗

}

// TODO 整个 CommentDomain 需要重构,代码结构和其他服务相比很不协调
Copy link
Owner

Choose a reason for hiding this comment

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

其实我想说Comment Domain是重构的结果,虽然Go本身并不一定要求OOP,但是这套组织结构是尽可能考虑OOP的一些做法的集合,参考于我之前实习的地方,感觉还是可以的。

@@ -34,7 +33,11 @@ func CommentAction(ctx context.Context, c *app.RequestContext) {
return
}

userId := int64(jwt.ExtractClaims(ctx, c)[initialize.AuthMiddleware.IdentityKey].(float64))
userId, err := strconv.ParseInt(c.Keys["user_id"].(string), 10, 64)
Copy link
Owner

Choose a reason for hiding this comment

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

这里c.Keys["user_id"].(string)是有在什么地方去设置这个值嘛,我好像没太找到在哪设置

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
项目中身份校验的部分使用到了 github.com/hertz-contrib/jwt 这个包,在包内部实现了改功能

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个包是项目中本来就使用到的,不是我加的

Copy link
Owner

Choose a reason for hiding this comment

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

这个包是项目中本来就使用到的,不是我加的

好滴我之前没有找到这里来

@@ -16,7 +16,7 @@ func (s *CommentDomainHandler) AddComment(ctx context.Context, req *commentDomai
}, ParametersError
}

result, err := s.service.AddComment(ctx, req.VideoId, req.UserId, utils.GetSnowFlakeId().Int64(), 0, 0, req.CommentText)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

其实主要问题是这里,调用utils.GetSnowFlakeId()会导致空指针异常,因为初始化的snowflake并没有保存在这里,而是其他地方

Copy link
Owner

Choose a reason for hiding this comment

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

原本Service结构体里有一个SnowFlake的Handler,用那个应该就可以了。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

但是由于使用了接口无法调用到

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
这里是通过这个接口接收的,导致无法使用原本Service里的内容

Copy link
Owner

Choose a reason for hiding this comment

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

但是由于使用了接口无法调用到

在结构体的成员方法里去调用应该是不受影响的。

@@ -16,7 +16,7 @@ func (s *CommentDomainHandler) AddComment(ctx context.Context, req *commentDomai
}, ParametersError
}

result, err := s.service.AddComment(ctx, req.VideoId, req.UserId, utils.GetSnowFlakeId().Int64(), 0, 0, req.CommentText)
result, err := service.DomainUtil.AddComment(ctx, req.VideoId, req.UserId, service.DomainUtil.SnowflakeHandle.GetId().Int64(), 0, 0, req.CommentText)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

实际上是在这里,所以我才把SnowflakeHandle这些字段改为大写,让外部可以调用到

Copy link
Owner

Choose a reason for hiding this comment

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

这里可以看两种情况

  1. Snowflake给出的ID一定要handler控制,将snowflake handler上移给handler
  2. 如果不需要,则减少AddComment的参数,在service内部去给出一个snowflake id

@AbigailJixiangyuyu
Copy link
Contributor Author

commentDomain我还原回去了,换个个方案来处理 utils.GetSnowFlakeId() 导致的空指针异常

@@ -70,9 +71,12 @@ func CommentList(ctx context.Context, c *app.RequestContext) {
handler.SendResponse(c, handler.BuildCommendListResp(errno.ErrBind))
return
}

videoId, err := strconv.ParseInt(req.VideoId, 10, 64)
Copy link
Owner

Choose a reason for hiding this comment

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

想确认下这里VideoId变成string类型是因为proto中添加了video_id,string这样的tag导致的吗,感觉在golang范围内都是可以当int64处理的,如果总是需要类型转换不太好

Copy link
Contributor Author

Choose a reason for hiding this comment

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

前端传过来的不用,我改下,我也觉得很难处理,js里面没有int64类型,都是用float保存了,我也不知道怎么处理好一些

type Config struct {
}

func (c *Config) GetNodeCode() int32 {
Copy link
Owner

Choose a reason for hiding this comment

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

@AbigailJixiangyuyu 这里需要确认

proto/api.proto Outdated
@@ -180,8 +180,8 @@ message DouyinPublishActionResponse {
}

message DouyinPublishListRequest {
int64 user_id = 1; // 用户id
string token = 2; // 用户鉴权token
// int64 user_id = 1 [(api.body) = "id,string"]; // 用户id
Copy link
Owner

Choose a reason for hiding this comment

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

@AbigailJixiangyuyu 这里需要确认

proto/api.proto Outdated
int64 video_id = 1; // 视频id
int32 action_type = 2; // 1-发布评论,2-删除评论
string comment_text = 3; // 用户填写的评论内容,在action_type=1的时候使用
int64 comment_id = 4; // 要删除的评论id,在action_type=2的时候使用
Copy link
Owner

Choose a reason for hiding this comment

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

@AbigailJixiangyuyu 这里需要确认

@AbigailJixiangyuyu
Copy link
Contributor Author

err.config 那部分只是为了先让他不报错,不然有空指针

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