Protobuf domains #280

tegefaulkes · 2021-10-29T01:42:50Z

Description

This PR addresses changes relating to adding subdomains to the proto definitions and importing useful google.protobuf types.

The new directory structure looks like this now. when using a message from a domain in a rpc you import it with import "domains/vaults.proto" and refer to the messages with domains.vaults.Vault.

schemas/
├── agent.proto
├── client.proto
├── domains
│   ├── common.proto
│   ├── gestalts.proto
│   ├── identities.proto
│   ├── keys.proto
│   ├── nodes.proto
│   ├── notifications.proto
│   ├── permissions.proto
│   ├── secrets.proto
│   ├── sessions.proto
│   └── vaults.proto
└── test.proto

Issues Fixed

Fixes Creating .proto definition subdomains. #279
Relates to Transport Agnostic RPC #249

Tasks

- Prototype by abstracting into subdirectories under src/proto/schemas, and integrate google's protobuf library by first referencing it and then just copying verbatim
- - work out how to import google.protobuf messages.
- - examine google.protobuf.Any usage.
- - examine timestamp usage.
- - examine struct usage.
- Create common domain types and common message types and share between Client.proto and Agent.proto

Final checklist

tegefaulkes · 2021-10-29T01:50:37Z

Right now I have the message subdomains in the same directory as the Agent.proto and Client.proto in proto/schema. Do we want to move them into their own sub directory like proto/schema/domains

Do we want to keep the agent and client specific messages separate. group them like vaultsAgent.proto, vaultsCommon.proto and vaultsClient.proto?

so we want to split the services into the domains as well? Currently the clientService is defined in Client.proto it could be split into the domain proto files such as the vaults rpc definitions in the vaults.proto.

scripts/proto-generate.sh

src/agent/GRPCClientAgent.ts

src/agent/agentPB.ts

CMCDragonkai · 2021-11-01T04:45:41Z

Can you only copy in the google protos that we are actually using? For the google protos that we are not using, please delete them.

Also on this MR in the description, you should describe the new structure. Use tree src/proto/schemas and copy the output to the MR description. Makes it easier to show context.

Also give some annotations explaining why you did what you did. As in put them where.

src/proto/schemas/domains/Permissions.proto

CMCDragonkai · 2021-11-01T05:03:50Z

Yea that's fine, but I wouldn't export this from `src/agent/index.ts`. The `src/agent/index.ts` and `src/client/index.ts` should only export what's in the local directory.

…

On 11/1/21 4:00 PM, Brian Botha wrote: ***@***.**** commented on this pull request. ------------------------------------------------------------------------ In src/agent/agentPB.ts <#280 (comment)>: > @@ -0,0 +1,4 @@ +export * as common from '../proto/js/domains/Common_pb'; It's just to make it clearer in the code when using the messages. I wanted to replicate the 'name spacing' so in the proto if the message was defined in vaults EG |vaults.Vault| then we'd refer to it in the code as |messages.vaults.Vault|. It also allows us to import the messages from one place. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#280 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAE4OHJWZEO77M5ONHRAEGLUJYNGDANCNFSM5G6GQRIQ>.

tegefaulkes · 2021-11-01T05:15:43Z

Oh, so you're saying I should import the agentPb directly instead of through the index.ts?

CMCDragonkai · 2021-11-01T08:49:40Z

Ok so I see what you did here...

├── google
│   └── protobuf
│       ├── any.proto
│       ├── api.proto
│       ├── descriptor.proto
│       ├── duration.proto
│       ├── empty.proto
│       ├── field_mask.proto
│       ├── struct.proto
│       ├── timestamp.proto
│       ├── type.proto
│       └── wrappers.proto

When we start using these, we can copy just the ones we want to use.

Which are all copied from: https://github.com/protocolbuffers/protobuf/tree/master/src/google/protobuf

So atm, none of them are being used, so we can push that to later increment of the #275.

CMCDragonkai · 2021-11-01T08:53:46Z

I like the new structure. Now we can clearly see that src/proto/schemas/agent.proto and src/proto/schemas/client.proto are used for the services. However I would want to change the service name to AgentService instead of just Agent. This follows the style guide and why is given here: https://docs.buf.build/lint/rules/#service_suffix mainly to ensure no overlap in naming between message types and service types.

CMCDragonkai · 2021-11-01T08:55:27Z

I'm going to change it to agent_service.proto and AgentService correspondingly.

The package is supposed to have the version specifiers as per the #249. Let me check what they should be.

CMCDragonkai · 2021-11-01T08:58:35Z

I noticed that domains.common.EchoMessage can be used by TestService.

Also because we are regenerating the code, we should be deleting the src/proto/js so that the old files aren't left there.

CMCDragonkai · 2021-11-01T09:04:47Z

From #249 (comment)

Because the package name is meant to follow the directory structure. It would mean that schemas should have a sub directory. Maybe something like:

agent/v1/agent_service.proto
client/v1/client_service.proto

Of course that would imply that we have 2 different versions. So instead we might do something like:

polykey/v1/agent_service.proto
polykey/v1/client_service.proto
polykey/v1/test_service.proto

And instead of domains, I reckon we can use types as the directory since they are just different types.
That would ensures that our package name is polykey.v1

CMCDragonkai · 2021-11-01T09:07:42Z

Also the package name should be the directory, but not contain the name of the file. This follows golang packaging/file hierarchy.

CMCDragonkai · 2021-11-01T09:09:43Z

This means all proto files part of the same package is part of the same namespace. So when referring to them, you just use types.EchoMessage and types.VaultMessage instead of types.vaults.VaultMessage or types.common.EchoMessage. Again this is standard in golang where protobuf is part of the same ecosystem.

CMCDragonkai · 2021-11-01T09:14:08Z

Note that imports are all like this now:

import "polykey/v1/types/nodes.proto";
import "polykey/v1/types/identities.proto";

Which means one has to refer to them using the full package name: polykey.v1.types.Node.

CMCDragonkai · 2021-11-01T09:15:22Z

Oh it seems you can use the same types within the same package. That means you don't really need to import right?

CMCDragonkai · 2021-11-01T09:28:42Z

I see that the List was reused twice as a message name. That's why you created separate packages for each file. But according to the style guide, the packages are meant to be different directories. I reckon here it's better to differentiate the 2 list types and not conflict in the same namespace. So all proto files within the same package are in the same namespace and package corresponds to directory name here.

CMCDragonkai · 2021-11-01T09:35:12Z

From a naming standard pov, the messages are more freeform. However specific kinds of messages should have Request and Response suffix. See: https://docs.buf.build/lint/rules/#rpc_request_standard_name-rpc_response_standard_name-rpc_request_response_unique But I'll leave this out for now.

Right now I need to ensure that all our types are not conflicting with each other.

CMCDragonkai · 2021-11-01T09:36:51Z

So we'll drop the Message suffix. But we will need to add wrapper types for Request and Response suffixes.

CMCDragonkai · 2021-11-01T09:40:45Z

Ok to preserve what @tegefaulkes did, I will instead do something like:

schemas/polykey/v1/agent_service.proto
schemas/polykey/v1/client_service.proto
schemas/polykey/v1/test_service.proto
schemas/polykey/v1/vaults/vaults.proto
schemas/polykey/v1/secrets/secrets.proto

This means we have a package like polykey.v1.secrets.Rename. Then the directory here means the protofiles.

That way it expands more cleanly and less need to to write the prefix/suffix within each file.

CMCDragonkai · 2021-11-01T12:06:24Z

Updated to:

»» ~/Projects/js-polykey/src/proto
 ♖ tree .              ⚡(protobuf_domains) pts/2 23:05:40
.
├── js
│   └── polykey
│       └── v1
│           ├── agent_service_grpc_pb.d.ts
│           ├── agent_service_grpc_pb.js
│           ├── agent_service_pb.d.ts
│           ├── agent_service_pb.js
│           ├── client_service_grpc_pb.d.ts
│           ├── client_service_grpc_pb.js
│           ├── client_service_pb.d.ts
│           ├── client_service_pb.js
│           ├── gestalts
│           │   ├── gestalts_grpc_pb.js
│           │   ├── gestalts_pb.d.ts
│           │   └── gestalts_pb.js
│           ├── identities
│           │   ├── identities_grpc_pb.js
│           │   ├── identities_pb.d.ts
│           │   └── identities_pb.js
│           ├── keys
│           │   ├── keys_grpc_pb.js
│           │   ├── keys_pb.d.ts
│           │   └── keys_pb.js
│           ├── nodes
│           │   ├── nodes_grpc_pb.js
│           │   ├── nodes_pb.d.ts
│           │   └── nodes_pb.js
│           ├── notifications
│           │   ├── notifications_grpc_pb.js
│           │   ├── notifications_pb.d.ts
│           │   └── notifications_pb.js
│           ├── permissions
│           │   ├── permissions_grpc_pb.js
│           │   ├── permissions_pb.d.ts
│           │   └── permissions_pb.js
│           ├── secrets
│           │   ├── secrets_grpc_pb.js
│           │   ├── secrets_pb.d.ts
│           │   └── secrets_pb.js
│           ├── sessions
│           │   ├── sessions_grpc_pb.js
│           │   ├── sessions_pb.d.ts
│           │   └── sessions_pb.js
│           ├── test_service_grpc_pb.d.ts
│           ├── test_service_grpc_pb.js
│           ├── test_service_pb.d.ts
│           ├── test_service_pb.js
│           ├── utils
│           │   ├── utils_grpc_pb.js
│           │   ├── utils_pb.d.ts
│           │   └── utils_pb.js
│           └── vaults
│               ├── vaults_grpc_pb.js
│               ├── vaults_pb.d.ts
│               └── vaults_pb.js
└── schemas
    └── polykey
        └── v1
            ├── agent_service.proto
            ├── client_service.proto
            ├── gestalts
            │   └── gestalts.proto
            ├── identities
            │   └── identities.proto
            ├── keys
            │   └── keys.proto
            ├── nodes
            │   └── nodes.proto
            ├── notifications
            │   └── notifications.proto
            ├── permissions
            │   └── permissions.proto
            ├── secrets
            │   └── secrets.proto
            ├── sessions
            │   └── sessions.proto
            ├── test_service.proto
            ├── utils
            │   └── utils.proto
            └── vaults
                └── vaults.proto

26 directories, 55 files

CMCDragonkai · 2021-11-01T12:08:39Z

The usage of polykey/v1 means that if a breaking change occurs, we will create polykey/v2 and always the new proto files there. Only changes need to be created there, but the service files will always need to be copied over. But files that aren't changed, they can be kept the same.

For example if permissions.proto is breaking change, then we would have:

src/schemas/polykey/v2/permissions/permissions.proto
src/schemas/polykey/v2/agent_service.proto
src/schemas/polykey/v2/client_service.proto
src/schemas/polykey/v2/test_service.proto

But of course we should limit such changes, and try to make things as backwards compatible as possible.

CMCDragonkai · 2021-11-01T12:16:56Z

If I copy back in the google proto. There's actually some missing proto files. The original globbing wasn't working. So with the new globstar it is now working, so there's some extra google proto to bring in.

By removing the types.proto and api.proto this solves the problems such that it works now. So for now I'm leaving:

»» ~/Projects/js-polykey/src/proto/schemas/google/protobuf
 ♜ tree .                                   ⚡(protobuf_domains) pts/2 23:16:00
.
├── any.proto
├── descriptor.proto
├── duration.proto
├── empty.proto
├── field_mask.proto
├── struct.proto
├── timestamp.proto
└── wrappers.proto

0 directories, 8 files

In case we need it for the future completion of #249.

CMCDragonkai · 2021-11-01T12:17:42Z

Now to update all the src code with the proper imports.

…t for `google.protobuf` message types. Fixes (#279)

CMCDragonkai · 2021-11-01T13:48:06Z

All code swapped over. Ran lintfix.

The last thing to do is to run tests.

If the relevant tests pass for this, then merging time.

All other MRs need to rebase on top. @emmacasolin can you help rebase the CLI retry MR https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/213 cause this makes some big changes to the import paths.

@scottmmorris you'll have to rebase for #266.

CMCDragonkai · 2021-11-01T13:52:41Z

All tests pass. Except of course for nodes being dealt with here #274.

tegefaulkes marked this pull request as draft October 29, 2021 01:44

tegefaulkes mentioned this pull request Oct 29, 2021

GRPC API Review Changes #275

Closed

24 tasks

tegefaulkes force-pushed the protobuf_domains branch 2 times, most recently from 75a3627 to 5e25cd9 Compare October 29, 2021 03:45

tegefaulkes marked this pull request as ready for review October 29, 2021 06:41

tegefaulkes requested a review from CMCDragonkai October 29, 2021 06:41