Skip to content
This repository has been archived by the owner on Nov 18, 2021. It is now read-only.

Protobuf to cue mapping for int64 #606

Closed
harveyxia opened this issue Dec 4, 2020 · 9 comments
Closed

Protobuf to cue mapping for int64 #606

harveyxia opened this issue Dec 4, 2020 · 9 comments
Labels
Accepted FeatureRequest New feature or request good first issue Good for newcomers protobuf

Comments

@harveyxia
Copy link

harveyxia commented Dec 4, 2020

Is your feature request related to a problem? Please describe.
As described by the documentation for the mapping from protobuf to CUE states, proto definitions are mapped to CUE following the Protobuf to JSON mapping. However, int64, fixed64, and uint64 are mapped to numeric types, whereas the protobuf to json mapping maps these types to string.

For context, we're using CUE to generate openapi schemas for k8s validation schema, and our protobuf to json serializers map 64 bit numeric types to strings, which conflicts with the CUE generated openapi schema that's expecting a numeric type.

Describe the solution you'd like
Update the protobuf to CUE mapping such that 64 bit numeric types map to string.

Additional context
If there are reasons why we want to map proto int64's to CUE int64's (which I'm assuming there are, the primary one being that the proto to cue mapping shouldn't be concerned with the eccentricities of the proto to json mapping), another option might be to use openapi's mixed types to represent 64 bit numerics as either string or 64 numeric types when converting proto to openapi.

@harveyxia harveyxia added the FeatureRequest New feature or request label Dec 4, 2020
@mpvl
Copy link
Contributor

mpvl commented Dec 4, 2020

Interesting. Note that the protobuf to json mapping doc does mention it accepts both. But I see how the resulting CUE API does not correctly reflect what is accepted.

It would indeed be possible to convert it to int64 | string optionally constraining the string to be a valid JSON string. This probably would then also have to be done for int32, as it mentions it accepts both there too.

I can imagine this will give some problems for some as not all OpenAPI dialects support mixed types. Although I'm not a fan of options, would an option of some sort work for you? Do you have control over the .proto files? Are you using CUE's API or tool?

@verdverm
Copy link
Contributor

verdverm commented Dec 4, 2020

I've noticed the options idea, by way of importing the k8s go code to Cue, means I can't directly validate my imported yaml without changes to the Cue. The result has an IntOrString type which is a struct. So now, even if you have an int or a string, you have to change it to be in a nested struct, which seems off and I personally find annoying.

I describe some examples in more detail here: https://cuetorials.com/starting-out/import-configuration/#importing-go-to-cue

This is an example of the generated Cue from Go which has this IntOrString struct: https://github.com/hofstadter-io/c8s/blob/main/cue.mod/gen/k8s.io/api/apps/v1/types_go_gen.cue#L360

@harveyxia
Copy link
Author

@mpvl Thanks for the quick response.

We do indeed have control over the .proto files and we are open to using proto options if that's what you meant by "option". However, this approach would also need to account for imported proto definitions, which we do not necessarily have control over.

We are using cuelang's API, as shown here.

@mpvl
Copy link
Contributor

mpvl commented Dec 7, 2020

In that case it probably makes sense to make it an option in the API first and see how that works out. That would allow control over imported protos too.

Sticking to the JSON standard is a good goal, but this to string conversion is too unfortunate, I would say.

Probably it makes sense to define arbitrary mappings at some point, but for now a JSON profile option would be good. I kinda wished we had adopted the "function option" pattern here.

To allow future extendibility, I could imagine having a different Profile type set as a field in Config that just has a JSON variant now. That way we don't have to think about the option set too much now.

@mpvl mpvl added Accepted good first issue Good for newcomers labels Dec 7, 2020
@harveyxia
Copy link
Author

The approach of using a JSON profile option sounds good to me! I'm happy to help contribute, though I'll need some guidance on where to make these changes as I'm not familiar with the project topology.

@mpvl
Copy link
Contributor

mpvl commented Dec 14, 2020

The changes should be solely in encoding/protobuf. The bulk of the mapping is done in parse.go and the Config type is in protobuf.go. Thanks!

@mpvl mpvl added the protobuf label Feb 18, 2021
@mpvl
Copy link
Contributor

mpvl commented Mar 26, 2021

I'm currently working on text proto support.

I'm deploying a technique there where, to parse, I pass a cue value as a schema that is used to interpret the text proto.

I realized the same thing could be done for JSON. This would allow selectively interpreting JSON input strings as numbers.

One problem with a broad option in the JSON converter is that one can't tell from just looking at a string with a number whether or not to parse it as a number. One needs to know the destination field.

Would that approach work?

@mpvl
Copy link
Contributor

mpvl commented Mar 31, 2021

In the end I'm going with an implementation that takes an ast.Expr or ast.File and, given a cue.Value modifies it according to the PB interpretation of JSON.

cueckoo pushed a commit that referenced this issue Mar 31, 2021
This allows code that uses the CUE API to modify an ast.Expr
or ast.File to conform to a CUE schema, allowing mappings
that Protobuf allows, but that are otherwise not allowed by
a strict interpretation of the schema.

Note that this assumes that enum integers can be mapped
to strings with a corresponding #intValue field. This is not
yet set by the proto mapping.

Issue #606

Change-Id: I71d7bfa9e69f985c1eaaf1c1e20e5a473b882e70
cueckoo pushed a commit that referenced this issue Apr 1, 2021
This allows code that uses the CUE API to modify an ast.Expr
or ast.File to conform to a CUE schema, allowing mappings
that Protobuf allows, but that are otherwise not allowed by
a strict interpretation of the schema.

Note that this assumes that enum integers can be mapped
to strings with a corresponding #intValue field. This is not
yet set by the proto mapping.

Issue #606

Change-Id: I71d7bfa9e69f985c1eaaf1c1e20e5a473b882e70
cueckoo pushed a commit that referenced this issue Apr 1, 2021
This allows code that uses the CUE API to modify an ast.Expr
or ast.File to conform to a CUE schema, allowing mappings
that Protobuf allows, but that are otherwise not allowed by
a strict interpretation of the schema.

Note that this assumes that enum integers can be mapped
to strings with a corresponding #intValue field. This is not
yet set by the proto mapping.

Issue #606

Change-Id: I71d7bfa9e69f985c1eaaf1c1e20e5a473b882e70
cueckoo pushed a commit that referenced this issue Apr 2, 2021
This allows code that uses the CUE API to modify an ast.Expr
or ast.File to conform to a CUE schema, allowing mappings
that Protobuf allows, but that are otherwise not allowed by
a strict interpretation of the schema.

Note that this assumes that enum integers can be mapped
to strings with a corresponding #intValue field. This is not
yet set by the proto mapping.

Issue #606

Change-Id: I71d7bfa9e69f985c1eaaf1c1e20e5a473b882e70
cueckoo pushed a commit that referenced this issue Apr 2, 2021
This allows code that uses the CUE API to modify an ast.Expr
or ast.File to conform to a CUE schema, allowing mappings
that Protobuf allows, but that are otherwise not allowed by
a strict interpretation of the schema.

Note that this assumes that enum integers can be mapped
to strings with a corresponding #intValue field. This is not
yet set by the proto mapping.

Issue #606

Change-Id: I71d7bfa9e69f985c1eaaf1c1e20e5a473b882e70
cueckoo pushed a commit that referenced this issue Apr 2, 2021
This allows code that uses the CUE API to modify an ast.Expr
or ast.File to conform to a CUE schema, allowing mappings
that Protobuf allows, but that are otherwise not allowed by
a strict interpretation of the schema.

Note that this assumes that enum integers can be mapped
to strings with a corresponding #intValue field. This is not
yet set by the proto mapping.

Issue #606

Change-Id: I71d7bfa9e69f985c1eaaf1c1e20e5a473b882e70
Reviewed-on: https://cue-review.googlesource.com/c/cue/+/9243
Reviewed-by: CUE cueckoo <cueckoo@gmail.com>
Reviewed-by: Marcel van Lohuizen <mpvl@golang.org>
cueckoo pushed a commit that referenced this issue Apr 9, 2021
Issue #606

Change-Id: I3bf6cbc1ecd5e83c94b5a7d97c845a35f4b84878
cueckoo pushed a commit that referenced this issue Apr 9, 2021
Closes #606

Change-Id: I712ed6f9fc37d53b633a44ed87e4c71c1e018b0f
cueckoo pushed a commit that referenced this issue Apr 12, 2021
Issue #606

Change-Id: I3bf6cbc1ecd5e83c94b5a7d97c845a35f4b84878
cueckoo pushed a commit that referenced this issue Apr 12, 2021
Closes #606

Change-Id: I712ed6f9fc37d53b633a44ed87e4c71c1e018b0f
cueckoo pushed a commit that referenced this issue Apr 12, 2021
Issue #606

Change-Id: I3bf6cbc1ecd5e83c94b5a7d97c845a35f4b84878
cueckoo pushed a commit that referenced this issue Apr 12, 2021
Closes #606

Change-Id: I712ed6f9fc37d53b633a44ed87e4c71c1e018b0f
cueckoo pushed a commit that referenced this issue Apr 15, 2021
Issue #606

Change-Id: I3bf6cbc1ecd5e83c94b5a7d97c845a35f4b84878
cueckoo pushed a commit that referenced this issue Apr 15, 2021
Closes #606

Change-Id: I712ed6f9fc37d53b633a44ed87e4c71c1e018b0f
cueckoo pushed a commit that referenced this issue Apr 15, 2021
Issue #606

Change-Id: I3bf6cbc1ecd5e83c94b5a7d97c845a35f4b84878
cueckoo pushed a commit that referenced this issue Apr 15, 2021
Closes #606

Change-Id: I712ed6f9fc37d53b633a44ed87e4c71c1e018b0f
cueckoo pushed a commit that referenced this issue Apr 15, 2021
Issue #606

Change-Id: I3bf6cbc1ecd5e83c94b5a7d97c845a35f4b84878
Reviewed-on: https://cue-review.googlesource.com/c/cue/+/9371
Reviewed-by: CUE cueckoo <cueckoo@gmail.com>
Reviewed-by: Marcel van Lohuizen <mpvl@golang.org>
cueckoo pushed a commit that referenced this issue Apr 15, 2021
Closes #606

Change-Id: I712ed6f9fc37d53b633a44ed87e4c71c1e018b0f
@cueckoo
Copy link

cueckoo commented Jul 3, 2021

This issue has been migrated to cue-lang/cue#606.

For more details about CUE's migration to a new home, please see cue-lang/cue#1078.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Accepted FeatureRequest New feature or request good first issue Good for newcomers protobuf
Projects
None yet
Development

No branches or pull requests

4 participants