chore: add pbandk protobuf (AR-1303) #301

vitorhugods · 2022-03-18T17:36:35Z

PR Submission Checklist for internal contributors

The PR Title
- conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
- contains a reference JIRA issue number like SQPIT-764
- answers the question: If merged, this PR will: ... ³
The PR Description
- is free of optional paragraphs and you have filled the relevant parts to the best of your ability

What's new in this PR?

Issues

We need a multiplatform protobuf solution that covers all our usecases

Solutions

Add PBandK.

How it is setup:
Two new modules:

protobuf-codegen which is a "Java Library" that includes the .proto files. It will call Google's Gradle Protobuf plugin to generate the code, with the PBandK add-on.
protobuf which is a bit of a hack, as Google's Protobuf plugin doesn't work with Kotlin Multiplatform yet. This module depends on protobuf-codegen and will trigger a code generation there, adding the generated files to the commonMain SourceSet. This module also contains the PBandK in commonMain, which allows all targets to encode and decode to/from byte arrays.

The idea was taken from a sample in pbandk's repo. As suggested in this issue/discussion.

Testing

Add a simple test to serve as a sample and sanity check.

Notes

This current implementation replaces the current Protobuf utilization to encode/decode messages, but doesn't replace the creation of envelopes yet. This should be trivial and tackled in a next PR to make this one smaller.
The 475+ lines of .proto files are copy/pasted from our generic-message-proto repo.
Both messages.proto and otr.proto contained a QualifiedUserID. So I've extracted it to its own dedicated file called qualified_user_id.proto.
Naming is an issue and we probably want to solve this before merging. Especially for ClientId and UserId, which can be misleading, making discoverability through autocompletion a bit annoying.

PR Post Merge Checklist for internal contributors

If any soft of configuration variable was introduced by this PR, it has been added to the relevant documents and the CI jobs have been updated.

References

https://sparkbox.com/foundry/semantic_commit_messages
https://github.com/wireapp/.github#usage
E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

…being used to create envelopes

vitorhugods · 2022-03-18T17:47:37Z

logic/src/commonMain/kotlin/com/wire/kalium/logic/data/message/ProtoContentMapper.kt

+        val content = when (val protoContent = genericMessage.content) {
+            is GenericMessage.Content.Text -> MessageContent.Text(protoContent.value.content)
+            is GenericMessage.Content.Asset -> MessageContent.Unknown
+            is GenericMessage.Content.Availability -> MessageContent.Unknown
+            is GenericMessage.Content.ButtonAction -> MessageContent.Unknown
+            is GenericMessage.Content.ButtonActionConfirmation -> MessageContent.Unknown
+            is GenericMessage.Content.Calling -> MessageContent.Unknown
+            is GenericMessage.Content.Cleared -> MessageContent.Unknown
+            is GenericMessage.Content.ClientAction -> MessageContent.Unknown
+            is GenericMessage.Content.Composite -> MessageContent.Unknown
+            is GenericMessage.Content.Confirmation -> MessageContent.Unknown
+            is GenericMessage.Content.DataTransfer -> MessageContent.Unknown
+            is GenericMessage.Content.Deleted -> MessageContent.Unknown
+            is GenericMessage.Content.Edited -> MessageContent.Unknown
+            is GenericMessage.Content.Ephemeral -> MessageContent.Unknown
+            is GenericMessage.Content.External -> MessageContent.Unknown
+            is GenericMessage.Content.Hidden -> MessageContent.Unknown
+            is GenericMessage.Content.Image -> MessageContent.Unknown
+            is GenericMessage.Content.Knock -> MessageContent.Unknown
+            is GenericMessage.Content.LastRead -> MessageContent.Unknown
+            is GenericMessage.Content.Location -> MessageContent.Unknown
+            is GenericMessage.Content.Reaction -> MessageContent.Unknown
+            null -> {
+                kaliumLogger.w("Null content when parsing protobuf. Message UUID = $genericMessage.")
+                MessageContent.Unknown
+            }
+        }


Exhaustive when for protobufs as the Content is a sealed class!

github-actions · 2022-03-18T18:09:32Z

Unit Test Results

  2 files   2 suites 18s ⏱️
59 tests 59 ✔️ 0 💤 0 ❌

Results for commit ab20aab.

♻️ This comment has been updated with latest results.

alexandreferris

Great work!

I just have two comments 🤔

build.gradle.kts

logic/src/commonTest/kotlin/com/wire/kalium/logic/data/message/ProtoContentMapperProvider.kt

buildSrc/src/main/java/Dependencies.kt

typfel · 2022-03-21T08:57:53Z

protobuf-codegen/src/main/proto/messages.proto

+
+import "qualified_user_id.proto";
+
+message GenericMessage {


The proto files are copied into the project now? This makes it hard to know which of the protobuf file we are using in Kalium.

Can we pull them GitHub only define the release tag in the project. If this is not easy we could do it in a separate PR but I think we should do it.

Not 100% copy pasted. As I mentioned in the PR description, the QualifiedUserID, for example, needed to be extracted to a common file.

Some of the Java options supported by Protoc (like outer class name or adding a suffix) are not respected when using PBandK.

I'd like to streamline the process and checkout from the otr-proto repo, but it's far from easy to do so right now. Maybe it would help by extracting the QualifiedUserID in the original otr-proto repo, but that would be a breaking change.

I don't have any solutions now.

The otr.proto and messages.proto are supposed to live in different namesakes doesn't PBandK respect that? See wireapp/generic-message-proto#74 (review)

So yeah, seems like streem/pbandk#179 doesn't respect package but also the option java_package is different so I don't understand why they clash.

Right so it clashes because package name inside the proto files are override by the plugin option:
option("kotlin_package=com.wire.kalium.protobuf")

TBH I felt like PBandK doesn't respect a lot of these options yet. I tried playing with them before submitting the PR and in the end I just removed them, as they didn't have any effect on the generated code.
I didn't want to leave package and options in there, so they would suddenly kick-in when we bump PBandK's version or someone spends time trying to understand these lines of code in the future.

It's a trade-off for now. We increase a bit of maintenance with .proto definitions, but as a pro: we have a multi-platform solution. In my opinion, it's worth it. But I'd like a second confirmation before we move on.

Hopefuly these will be supported Soon™.

Yes, it's definitely worth it! Just looking for way to avoid having to copy & paste the files and edit them. But like I said earlier I'm not pushing for solving it in this PR.

Right so it clashes because package name inside the proto files are override by the plugin option: option("kotlin_package=com.wire.kalium.protobuf")

Indeed, the kotlin_package was overwriting it 🤦🏼‍♂️. Removing that made the package work.

Still, the original messages.proto doesn't have a package. So we still need to modify the files a tiny bit when getting from the original repo. But the changes are one-liner.

This simplifies our process of getting proto definitions from otr-proto, and only requires changing the package name. Everything else is 100% the same.

vitorhugods added 4 commits March 18, 2022 09:55

chore: add pbandk protobuf generation

cb3dc34

chore: replace current protobufs with PBandK

52a90e7

chore: add proguard rules for protobufs

80b5802

chore: add deprecated warnings on old protobuf data while it's still …

c0a1004

…being used to create envelopes

vitorhugods requested review from typfel, gongracr, alexandreferris, MohamadJaara and mchenani March 18, 2022 17:36

Merge branch 'develop' into chore/add-pbandk-protobuf

e887ee7

vitorhugods commented Mar 18, 2022

View reviewed changes

alexandreferris reviewed Mar 21, 2022

View reviewed changes

build.gradle.kts Outdated Show resolved Hide resolved

logic/src/commonTest/kotlin/com/wire/kalium/logic/data/message/ProtoContentMapperProvider.kt Outdated Show resolved Hide resolved

typfel requested changes Mar 21, 2022

View reviewed changes

vitorhugods added 4 commits March 21, 2022 21:08

test: use Mocha for JS testing

3e84cea

style: extract plugin versions

c5c0ca9

refactor: remove unused file

b998739

Merge branch 'develop' into chore/add-pbandk-protobuf

f00c58f

vitorhugods requested review from typfel and alexandreferris March 21, 2022 21:09

vitorhugods added 2 commits March 21, 2022 22:10

Merge branch 'develop' into chore/add-pbandk-protobuf

c5c525c

Merge branch 'develop' into chore/add-pbandk-protobuf

d544b96

typfel approved these changes Mar 22, 2022

View reviewed changes

alexandreferris approved these changes Mar 22, 2022

View reviewed changes

vitorhugods added 2 commits March 22, 2022 10:45

refactor: use package names in proto files

110322f

This simplifies our process of getting proto definitions from otr-proto, and only requires changing the package name. Everything else is 100% the same.

Merge branch 'develop' into chore/add-pbandk-protobuf

ab20aab

vitorhugods merged commit e4c6315 into develop Mar 22, 2022

vitorhugods deleted the chore/add-pbandk-protobuf branch March 22, 2022 10:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: add pbandk protobuf (AR-1303) #301

chore: add pbandk protobuf (AR-1303) #301

vitorhugods commented Mar 18, 2022 •

edited

Loading

vitorhugods Mar 18, 2022 •

edited

Loading

github-actions bot commented Mar 18, 2022 •

edited

Loading

alexandreferris left a comment

typfel Mar 21, 2022

vitorhugods Mar 21, 2022

typfel Mar 22, 2022

typfel Mar 22, 2022 •

edited

Loading

typfel Mar 22, 2022

vitorhugods Mar 22, 2022

typfel Mar 22, 2022

vitorhugods Mar 22, 2022


		import "qualified_user_id.proto";

		message GenericMessage {

chore: add pbandk protobuf (AR-1303) #301

chore: add pbandk protobuf (AR-1303) #301

Conversation

vitorhugods commented Mar 18, 2022 • edited Loading

PR Submission Checklist for internal contributors

What's new in this PR?

Issues

Solutions

Testing

Notes

PR Post Merge Checklist for internal contributors

References

vitorhugods Mar 18, 2022 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Mar 18, 2022 • edited Loading

Unit Test Results

alexandreferris left a comment

Choose a reason for hiding this comment

typfel Mar 21, 2022

Choose a reason for hiding this comment

vitorhugods Mar 21, 2022

Choose a reason for hiding this comment

typfel Mar 22, 2022

Choose a reason for hiding this comment

typfel Mar 22, 2022 • edited Loading

Choose a reason for hiding this comment

typfel Mar 22, 2022

Choose a reason for hiding this comment

vitorhugods Mar 22, 2022

Choose a reason for hiding this comment

typfel Mar 22, 2022

Choose a reason for hiding this comment

vitorhugods Mar 22, 2022

Choose a reason for hiding this comment

vitorhugods commented Mar 18, 2022 •

edited

Loading

vitorhugods Mar 18, 2022 •

edited

Loading

github-actions bot commented Mar 18, 2022 •

edited

Loading

typfel Mar 22, 2022 •

edited

Loading