Skip to content

Commit 6fe1ee3

Browse files
sofislsofisl
authored
feat(internal/librarian/java): pre-validate java libraries bom version in config (#6613)
This change modifies librarian to pre-validate that the LibrariesBOMVersion is present for Java libraries during the configuration parsing and tidying phase (`librarian tidy`). Previously, this check was performed deep in the generation phase, which could result in a later, less predictable failure. Fixes #5152 --------- Signed-off-by: sofisl <[email protected]>
1 parent 22c3b33 commit 6fe1ee3

11 files changed

Lines changed: 136 additions & 62 deletions

File tree

‎doc/config-schema.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -352,7 +352,7 @@ This document describes the schema for the librarian.yaml.
352352
| Field | Type | Description |
353353
| :--- | :--- | :--- |
354354
| `custom_group_ids` | map[string]string | Maps API path prefixes (e.g., "google/shopping") to their corresponding Maven Group IDs (e.g., "com.google.shopping"). Use this to override the default "com.google.cloud" Group ID for specific API paths (e.g., maps, ads, shopping). |
355-
| `libraries_bom_version` | string | Is the version of the libraries-bom to use for Java. |
355+
| `libraries_bom_version` | string | Is the version of the libraries-bom to use for Java. This must be set in the default configuration. |
356356

357357
## JavaFileCopy Configuration
358358

‎internal/config/language.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -464,6 +464,7 @@ type JavaDefault struct {
464464
// paths (e.g., maps, ads, shopping).
465465
CustomGroupIDs map[string]string `yaml:"custom_group_ids,omitempty"`
466466
// LibrariesBOMVersion is the version of the libraries-bom to use for Java.
467+
// This must be set in the default configuration.
467468
LibrariesBOMVersion string `yaml:"libraries_bom_version,omitempty"`
468469
}
469470

‎internal/librarian/java/defaults.go‎

Lines changed: 29 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -169,32 +169,44 @@ var (
169169
ErrOmitCommonResourcesConflict = errors.New("conflict: OmitCommonResources is true but google/cloud/common_resources.proto is explicitly listed in AdditionalProtos")
170170
// ErrCannotDeriveReleasedVersion is returned when released_version cannot be derived.
171171
ErrCannotDeriveReleasedVersion = errors.New("cannot derive released version")
172+
// errBOMVersionMissing is returned when libraries_bom_version is not set.
173+
errBOMVersionMissing = errors.New("libraries bom version not found in config")
172174
)
173175

174-
// Validate checks that the Java-specific configuration for a library is
176+
// Validate checks that the Java-specific configuration for a library and global config is
175177
// correctly formatted. It ensures that there are no conflicts in common
176178
// resources configuration.
177-
func Validate(library *config.Library) error {
178-
if library.Version != "" {
179-
if _, err := semver.Parse(library.Version); err != nil {
180-
return fmt.Errorf("library %q: invalid version %q: %w", library.Name, library.Version, err)
181-
}
182-
}
183-
if !library.SkipGenerate && library.Java != nil && library.Java.ReleasedVersion != "" {
184-
if _, err := semver.Parse(library.Java.ReleasedVersion); err != nil {
185-
return fmt.Errorf("library %q: invalid released_version %q: %w", library.Name, library.Java.ReleasedVersion, err)
186-
}
179+
func Validate(cfg *config.Config) error {
180+
var errs []error
181+
if cfg.Default == nil || cfg.Default.Java == nil || cfg.Default.Java.LibrariesBOMVersion == "" {
182+
errs = append(errs, errBOMVersionMissing)
187183
}
188-
for _, api := range library.APIs {
189-
if api.Java == nil || !api.Java.OmitCommonResources {
190-
continue
184+
185+
for _, library := range cfg.Libraries {
186+
if library.Version != "" {
187+
if _, err := semver.Parse(library.Version); err != nil {
188+
errs = append(errs, fmt.Errorf("library %q: invalid version %q: %w", library.Name, library.Version, err))
189+
}
191190
}
192-
for _, proto := range api.Java.AdditionalProtos {
193-
if proto != nil && proto.Path == commonResourcesProto {
194-
return fmt.Errorf("%s: %w", api.Path, ErrOmitCommonResourcesConflict)
191+
if !library.SkipGenerate && library.Java != nil && library.Java.ReleasedVersion != "" {
192+
if _, err := semver.Parse(library.Java.ReleasedVersion); err != nil {
193+
errs = append(errs, fmt.Errorf("library %q: invalid released_version %q: %w", library.Name, library.Java.ReleasedVersion, err))
195194
}
196195
}
196+
for _, api := range library.APIs {
197+
if api.Java == nil || !api.Java.OmitCommonResources {
198+
continue
199+
}
200+
for _, proto := range api.Java.AdditionalProtos {
201+
if proto != nil && proto.Path == commonResourcesProto {
202+
errs = append(errs, fmt.Errorf("%s: %w", api.Path, ErrOmitCommonResourcesConflict))
203+
}
204+
}
197205

206+
}
207+
}
208+
if len(errs) > 0 {
209+
return errors.Join(errs...)
198210
}
199211
return nil
200212
}

‎internal/librarian/java/defaults_test.go‎

Lines changed: 72 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -544,7 +544,15 @@ func TestValidate(t *testing.T) {
544544
},
545545
} {
546546
t.Run(test.name, func(t *testing.T) {
547-
if err := Validate(test.lib); err != nil {
547+
cfg := &config.Config{
548+
Default: &config.Default{
549+
Java: &config.JavaDefault{
550+
LibrariesBOMVersion: "1.2.3",
551+
},
552+
},
553+
Libraries: []*config.Library{test.lib},
554+
}
555+
if err := Validate(cfg); err != nil {
548556
t.Errorf("Validate(%+v) error = %v, want nil", test.lib, err)
549557
}
550558
})
@@ -607,7 +615,15 @@ func TestValidate_Error(t *testing.T) {
607615
},
608616
} {
609617
t.Run(test.name, func(t *testing.T) {
610-
err := Validate(test.lib)
618+
cfg := &config.Config{
619+
Default: &config.Default{
620+
Java: &config.JavaDefault{
621+
LibrariesBOMVersion: "1.2.3",
622+
},
623+
},
624+
Libraries: []*config.Library{test.lib},
625+
}
626+
err := Validate(cfg)
611627
if !errors.Is(err, test.wantErr) {
612628
t.Errorf("Validate() error = %v, want %v", err, test.wantErr)
613629
}
@@ -714,3 +730,57 @@ func TestDeriveLastReleasedVersion_Error(t *testing.T) {
714730
})
715731
}
716732
}
733+
734+
func TestValidate_Config(t *testing.T) {
735+
for _, test := range []struct {
736+
name string
737+
def *config.Default
738+
}{
739+
{
740+
name: "valid bom version",
741+
def: &config.Default{
742+
Java: &config.JavaDefault{
743+
LibrariesBOMVersion: "1.2.3",
744+
},
745+
},
746+
},
747+
} {
748+
t.Run(test.name, func(t *testing.T) {
749+
cfg := &config.Config{Default: test.def}
750+
if err := Validate(cfg); err != nil {
751+
t.Fatal(err)
752+
}
753+
})
754+
}
755+
}
756+
757+
func TestValidate_ConfigError(t *testing.T) {
758+
for _, test := range []struct {
759+
name string
760+
def *config.Default
761+
wantErr error
762+
}{
763+
{
764+
name: "nil default",
765+
def: nil,
766+
wantErr: errBOMVersionMissing,
767+
},
768+
{
769+
name: "empty bom version",
770+
def: &config.Default{
771+
Java: &config.JavaDefault{
772+
LibrariesBOMVersion: "",
773+
},
774+
},
775+
wantErr: errBOMVersionMissing,
776+
},
777+
} {
778+
t.Run(test.name, func(t *testing.T) {
779+
cfg := &config.Config{Default: test.def}
780+
got := Validate(cfg)
781+
if !errors.Is(got, test.wantErr) {
782+
t.Errorf("Validate() error = %v, wantErr %v", got, test.wantErr)
783+
}
784+
})
785+
}
786+
}

‎internal/librarian/java/generate.go‎

Lines changed: 4 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -37,11 +37,10 @@ const (
3737
)
3838

3939
var (
40-
errNoProtos = errors.New("no protos found")
41-
errMonorepoVersion = fmt.Errorf("failed to find monorepo version for %q in config", rootLibrary)
42-
errParentVersion = fmt.Errorf("failed to find parent version for %q in config", parentPOM)
43-
errBOMVersionMissing = errors.New("libraries bom version not found in config")
44-
errUnrecognizedAPI = errors.New("unrecognized non-cloud API: configure java.group_id and java.distribution_name_override in librarian.yaml")
40+
errNoProtos = errors.New("no protos found")
41+
errMonorepoVersion = fmt.Errorf("failed to find monorepo version for %q in config", rootLibrary)
42+
errParentVersion = fmt.Errorf("failed to find parent version for %q in config", parentPOM)
43+
errUnrecognizedAPI = errors.New("unrecognized non-cloud API: configure java.group_id and java.distribution_name_override in librarian.yaml")
4544
// nonRecursivePaths is a set of paths where proto gathering should not be recursive.
4645
nonRecursivePaths = map[string]bool{
4746
"google/api": true,
@@ -336,15 +335,6 @@ func gapicOpt(key, value string) string {
336335
return fmt.Sprintf("%s=%s", key, value)
337336
}
338337

339-
// TODO(https://github.com/googleapis/librarian/issues/5152):
340-
// BOM version should be required and pre-validated, remove this and inline when done.
341-
func findBOMVersion(cfg *config.Config) (string, error) {
342-
if cfg.Default != nil && cfg.Default.Java != nil && cfg.Default.Java.LibrariesBOMVersion != "" {
343-
return cfg.Default.Java.LibrariesBOMVersion, nil
344-
}
345-
return "", errBOMVersionMissing
346-
}
347-
348338
// gatherProtos returns a sorted list of proto files in the given root directory,
349339
// ensuring that subpackage protos (e.g., in a "schema" directory) are included
350340
// in the generation.

‎internal/librarian/java/pom.go‎

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -523,22 +523,21 @@ func writePOM(pomPath, templateName string, data any) (err error) {
523523
return nil
524524
}
525525

526-
func findMonorepoVersion(cfg *config.Config) (string, error) {
526+
func findLibraryVersion(cfg *config.Config, name string, errNotFound error) (string, error) {
527527
for _, lib := range cfg.Libraries {
528-
if lib.Name == rootLibrary {
528+
if lib.Name == name {
529529
return lib.Version, nil
530530
}
531531
}
532-
return "", errMonorepoVersion
532+
return "", errNotFound
533+
}
534+
535+
func findMonorepoVersion(cfg *config.Config) (string, error) {
536+
return findLibraryVersion(cfg, rootLibrary, errMonorepoVersion)
533537
}
534538

535539
// TODO(https://github.com/googleapis/librarian/issues/6411):
536540
// Simplify logic here and check at validate step.
537541
func findParentPOMVersion(cfg *config.Config) (string, error) {
538-
for _, lib := range cfg.Libraries {
539-
if lib.Name == parentPOM {
540-
return lib.Version, nil
541-
}
542-
}
543-
return "", errParentVersion
542+
return findLibraryVersion(cfg, parentPOM, errParentVersion)
544543
}

‎internal/librarian/java/postprocess.go‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -71,10 +71,7 @@ func postProcessLibrary(ctx context.Context, params libraryPostProcessParams) er
7171
if err := createOrVerifyOwlbotPy(params.outDir); err != nil {
7272
return err
7373
}
74-
bomVersion, err := findBOMVersion(params.cfg)
75-
if err != nil {
76-
return err
77-
}
74+
bomVersion := params.cfg.Default.Java.LibrariesBOMVersion
7875
if err := removeKeptFilesFromStaging(params.library, params.outDir); err != nil {
7976
return fmt.Errorf("failed to remove kept files from staging: %w", err)
8077
}

‎internal/librarian/java/postprocess_test.go‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -687,14 +687,6 @@ func TestPostProcessLibrary_ErrorCase(t *testing.T) {
687687
setup func(t *testing.T, outDir string)
688688
wantErr error
689689
}{
690-
{
691-
name: "findBOMVersion failure",
692-
cfg: &config.Config{},
693-
setup: func(t *testing.T, outDir string) {
694-
writeOwlBot(t, outDir, "sys.exit(0)")
695-
},
696-
wantErr: errBOMVersionMissing,
697-
},
698690
{
699691
name: "runOwlBot failure (missing templates)",
700692
cfg: defaultCfg,

‎internal/librarian/tidy.go‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -165,9 +165,6 @@ func validateLibraries(cfg *config.Config) error {
165165
pathCount[ch.Path]++
166166
}
167167
}
168-
if err := validateLanguageConfig(lib, cfg.Language); err != nil {
169-
errs = append(errs, err)
170-
}
171168
}
172169
for name, count := range nameCount {
173170
if count > 1 {
@@ -179,6 +176,9 @@ func validateLibraries(cfg *config.Config) error {
179176
errs = append(errs, fmt.Errorf("%w: %s (appears %d times)", errDuplicateAPIPath, path, count))
180177
}
181178
}
179+
if err := validateLanguageConfig(cfg); err != nil {
180+
errs = append(errs, err)
181+
}
182182
if len(errs) > 0 {
183183
return errors.Join(errs...)
184184
}
@@ -187,14 +187,14 @@ func validateLibraries(cfg *config.Config) error {
187187

188188
// languageValidators maps a language to a function that validates the language-specific
189189
// configuration.
190-
var languageValidators = map[string]func(*config.Library) error{
190+
var languageValidators = map[string]func(*config.Config) error{
191191
config.LanguageJava: java.Validate,
192192
}
193193

194194
// validateLanguageConfig finds and executes the language-specific validator for a library.
195-
func validateLanguageConfig(lib *config.Library, language string) error {
196-
if validator, ok := languageValidators[language]; ok {
197-
return validator(lib)
195+
func validateLanguageConfig(cfg *config.Config) error {
196+
if validator, ok := languageValidators[cfg.Language]; ok {
197+
return validator(cfg)
198198
}
199199
return nil
200200
}

‎internal/librarian/tidy_test.go‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,13 @@ func TestValidateLibraries(t *testing.T) {
8787
Language: test.language,
8888
Libraries: test.libraries,
8989
}
90+
if test.language == config.LanguageJava {
91+
cfg.Default = &config.Default{
92+
Java: &config.JavaDefault{
93+
LibrariesBOMVersion: "1.2.3",
94+
},
95+
}
96+
}
9097
err := validateLibraries(cfg)
9198
if test.wantErr == nil {
9299
if err != nil {
@@ -620,6 +627,9 @@ func TestTidy_DerivableOutput(t *testing.T) {
620627
Language: test.language,
621628
Default: &config.Default{
622629
Output: "generated/",
630+
Java: &config.JavaDefault{
631+
LibrariesBOMVersion: "1.0.0",
632+
},
623633
},
624634
Sources: googleapisSource,
625635
Libraries: []*config.Library{lib},

0 commit comments

Comments
 (0)