slime-io / slime

An intelligent ServiceMesh manager based on Istio
https://slime-io.github.io/
Other
424 stars 78 forks source link

fix int64 marshal to string, use int32 to int #387

Closed zackzhangkai closed 1 year ago

zackzhangkai commented 1 year ago

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

https://github.com/grpc-ecosystem/grpc-gateway/issues/438

MouceL commented 1 year ago

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

zackzhangkai commented 1 year ago

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

Done. May you reivew again please?

MouceL commented 1 year ago

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

Done. May you reivew again please?

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

zackzhangkai commented 1 year ago

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 commented 1 year ago

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 ?

believening commented 1 year ago

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

zackzhangkai commented 1 year ago

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 commented 1 year ago

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

@zackzhangkai

believening commented 1 year ago

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

Not for solving fillInterval and fill_interval, but another idea for changing int64 to int32 to solve serialization problems.

zackzhangkai commented 1 year ago

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

Not for solving fillInterval and fill_interval, but another idea for changing int64 to int32 to solve serialization problems.

Both google.protobuf.Duration and Duration api defined by slime, had the same result. It would be serializated to string.

But slime pod would be crashed, for it requied int.

image

believening commented 1 year ago

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

Not for solving fillInterval and fill_interval, but another idea for changing int64 to int32 to solve serialization problems.

Both google.protobuf.Duration and Duration api defined by slime, had the same result. It would be serializated to string.

But slime pod would be crashed, for it requied int.

image

yes, google.protobuf.Duration would be serializated to string, as expected.

If we decide to use google.protobuf.Duration, it means we have to use string to configure related fields. In my opinion, this is more friendly to human reading. In addition, it can also be consistent with the envoy xds API without additional type conversion.

zackzhangkai commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

YonkaFang commented 1 year ago

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

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

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