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

Using UploadRequest and DownloadRequest for FileBox after Dec 31, 2022 #185

Closed
huan opened this issue Aug 18, 2022 · 11 comments
Closed

Using UploadRequest and DownloadRequest for FileBox after Dec 31, 2022 #185

huan opened this issue Aug 18, 2022 · 11 comments
Labels
breaking Breaking changes

Comments

@huan
Copy link
Member

huan commented Aug 18, 2022

We will remove three gRPC interfaces after Dec 31, 2022:

  1. MessageFileStream
  2. MessageImageStream
  3. MessageSendFileStream

service Puppet {
/**
* Deprecated: will be removed after Dec 31, 2022
*/
rpc MessageFileStream (puppet.MessageFileStreamRequest) returns (stream puppet.MessageFileStreamResponse) {}
rpc MessageImageStream (puppet.MessageImageStreamRequest) returns (stream puppet.MessageImageStreamResponse) {}
rpc MessageSendFileStream (stream puppet.MessageSendFileStreamRequest) returns (puppet.MessageSendFileStreamResponse) {}

The removal of them is because we need to simplify the design and make it more robust and scalable with our infrastructure.

The New Way

gRPC

We are using the below four new abstracted interfaces UploadRequest, UploadResponse, DownloadRequest, and DownloadResponse in the future, for simplification.

message UploadRequest {
bytes chunk = 1;
}
message UploadResponse {
string id = 1;
}
message DownloadRequest {
string id = 1;
}
message DownloadResponse {
bytes chunk = 1;
}

gRPC Related Code

DownloadRequest

A DownloadRequest can convert a UUID to Stream:

https://github.com/wechaty/puppet-service/blob/ef72d99747c037cad48e532c0797737adffc0c2b/src/file-box-helper/uuidify-file-box-grpc.ts#L27-L35

UploadRequest

A UploadRequest can convert a Stream to UUID:

https://github.com/wechaty/puppet-service/blob/ef72d99747c037cad48e532c0797737adffc0c2b/src/file-box-helper/uuidify-file-box-grpc.ts#L44-L59

Using UUID type of FileBox

  1. If the fileBox is one of type Url, QRCode, Uuid, etc., then it can be serialized by fileBox.toString()
  2. If the fileBox is one of types Stream, Buffer, File, etc, then it needs to be converted to type Uuid before serialized by fileBox.toString()

Check if the FileBox should be converted to UUID

https://github.com/wechaty/puppet-service/blob/ef72d99747c037cad48e532c0797737adffc0c2b/src/server/puppet-implementation.ts#L83-L93

Convert a FileBox from Stream (etc.) types to UUID type

https://github.com/wechaty/puppet-service/blob/ef72d99747c037cad48e532c0797737adffc0c2b/src/file-box-helper/normalize-filebox.ts#L72-L79

CC @zpaimon

@huan huan added the breaking Breaking changes label Aug 18, 2022
@zpaimon
Copy link

zpaimon commented Aug 19, 2022

Why not use protobuf to define FileBox ?
FileBox must be implemented in multi-language,so I suggest that we should define FileBox in proto file.

@zpaimon
Copy link

zpaimon commented Aug 19, 2022

wechaty:1.20.2

contact?.say(fileBox) still call MessageSendFileStream !

@huan

@huan
Copy link
Member Author

huan commented Aug 19, 2022

Thanks for the suggestion and question, @zpaimon !

For your first question about defiling a FileBox in protobuf; You can refer to the below links first to get some background:

To brief short:

  1. We use a stringified JSON string in gRPC proto with the name file_box
  2. We use a Download & Upload proto for the Request & Response for the streaming data

The reason we use the above design is that when we are using gRPC stream API, it's hard to pass parameters. (refer to #96 )

For your second question, please learn more about our new UUID FileBox design from this PR:

And the current code base of the wechaty-puppet-service will do that:

  1. Try the deprecated gRPC call: if it works, return the data. This is for the compatible
  2. If the deprecated gRPC call does not work, use the new design.

You can read the source code about the above logic, and please feel free to let me know if you have more questions.

@zpaimon
Copy link

zpaimon commented Aug 19, 2022

If the deprecated gRPC call does not work, use the new design.

How to force wechaty to use new gRPC design?

If I don't implement MessageStream method,it will throw Method wechaty.Puppet/MessageSendFileStream is unimplemented.

@huan
Copy link
Member Author

huan commented Aug 19, 2022

That's a good question.

We can think about the below two solutions:

  1. Find the code in wechaty-puppet-service and try {} catch () {} the unimplemented error, which can make it to use the new way;
  2. for a workaround, you can implement a Method wechaty.Puppet/MessageSendFileStream and always throw an error? (We can remove the deprecated compatible code after Dec 31, 2022)

@zpaimon
Copy link

zpaimon commented Aug 19, 2022

Thank you very much! I make it work under my wework protocol project(code:marbas).

@huan
Copy link
Member Author

huan commented Aug 19, 2022

That's awesome; great to know that!

Do you have any plan for publishing the marbas to the community? I'm looking forward to that.

@zpaimon
Copy link

zpaimon commented Aug 20, 2022

I'm working hard at marbas.I will push alpha version in the recent days.

@zpaimon zpaimon closed this as not planned Won't fix, can't repro, duplicate, stale Aug 20, 2022
@zpaimon
Copy link

zpaimon commented Aug 21, 2022

@huan who can help me to test marbas?

@huan
Copy link
Member Author

huan commented Aug 21, 2022

My suggestion is to post an alpha testing announcement in our blog post (or GitHub issue), then tell our developers in the WeChat contributors & developers' home group to let them contact you with the alpha testing (distribute marbas TOKEN to them)

Feel free to let me know if you need any help from my side.

@1061168609
Copy link

The download does not respond when the download exceeds 90K

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking changes
Projects
None yet
Development

No branches or pull requests

3 participants