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

Protobuf specification rework #131

Merged
merged 17 commits into from
Feb 2, 2024
Merged

Protobuf specification rework #131

merged 17 commits into from
Feb 2, 2024

Conversation

Baccata
Copy link
Contributor

@Baccata Baccata commented Jan 25, 2024

Rework the protobuf specification.

The goal is to allow for more control over what should be wrapped in messages and what should not be wrapped, avoiding implicit rules that prevent sound refactoring/evolutions of smithy specifications.

In particular :

  1. addition of a alloy.proto#protoWrapped trait that can be added on simple shapes + lists + maps + members targeting those. This is the only signal allowed for deciding whether something should be wrapped or not.
  2. addition of a number of validation rules for smithy shapes that belong to a transitive closure of a gRPC or protoEnabled shape. These rules ported from the proto language, such as (but not limited to) :
    • proto oneof members cannot be repeated/map fields => smithy members of unions targeting map/list shapes MUST have @protoWrapped
    • proto maps cannot have messages => smithy map keys MUST NOT have @protoWrapped
    • when @protoIndex are present on enum/int enum members, one of the value MUST BE 0
    • when @protoIndex is absent on intEnum members, one of its enum values must be 0
    • etc etc
  3. created an alloy-protobuf artifact with proto files containing messages that can be used by smithy-translate and other tools to facilitate the abidance to the rules described in the alloy protobuf specification

This PR is to remain a draft until the semantics described in it are incorporated in smithy-translate and validated in some "smithy4s-protobuf" POC

use alloy#uuidFormat
use alloy.proto#protoCompactUUID

@protoCompactUUID
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm aware. We shouldn't use shapeIds to discriminate behaviour, it's pretty unsound. Using traits is much better.

I guess your point is that we could apply that trait to the alloy#UUID idea to retain the current behaviour.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One potential downside of applying it to alloy#UUID is then the alloy core namespace would depend on the proto one which I'm not sure we want (maybe fine though)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not know you were aware, I agree with you that the trait is a better approach now that I know better!
But as jeff points out, I don't think introducing a dependency between the two artifacts is desired so I would just leave it like that

build.sc Show resolved Hide resolved
use alloy#uuidFormat
use alloy.proto#protoCompactUUID

@protoCompactUUID
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One potential downside of applying it to alloy#UUID is then the alloy core namespace would depend on the proto one which I'm not sure we want (maybe fine though)

docs/serialisation/protobuf.md Outdated Show resolved Hide resolved
Co-authored-by: David Francoeur <dfrancoeur04@gmail.com>
@Baccata Baccata marked this pull request as ready for review January 31, 2024 09:43
Baccata and others added 4 commits February 1, 2024 13:50
Co-authored-by: David Francoeur <dfrancoeur04@gmail.com>
Co-authored-by: David Francoeur <dfrancoeur04@gmail.com>
Co-authored-by: David Francoeur <dfrancoeur04@gmail.com>
}

test(
"int-enum - no-zero on open enums is valid if protoIndex(0) is present"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized this test title mentions OpenEnumTrait but it's not used in the code, so either the title is wrong, of the annotation was forgotten on the intEnum member

@Baccata Baccata merged commit e37c296 into main Feb 2, 2024
3 checks passed
@Baccata Baccata deleted the protobuf-spec-rework branch February 2, 2024 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants