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 int64 marshal to string, use int32 to int #387

Merged
merged 1 commit into from
Jun 19, 2023

Conversation

zackzhangkai
Copy link
Contributor

int64, uint64 to string instead of number.
unit32, float, doubles are marshaled as numbers.

grpc-ecosystem/grpc-gateway#438

@MouceL
Copy link
Contributor

MouceL commented Jun 9, 2023

7B23B23B-127B-47A5-9A44-926217C4ECC8

@zackzhangkai
Copy link
Contributor Author

7B23B23B-127B-47A5-9A44-926217C4ECC8

Done. May you reivew again please?

@MouceL
Copy link
Contributor

MouceL commented Jun 13, 2023

7B23B23B-127B-47A5-9A44-926217C4ECC8

Done. May you reivew again please?

here >> https://github.com/slime-io/slime/pull/387/files#r1223821553

Signed-off-by: zackzhangkai <zhangkaiamm@gmail.com>
@zackzhangkai
Copy link
Contributor Author

7B23B23B-127B-47A5-9A44-926217C4ECC8

Done. May you reivew again please?

here >> https://github.com/slime-io/slime/pull/387/files#r1223821553

Fix it. And code can run successfully locally.

@zackzhangkai zackzhangkai changed the title fix int64 marshal to string, use int32 to int [wip]fix int64 marshal to string, use int32 to int Jun 13, 2023
@zackzhangkai zackzhangkai marked this pull request as draft June 13, 2023 02:58
@zackzhangkai
Copy link
Contributor Author

smart_limit.yaml as follows:

spec:
  sets:
    _base:
      descriptor:
      - action:
          fill_interval:    # notice this is fill_interval
...

As pb.go :

type SmartLimitDescriptor_Action struct {
	Quota                string    `protobuf:"bytes,1,opt,name=quota,proto3" json:"quota,omitempty"`
	FillInterval         *Duration `protobuf:"bytes,2,opt,name=fill_interval,json=fillInterval,proto3" json:"fill_interval,omitempty"`

when use k8s client(controller-runtime) to apply , the yaml would be

    _base:
      descriptor:
      - action:
          fillInterval:    # notice this 
...

How to solve this ?

@zackzhangkai zackzhangkai changed the title [wip]fix int64 marshal to string, use int32 to int fix int64 marshal to string, use int32 to int Jun 13, 2023
@zackzhangkai zackzhangkai marked this pull request as ready for review June 13, 2023 03:31
@believening

This comment was marked as off-topic.

@zackzhangkai
Copy link
Contributor Author

Is it possible to use google.protobuf.Duration to replace the custom duration?

Has no effect , for I had tested it.

The may reason is the behavior of protoc:

(Refer to https://loesspie.com/2022/01/06/go-protobuf-custom-json-tag/ )

如果字段原本就是驼峰格式,那么默认情况下,protobuf中是不会额外出现json=xxx内容的
见字段 img / titleName / CountNum
如果字段不是驼峰格式,或者指定了 json_name,那么protobuf中会出现json=xxx内容
见字段 id / author_name
type Blog struct {
	state         protoimpl.MessageState
	sizeCache     protoimpl.SizeCache
	unknownFields protoimpl.UnknownFields

	Id         int64  `protobuf:"varint,1,opt,name=id,json=myid,proto3" json:"id,omitempty"`
	TitleName  string `protobuf:"bytes,2,opt,name=titleName,proto3" json:"titleName,omitempty"`
	AuthorName string `protobuf:"bytes,3,opt,name=author_name,json=authorName,proto3" json:"author_name,omitempty"`
	Img        string `protobuf:"bytes,4,opt,name=img,proto3" json:"img,omitempty"`
	CountNum   int64  `protobuf:"varint,5,opt,name=CountNum,proto3" json:"CountNum,omitempty"`
}
json序列化

import "github.com/golang/protobuf/jsonpb"

func main() {
	b := blog.Blog{Id: 42, TitleName: "nothing", AuthorName: "who"}

	m := jsonpb.Marshaler{
		OrigName:     false,
		EnumsAsInts:  false,
		EmitDefaults: false,
		Indent:       "",
		AnyResolver:  nil,
	}

	fmt.Println(m.MarshalToString(&b))
	// {"myid":"42","titleName":"nothing","authorName":"who"} <nil>

	// 使用原始的 protobuf 字段名
	m.OrigName = true
	fmt.Println(m.MarshalToString(&b))
	// {"id":"42","titleName":"nothing","author_name":"who"} <nil>

	// 零值字段输出
	m.EmitDefaults = true
	fmt.Println(m.MarshalToString(&b))
	// {"id":"42","titleName":"nothing","author_name":"who","img":"","CountNum":"0"} <nil>

	// 零值字段输出,但使用 protobuf 的 json tag
	m.OrigName = false
	fmt.Println(m.MarshalToString(&b))
	// {"myid":"42","titleName":"nothing","authorName":"who","img":"","CountNum":"0"} <nil>

	// 自定义缩进字符
	m.Indent = "|——"
	fmt.Println(m.MarshalToString(&b))
	// {
	// |——"id": "42",
	// |——"titleName": "nothing",
	// |——"author_name": "who",
	// |——"img": "",
	// |——"CountNum": "0"
	// } <nil>

	// 直接输出字符串
	fmt.Println(b.String())
	// id:42  titleName:"nothing"  author_name:"who"
}

@MouceL
Copy link
Contributor

MouceL commented Jun 13, 2023

4C45E8BD-1084-4B53-8BBE-048121F405F8

@zackzhangkai

@believening

This comment was marked as off-topic.

@zackzhangkai

This comment was marked as off-topic.

@believening

This comment was marked as off-topic.

@zackzhangkai
Copy link
Contributor Author

zackzhangkai commented Jun 19, 2023

The main concern is that it would be imcomtible with the old version for slime. Old version required int, new version required string. This is the main problem.

@believening
Copy link
Contributor

The main concern is that it would be imcomtible with the old version for slime. Old version required int, new version required string. This is the main problem.

I'm sorry that the discussion about Duration has affected the discussion history of the PR itself.
We can discuss in another thread whether it is worth making an API change to Duration and how to ensure compatibility.

@zackzhangkai
Copy link
Contributor Author

So what's the descision?It's miserable for the community, I wonder.

@YonkaFang
Copy link
Contributor

"protobuf JSON 映射标准要求将 in64 值作为带引号的字符串发出,以解决 JSON 标准的一个众所周知的复杂性,即除了 float64 类型提供的数字之外,无法保证或要求数字的准确性"

从对上层友好的角度,不必要的场景下改为int32更好。

源头来自 <- jsonpb规范 https://developers.google.com/protocol-buffers/docs/proto3#json <- json <- 浏览器支持

@YonkaFang YonkaFang merged commit 24c2f36 into slime-io:master Jun 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants