Skip to content

Proposal: generic *.Result types#3036

Merged
tonistiigi merged 2 commits into
moby:masterfrom
jedevc:generic-results
Aug 25, 2022
Merged

Proposal: generic *.Result types#3036
tonistiigi merged 2 commits into
moby:masterfrom
jedevc:generic-results

Conversation

@jedevc
Copy link
Copy Markdown
Member

@jedevc jedevc commented Aug 16, 2022

This PR requires #2935 - to see only the commits that differ, see here.

Summary

The new generic Result struct in this package is intended to replace the various Result and related Attestation structs across the codebase, including:

  • frontend.Result
  • client.Result/gateway.Result
  • exporter.Source

Motivation

These Results are all fundamentally the same, and share the same logic, with the only difference being the type used in the Ref/Refs/Attestations properties.

Since these were all separate, we easily encounter the following problems:

  • Repetition of struct and method declarations.

    Each separate Result type required it's own properties and methods to be redeclared. Sometimes these were consistent with each other, and other times they were not. Unifying all Results together ensures a lack of duplication, and means that changing the structure of the Result will not require extensive changes throughout every single declaration.

  • Complex conversion between Result types.

    At gateway and api boundaries, we are required to convert each Result into a Result of a different type. Previously, this has been a lengthly and error prone conversion, requiring knowledge of the exact structure of the Result, so as to allow traversal. By unifying Results, we can implement a ConvertResult function to convert from types of Result[A] to types of Result[B] given a function from A to B, which cleanly encapsulates the structure of the Result.

  • Complex conversion to the protobuf gateway api types.

    At the gateway api, we have to convert to pb.Results. Previously, we had to include logic to convert to and from both frontend.Result and client.Result. With a generic Result, we can simple convert to a Result[pb.Ref] and then have one single set of methods to convert Result[pb.Ref] to a pb.Result.

These problems have been an issue for some time, however, recently with the addition of attestations, the problem has grown, since the amount of logic required at conversion boundaries has massively increased.

How

This patch intends to simplify all of the above, and reduce line-count and code complexity by using the new-ish go generics features. Care has been taken to ensure that this patch is as small as possible, and affects as few Go APIs as possible. This is ensured by using type aliases for the Result type to re-create the various previous Results, e.g. frontend.Result/client.Result/exporter.Source, so that little code in packages that utilize these Results need to be changed. The only major changes take place at conversion boundaries, where, for the most part, the code is simplified.

One major caveat is that the result package is made significantly more complex, due to the generic code, and the need for reflection to perform comparisons of any types to nil, as interfaces such as solver.ResultProxy (used as a Ref type) do not implement the comparable interface, which is a limitation of the current go language spec (hopefully to be resolved in the future).

This is marked as draft for now, as the final implementation isn't fixed, or even certain, as I know that generics have been a controversial feature in the past. Thoughts welcome! 🎉

@jedevc
Copy link
Copy Markdown
Member Author

jedevc commented Aug 16, 2022

Looks like this might also depend on #3022 for the linter to support the updated version of Go.

Comment thread exporter/exporter.go Outdated
@tonistiigi
Copy link
Copy Markdown
Member

I'd like to first see (and potentially merge) the version that normalizes the refs to a single map inside Result. This should mean the generic types that this PR adds in

result.Attestation[T]
result.InTotoAttestation[T]

are not needed at all.

That would also simplify the proto layer that this builds on and where it is more important to make changes early because we can't do backward-incompatible changes.

@jedevc jedevc marked this pull request as ready for review August 24, 2022 13:50
@jedevc
Copy link
Copy Markdown
Member Author

jedevc commented Aug 24, 2022

Have rebased onto #3038, which means that the Attestation interface isn't generic anymore 🎉

The diff affects fewer lines now, though I still think it's a worthwhile patch to pull in.

@jedevc jedevc force-pushed the generic-results branch 2 times, most recently from ebab938 to 916e202 Compare August 24, 2022 14:57
jedevc added 2 commits August 24, 2022 16:28
Signed-off-by: Justin Chadwell <me@jedevc.com>
The Result struct in this package is intended to replace the various
Result structs across the codebase, including:

- frontend.Result
- client.Result/gateway.Result
- exporter.Source

These Results are all fundamentally the same, and share the same logic,
with the only difference being the type used in the Ref/Refs properties.

Since these were all separate, we easily encounter the following
problems:

- Repetition of struct and method declarations.

  Each separate Result type required it's own properties and methods to
  be redeclared. Sometimes these were consistent with each other, and
  other times they were not. Unifying all Results together ensures a
  lack of duplication, and means that changing the structure of the Result
  will not require extensive changes throughout every single declaration.

- Complex conversion between Result types.

  At gateway and api boundaries, we are required to convert each Result
  into a Result of a different type. Previously, this has been a lengthly
  and error prone conversion, requiring knowledge of the exact structure
  of the Result, so as to allow traversal. By unifying Results, we can
  implement a ConvertResult function to convert from types of Result[A]
  to types of Result[B] given a function from A to B, which cleanly
  encapsulates the structure of the Result.

- Complex conversion to the protobuf gateway api types.

  At the gateway api, we have to convert to pb.Results. Previously, we
  had to include logic to convert to and from both frontend.Result and
  client.Result. With a generic Result, we can simple convert to a
  Result[pb.Ref] and then have one single set of methods to convert
  Result[pb.Ref] to a pb.Result.

These problems have been an issue for some time, however, recently with
the addition of attestations, the problem has grown, since the amount of
logic required at conversion boundaries has massively increased.

This patch intends to simplify all of the above, and reduce line-count
and code complexity by using the new-ish go generics features. Care has
been taken to ensure that this patch is as small as possible, and
affects as few Go APIs as possible. This is ensured by using type aliases
for the Result type to re-create the various previous Results, e.g.
frontend.Result/client.Result/exporter.Source, so that little code in
packages that utilize these Results need to be changed. The only major
changes take place at conversion boundaries, where, for the most part,
the code is simplified.

One major caveat is that the result package is made significantly more
complex, due to the generic code, and the need for reflection to perform
comparisons of any types to nil, as interfaces such solver.ResultProxy
(used as a Ref type) do not implement the comparable interface, which is
a limitation of the current go language spec (hopefully to be resolved
in the future).

Signed-off-by: Justin Chadwell <me@jedevc.com>
@tonistiigi tonistiigi merged commit b8c253f into moby:master Aug 25, 2022
if err != nil {
return nil, err
}
if res.Refs != nil {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Woops, just noticed this little section during review of this, this block isn't needed anymore - I've opened a PR to clear it up in #3086, apologies 😄

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.

3 participants