Add bufimage.ImageModuleDependencies (#2235)

private/bufpkg/bufimage/bufimage.go

@@ -87,6 +87,9 @@ type Image interface {
 	//
 	// This contains all files, including imports if available.
 	// The returned files are in correct DAG order.
+	//
+	// All files that have the same ModuleIdentity will also have the same commit, or no commit.
+	// This is enforced at construction time.
 	Files() []ImageFile
 	// GetFile gets the file for the root relative file path.
 	//
@@ -467,6 +470,98 @@ func ProtoImageToFileDescriptors(protoImage *imagev1.Image) []protodescriptor.Fi
 	return protoImageFilesToFileDescriptors(protoImage.File)
 }
 
+// ImageDependency is a dependency of an image.
+//
+// This could conceivably be part of ImageFile or bufmoduleref.FileInfo.
+// For ImageFile, this would be a field that is ignored when translated to proto,
+// and is calculated on creation from proto. IsImport would become ImportType.
+// You could go a step further and make this optionally part of the proto definition.
+//
+// You could even go down to bufmoduleref.FileInfo if you used the AST, but this
+// could be error prone.
+//
+// However, for simplicity now (and to not rewrite the whole codebase), we make
+// this a separate type that is calculated off of an Image after the fact.
+//
+// If this became part of ImageFile or bufmoduleref.FileInfo, you would get
+// all the ImageDependencies from the ImageFiles, and then sort | uniq them
+// to get the ImageDependencies for an Image. This would remove the requirement
+// of this associated type to have a ModuleIdentity and commit, so in
+// the IsDirect example  below, d.proto would not be "ignored" - it would
+// be an ImageFile like any other, with ImportType DIRECT.
+//
+// Note that if we ever do this, there is validation in newImage that enforces
+// that all ImageFiles with the same ModuleIdentity have the same commit. This
+// validation will likely have to be moved around.
+type ImageModuleDependency interface {
+	// String() returns remote/owner/repository[:commit].
+	fmt.Stringer
+
+	// Required. Will never be nil.
+	ModuleIdentity() bufmoduleref.ModuleIdentity
+	// Optional. May be empty.
+	Commit() string
+
+	// IsDirect returns true if the dependency is a direct dependency.
+	//
+	// A dependency is direct if it is only an import of non-imports in the image.
+	//
+	// Example:
+	//
+	//		a.proto, module buf.build/foo/a, is non-import, imports b.proto
+	//		b.proto, module buf.build/foo/b, is import, imports c.proto
+	//		c.proto, module buf.build/foo/c, is import
+	//
+	// In this case, the list would contain only buf.build/foo/b, as buf.build/foo/a
+	// for a.proto is a non-import, and buf.build/foo/c for c.proto is only imported
+	// by an import
+	IsDirect() bool
+
+	isImageModuleDependency()
+}
+
+// ImageModuleDependency returns all ImageModuleDependencies for the Image.
+//
+// Does not return any ImageModuleDependencies for non-imports, that is the
+// ModuleIdentities and commits represented by non-imports are not represented
+// in this list.
+func ImageModuleDependencies(image Image) []ImageModuleDependency {
+	importsOfNonImports := make(map[string]struct{})
+	for _, imageFile := range image.Files() {
+		if !imageFile.IsImport() {
+			for _, dependency := range imageFile.FileDescriptor().GetDependency() {
+				importsOfNonImports[dependency] = struct{}{}
+			}
+		}
+	}
+	// We know that all ImageFiles with the same ModuleIdentity
+	// have the same commit or no commit, so using String() will properly identify
+	// unique dependencies.
+	stringToImageModuleDependency := make(map[string]ImageModuleDependency)
+	for _, imageFile := range image.Files() {
+		if imageFile.IsImport() {
+			if moduleIdentity := imageFile.ModuleIdentity(); moduleIdentity != nil {
+				_, isDirect := importsOfNonImports[imageFile.Path()]
+				imageModuleDependency := newImageModuleDependency(
+					moduleIdentity,
+					imageFile.Commit(),
+					isDirect,
+				)
+				stringToImageModuleDependency[imageModuleDependency.String()] = imageModuleDependency
+			}
+		}
+	}
+	imageModuleDependencies := make([]ImageModuleDependency, 0, len(stringToImageModuleDependency))
+	for _, imageModuleDependency := range stringToImageModuleDependency {
+		imageModuleDependencies = append(
+			imageModuleDependencies,
+			imageModuleDependency,
+		)
+	}
+	sortImageModuleDependencies(imageModuleDependencies)
+	return imageModuleDependencies
+}
+
 type newImageForProtoOptions struct {
 	noReparse            bool
 	computeUnusedImports bool

private/bufpkg/bufimage/bufimagebuild/bufimagebuild.go

@@ -67,6 +67,12 @@ func WithExpectedDirectDependencies(expectedDirectDependencies []bufmoduleref.Mo
 }
 
 // WithWorkspace sets the workspace to be read from instead of ModuleReader, and to not warn imports for.
+//
+// TODO: this can probably be dealt with by finding out if an ImageFile has a commit
+// or not, although that is hacky, that's an implementation detail in practice, but perhaps
+// we could justify it - transitive dependencies without commits don't make sense?
+//
+// TODO: shouldn't buf.yamls in workspaces have deps properly declared in them anyways? Why not warn?
 func WithWorkspace(workspace bufmodule.Workspace) BuildOption {
 	return func(buildOptions *buildOptions) {
 		buildOptions.workspace = workspace

private/bufpkg/bufimage/bufimagebuild/builder.go

@@ -163,6 +163,9 @@ func (b *builder) build(
 // warnInvalidImports checks that all the target image files have valid imports statements that
 // point to files in the local module, in a direct dependency, or in a workspace local unnamed
 // module. It outputs WARN messages otherwise, one per invalid import statement.
+//
+// TODO: Understand this code before doing anything
+// TODO: switch to use bufimage.ImageModuleDependencies
 func (b *builder) warnInvalidImports(
 	ctx context.Context,
 	builtImage bufimage.Image,

private/bufpkg/bufimage/image.go

@@ -31,12 +31,24 @@ func newImage(files []ImageFile, reorder bool) (*image, error) {
 		return nil, errors.New("image contains no files")
 	}
 	pathToImageFile := make(map[string]ImageFile, len(files))
+	identityStringToCommit := make(map[string]string)
 	for _, file := range files {
 		path := file.Path()
 		if _, ok := pathToImageFile[path]; ok {
 			return nil, fmt.Errorf("duplicate file: %s", path)
 		}
 		pathToImageFile[path] = file
+		if moduleIdentity := file.ModuleIdentity(); moduleIdentity != nil {
+			identityString := moduleIdentity.IdentityString()
+			existingCommit, ok := identityStringToCommit[identityString]
+			if ok {
+				if existingCommit != file.Commit() {
+					return nil, fmt.Errorf("image had two different commits for the same module: %q and %q", existingCommit, file.Commit())
+				}
+			} else {
+				identityStringToCommit[identityString] = file.Commit()
+			}
+		}
 	}
 	if reorder {
 		files = orderImageFiles(files, pathToImageFile)

private/bufpkg/bufimage/image_module_dependency.go

@@ -0,0 +1,61 @@
+// Copyright 2020-2023 Buf Technologies, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package bufimage
+
+import (
+	"github.com/bufbuild/buf/private/bufpkg/bufmodule/bufmoduleref"
+)
+
+var _ ImageModuleDependency = &imageModuleDependency{}
+
+type imageModuleDependency struct {
+	moduleIdentity bufmoduleref.ModuleIdentity
+	commit         string
+	isDirect       bool
+}
+
+func newImageModuleDependency(
+	moduleIdentity bufmoduleref.ModuleIdentity,
+	commit string,
+	isDirect bool,
+) *imageModuleDependency {
+	return &imageModuleDependency{
+		moduleIdentity: moduleIdentity,
+		commit:         commit,
+		isDirect:       isDirect,
+	}
+}
+
+func (i *imageModuleDependency) ModuleIdentity() bufmoduleref.ModuleIdentity {
+	return i.moduleIdentity
+}
+
+func (i *imageModuleDependency) Commit() string {
+	return i.commit
+}
+
+func (i *imageModuleDependency) IsDirect() bool {
+	return i.isDirect
+}
+
+func (i *imageModuleDependency) String() string {
+	moduleIdentityString := i.moduleIdentity.IdentityString()
+	if i.commit != "" {
+		return moduleIdentityString + ":" + i.commit
+	}
+	return moduleIdentityString
+}
+
+func (*imageModuleDependency) isImageModuleDependency() {}

private/bufpkg/bufimage/util.go

@@ -17,6 +17,7 @@ package bufimage
 import (
 	"errors"
 	"fmt"
+	"sort"
 
 	"github.com/bufbuild/buf/private/bufpkg/bufmodule/bufmoduleref"
 	"github.com/bufbuild/buf/private/gen/data/datawkt"
@@ -499,3 +500,69 @@ func isFileToGenerate(
 	}
 	return true
 }
+
+func sortImageModuleDependencies(imageModuleDependencies []ImageModuleDependency) {
+	sort.Slice(imageModuleDependencies, func(i, j int) bool {
+		return imageModuleDependencyLess(imageModuleDependencies[i], imageModuleDependencies[j])
+	})
+}
+
+func imageModuleDependencyLess(a ImageModuleDependency, b ImageModuleDependency) bool {
+	return imageModuleDependencyCompareTo(a, b) < 0
+}
+
+// return -1 if less
+// return 1 if greater
+// return 0 if equal
+func imageModuleDependencyCompareTo(a ImageModuleDependency, b ImageModuleDependency) int {
+	if a == nil && b == nil {
+		return 0
+	}
+	if a == nil && b != nil {
+		return -1
+	}
+	if a != nil && b == nil {
+		return 1
+	}
+	aModuleIdentity := a.ModuleIdentity()
+	bModuleIdentity := b.ModuleIdentity()
+	if aModuleIdentity != nil || bModuleIdentity != nil {
+		if aModuleIdentity == nil && bModuleIdentity != nil {
+			return -1
+		}
+		if aModuleIdentity != nil && bModuleIdentity == nil {
+			return 1
+		}
+		if aModuleIdentity.Remote() < bModuleIdentity.Remote() {
+			return -1
+		}
+		if aModuleIdentity.Remote() > bModuleIdentity.Remote() {
+			return 1
+		}
+		if aModuleIdentity.Owner() < bModuleIdentity.Owner() {
+			return -1
+		}
+		if aModuleIdentity.Owner() > bModuleIdentity.Owner() {
+			return 1
+		}
+		if aModuleIdentity.Repository() < bModuleIdentity.Repository() {
+			return -1
+		}
+		if aModuleIdentity.Repository() > bModuleIdentity.Repository() {
+			return 1
+		}
+	}
+	if a.Commit() < b.Commit() {
+		return -1
+	}
+	if a.Commit() > b.Commit() {
+		return 1
+	}
+	if a.IsDirect() && !b.IsDirect() {
+		return -1
+	}
+	if !a.IsDirect() && b.IsDirect() {
+		return 1
+	}
+	return 0
+}