From dee4dca31136bc00635ee5fae764ea8bb5d51114 Mon Sep 17 00:00:00 2001 From: bufdev Date: Wed, 8 Nov 2023 15:54:24 -0500 Subject: [PATCH] make OpaqueID public --- private/bufnew/bufmodule/bufmodule_test.go | 32 +++++------ private/bufnew/bufmodule/cache.go | 2 +- private/bufnew/bufmodule/module.go | 63 +++++++++++---------- private/bufnew/bufmodule/module_builder.go | 2 +- private/bufnew/bufmodule/module_info.go | 18 ------ private/bufnew/bufmodule/module_provider.go | 2 +- 6 files changed, 49 insertions(+), 70 deletions(-) diff --git a/private/bufnew/bufmodule/bufmodule_test.go b/private/bufnew/bufmodule/bufmodule_test.go index 4ac5ac846b..0547ff5069 100644 --- a/private/bufnew/bufmodule/bufmodule_test.go +++ b/private/bufnew/bufmodule/bufmodule_test.go @@ -99,15 +99,15 @@ func TestBasic(t *testing.T) { require.NoError(t, err) require.Equal(t, 4, len(modules)) - module2 := testFindModuleWithName(t, modules, "buf.build/bar/module2") + module2 := testFindModuleWithOpaqueID(t, modules, "buf.build/bar/module2") require.Equal( t, []string{ "buf.build/foo/extdep1", "buf.build/foo/extdep2", - // Skipping module1 since it doesn't have a name, TODO + "path/to/module1", }, - testSortedDepModuleNames(t, module2), + testSortedDepOpaqueIDs(t, module2), ) } @@ -117,38 +117,32 @@ func testNewBucketForPathToData(t *testing.T, pathToData map[string][]byte) stor return bucket } -// TODO: switch to opaque ID -func testFindModuleWithName(t *testing.T, modules []bufmodule.Module, moduleFullNameString string) bufmodule.Module { +func testFindModuleWithOpaqueID(t *testing.T, modules []bufmodule.Module, opaqueID string) bufmodule.Module { var foundModules []bufmodule.Module for _, module := range modules { - if moduleFullName := module.ModuleFullName(); moduleFullName != nil { - if moduleFullName.String() == moduleFullNameString { - foundModules = append(foundModules, module) - } + if module.OpaqueID() == opaqueID { + foundModules = append(foundModules, module) } } switch len(foundModules) { case 0: - require.NoError(t, fmt.Errorf("no module found for name %q", moduleFullNameString)) + require.NoError(t, fmt.Errorf("no module found for opaqueID %q", opaqueID)) return nil case 1: return foundModules[0] default: - require.NoError(t, fmt.Errorf("multiple modules found for name %q", moduleFullNameString)) + require.NoError(t, fmt.Errorf("multiple modules found for opaqueID %q", opaqueID)) return nil } } -// TODO: switch to opaque ID -func testSortedDepModuleNames(t *testing.T, module bufmodule.Module) []string { +func testSortedDepOpaqueIDs(t *testing.T, module bufmodule.Module) []string { depModules, err := module.DepModules() require.NoError(t, err) - depModuleFullNameStrings := make([]string, 0, len(depModules)) + depOpaqueIDs := make([]string, 0, len(depModules)) for _, depModule := range depModules { - if moduleFullName := depModule.ModuleFullName(); moduleFullName != nil { - depModuleFullNameStrings = append(depModuleFullNameStrings, moduleFullName.String()) - } + depOpaqueIDs = append(depOpaqueIDs, depModule.OpaqueID()) } - sort.Strings(depModuleFullNameStrings) - return depModuleFullNameStrings + sort.Strings(depOpaqueIDs) + return depOpaqueIDs } diff --git a/private/bufnew/bufmodule/cache.go b/private/bufnew/bufmodule/cache.go index 51cdc328b5..91d6885f12 100644 --- a/private/bufnew/bufmodule/cache.go +++ b/private/bufnew/bufmodule/cache.go @@ -94,7 +94,7 @@ func (c *cache) SetModules(modules []Module) error { } opaqueIDToModule := make(map[string]Module) for _, module := range modules { - opaqueID := module.opaqueID() + opaqueID := module.OpaqueID() if _, ok := opaqueIDToModule[opaqueID]; ok { // This is a system error, we should have already validated this. return fmt.Errorf("duplicate opaqueID: %q", opaqueID) diff --git a/private/bufnew/bufmodule/module.go b/private/bufnew/bufmodule/module.go index a37b2f83a3..7b1261de94 100644 --- a/private/bufnew/bufmodule/module.go +++ b/private/bufnew/bufmodule/module.go @@ -18,7 +18,6 @@ import ( "context" "errors" "sort" - "strings" "sync" "github.com/bufbuild/buf/private/bufpkg/bufcas" @@ -33,11 +32,9 @@ type Module interface { // ModuleReadBucket allows for reading of a Module's files. // // A Module consists of .proto files, documentation file(s), and license file(s). All of these - // are accessible via the functions on ModuleReadBucket. As such, the FileTypes() function will - // return FileTypeProto, FileTypeDoc, FileTypeLicense. + // are accessible via the functions on ModuleReadBucket. // - // This bucket is not self-contained - it requires the files from dependencies to be so. As such, - // IsProtoFilesSelfContained() returns false. + // This bucket is not self-contained - it requires the files from dependencies to be so. // // This package currently exposes functionality to walk just the .proto files, and get the singular // documentation and license files, via WalkProtoFileInfos, GetDocFile, and GetLicenseFile. @@ -47,15 +44,33 @@ type Module interface { // exist within a Module (currently, only one of each is allowed). ModuleReadBucket + // OpaqueID returns an unstructured ID that can uniquely identify a Module relative + // to other Modules it was built with from a ModuleBuilder. + // + // An OpaqueID can be used to denote expected uniqueness of content; if two Modules + // have different IDs, they should be expected to be logically different Modules. + // + // This ID's structure should not be relied upon, and is not a globally-unique identifier. + // It's uniqueness property only applies to the lifetime of the Module, and only within + // Modules commonly built from a ModuleBuilder. + // + // This ID is not stable between different invocations; the same Module built twice + // in two separate ModuleBuilder invocations may have different IDs. + // + // This ID will never be empty. + OpaqueID() string + // DepModules returns the dependency list for this specific module. // // This list is pruned - only Modules that this Module actually depends on via import statements // within its .proto files will be returned. // // Dependencies with the same ModuleFullName will always have the same commits and digests. + // + // The order of returned list of Modules will be stable between invocations, but should + // not be considered to be sorted in any way. DepModules() ([]Module, error) - opaqueID() string isModule() } @@ -84,8 +99,9 @@ func newModule( moduleFullName ModuleFullName, commitID string, ) (*module, error) { - if bucketID == "" { - return nil, errors.New("bucketID was empty when constructing a new bucket-based Module") + if bucketID == "" && moduleFullName == nil { + // This is a system error. + return nil, errors.New("bucketID was empty and moduleFullName was nil when constructing a Module, one of these must be set") } module := &module{ cache: cache, @@ -123,30 +139,18 @@ func (m *module) Digest() (bufcas.Digest, error) { return m.getDigest() } -func (m *module) DepModules() ([]Module, error) { - return m.getDepModules() -} - -func (m *module) String() string { - var builder strings.Builder - if m.moduleFullName != nil { - builder.WriteString(m.moduleFullName.String()) - if m.commitID != "" { - builder.WriteString(":") - builder.WriteString(m.commitID) - } - } - return builder.String() -} - -func (m *module) opaqueID() string { +func (m *module) OpaqueID() string { + // We know that one of bucketID and moduleFullName are present via construction. if m.moduleFullName != nil { return m.moduleFullName.String() } - // We know bucketID is present via construction. return m.bucketID } +func (m *module) DepModules() ([]Module, error) { + return m.getDepModules() +} + func (*module) isModuleInfo() {} func (*module) isModule() {} @@ -206,11 +210,10 @@ func getActualDepModules( depModules = append(depModules, depModule) } // Sorting by at least Opaque ID to get a consistent return order for a given call. - // We should probably sort by digest TODO. sort.Slice( depModules, func(i int, j int) bool { - return depModules[i].opaqueID() < depModules[j].opaqueID() + return depModules[i].OpaqueID() < depModules[j].OpaqueID() }, ) return depModules, nil @@ -225,7 +228,7 @@ func getActualDepModulesRec( // already discovered deps depOpaqueIDToDepModule map[string]Module, ) error { - opaqueID := module.opaqueID() + opaqueID := module.OpaqueID() if _, ok := visitedOpaqueIDs[opaqueID]; ok { // TODO: detect cycles, this is just making sure we don't recurse return nil @@ -245,7 +248,7 @@ func getActualDepModulesRec( if err != nil { return err } - potentialDepOpaqueID := potentialDepModule.opaqueID() + potentialDepOpaqueID := potentialDepModule.OpaqueID() // If this is in the same module, it's not a dep if potentialDepOpaqueID != opaqueID { // No longer just potential, now real dep. diff --git a/private/bufnew/bufmodule/module_builder.go b/private/bufnew/bufmodule/module_builder.go index 6820586ea9..6ad0055e46 100644 --- a/private/bufnew/bufmodule/module_builder.go +++ b/private/bufnew/bufmodule/module_builder.go @@ -169,7 +169,7 @@ func getUniqueModulesWithEarlierPreferred(ctx context.Context, modules []Module) //alreadySeenDigestStrings := make(map[string]struct{}) uniqueModules := make([]Module, 0, len(modules)) for _, module := range modules { - opaqueID := module.opaqueID() + opaqueID := module.OpaqueID() if opaqueID == "" { return nil, errors.New("opaqueID was empty which should never happen") } diff --git a/private/bufnew/bufmodule/module_info.go b/private/bufnew/bufmodule/module_info.go index 4fc0e49ddf..8fd7699407 100644 --- a/private/bufnew/bufmodule/module_info.go +++ b/private/bufnew/bufmodule/module_info.go @@ -17,7 +17,6 @@ package bufmodule import ( "errors" "fmt" - "strings" "github.com/bufbuild/buf/private/bufpkg/bufcas" ) @@ -31,11 +30,6 @@ import ( // depending on the context. For example, when dealing with non-colocated Modules, we will // expect that ModuleFullName and CommitID are present, and this can be validated for. type ModuleInfo interface { - // String returns "registry/owner/name[:commitID]" if ModuleFullName and/or commitID are present. - // - // TODO: probably remove, this is mostly for debugging right now - fmt.Stringer - // ModuleFullName returns the full name of the Module. // // May be nil depending on context. For example, when read from lock files, this will @@ -100,16 +94,4 @@ func (m *moduleInfo) Digest() (bufcas.Digest, error) { return m.digest, nil } -func (m *moduleInfo) String() string { - var builder strings.Builder - if m.moduleFullName != nil { - builder.WriteString(m.moduleFullName.String()) - if m.commitID != "" { - builder.WriteString(":") - builder.WriteString(m.commitID) - } - } - return builder.String() -} - func (*moduleInfo) isModuleInfo() {} diff --git a/private/bufnew/bufmodule/module_provider.go b/private/bufnew/bufmodule/module_provider.go index 302c50030b..4bf5a3ed34 100644 --- a/private/bufnew/bufmodule/module_provider.go +++ b/private/bufnew/bufmodule/module_provider.go @@ -250,7 +250,7 @@ func (m *lazyModule) DepModules() ([]Module, error) { return m.getDepModules() } -func (m *lazyModule) opaqueID() string { +func (m *lazyModule) OpaqueID() string { // We know ModuleFullName is present via construction. return m.ModuleFullName().String() }