Skip to content

Commit 5c95940

Browse files
coryancoryan
authored
refactor(sidekick): parser consumes full options (#2533)
1 parent fd73df2 commit 5c95940

File tree

9 files changed

+110
-40
lines changed

9 files changed

+110
-40
lines changed

‎internal/sidekick/internal/parser/disco.go‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -25,9 +25,10 @@ import (
2525

2626
// ParseDisco reads discovery docs specifications and converts them into
2727
// the `api.API` model.
28-
func ParseDisco(source, serviceConfigFile string, options map[string]string) (*api.API, error) {
29-
for _, opt := range config.SourceRoots(options) {
30-
location, ok := options[opt]
28+
func ParseDisco(cfg *config.Config) (*api.API, error) {
29+
source := cfg.General.SpecificationSource
30+
for _, opt := range config.SourceRoots(cfg.Source) {
31+
location, ok := cfg.Source[opt]
3132
if !ok {
3233
// Ignore options that are not set
3334
continue
@@ -42,7 +43,7 @@ func ParseDisco(source, serviceConfigFile string, options map[string]string) (*a
4243
if err != nil {
4344
return nil, err
4445
}
45-
serviceConfig, err := loadServiceConfig(serviceConfigFile, options)
46+
serviceConfig, err := loadServiceConfig(cfg)
4647
if err != nil {
4748
return nil, err
4849
}

‎internal/sidekick/internal/parser/disco_test.go‎

Lines changed: 51 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,18 @@ import (
2020
"github.com/google/go-cmp/cmp"
2121
"github.com/google/go-cmp/cmp/cmpopts"
2222
"github.com/googleapis/librarian/internal/sidekick/internal/api"
23+
"github.com/googleapis/librarian/internal/sidekick/internal/config"
2324
)
2425

2526
func TestDisco_Parse(t *testing.T) {
26-
// Mixing Compute and Secret Manager like this is fine in tests.
27-
got, err := ParseDisco(discoSourceFile, secretManagerYamlFullPath, map[string]string{})
27+
cfg := &config.Config{
28+
General: config.GeneralConfig{
29+
// Mixing Compute and Secret Manager like this is fine in tests.
30+
SpecificationSource: discoSourceFile,
31+
ServiceConfig: secretManagerYamlFullPath,
32+
},
33+
}
34+
got, err := ParseDisco(cfg)
2835
if err != nil {
2936
t.Fatal(err)
3037
}
@@ -47,10 +54,16 @@ func TestDisco_Parse(t *testing.T) {
4754
}
4855

4956
func TestDisco_FindSources(t *testing.T) {
50-
got, err := ParseDisco(discoSourceFileRelative, "", map[string]string{
51-
"test-root": testdataDir,
52-
"roots": "undefined,test",
53-
})
57+
cfg := &config.Config{
58+
General: config.GeneralConfig{
59+
SpecificationSource: discoSourceFileRelative,
60+
},
61+
Source: map[string]string{
62+
"test-root": testdataDir,
63+
"roots": "undefined,test",
64+
},
65+
}
66+
got, err := ParseDisco(cfg)
5467
if err != nil {
5568
t.Fatal(err)
5669
}
@@ -70,7 +83,12 @@ func TestDisco_FindSources(t *testing.T) {
7083
}
7184

7285
func TestDisco_ParseNoServiceConfig(t *testing.T) {
73-
got, err := ParseDisco(discoSourceFile, "", map[string]string{})
86+
cfg := &config.Config{
87+
General: config.GeneralConfig{
88+
SpecificationSource: discoSourceFile,
89+
},
90+
}
91+
got, err := ParseDisco(cfg)
7492
if err != nil {
7593
t.Fatal(err)
7694
}
@@ -89,7 +107,12 @@ func TestDisco_ParseNoServiceConfig(t *testing.T) {
89107
}
90108

91109
func TestDisco_ParsePagination(t *testing.T) {
92-
model, err := ParseDisco(discoSourceFile, "", map[string]string{})
110+
cfg := &config.Config{
111+
General: config.GeneralConfig{
112+
SpecificationSource: discoSourceFile,
113+
},
114+
}
115+
model, err := ParseDisco(cfg)
93116
if err != nil {
94117
t.Fatal(err)
95118
}
@@ -113,7 +136,12 @@ func TestDisco_ParsePagination(t *testing.T) {
113136
}
114137

115138
func TestDisco_ParsePaginationAggregate(t *testing.T) {
116-
model, err := ParseDisco(discoSourceFile, "", map[string]string{})
139+
cfg := &config.Config{
140+
General: config.GeneralConfig{
141+
SpecificationSource: discoSourceFile,
142+
},
143+
}
144+
model, err := ParseDisco(cfg)
117145
if err != nil {
118146
t.Fatal(err)
119147
}
@@ -137,7 +165,12 @@ func TestDisco_ParsePaginationAggregate(t *testing.T) {
137165
}
138166

139167
func TestDisco_ParseDeprecatedEnum(t *testing.T) {
140-
model, err := ParseDisco(discoSourceFile, "", map[string]string{})
168+
cfg := &config.Config{
169+
General: config.GeneralConfig{
170+
SpecificationSource: discoSourceFile,
171+
},
172+
}
173+
model, err := ParseDisco(cfg)
141174
if err != nil {
142175
t.Fatal(err)
143176
}
@@ -157,15 +190,13 @@ func TestDisco_ParseDeprecatedEnum(t *testing.T) {
157190
}
158191

159192
func TestDisco_ParseBadFiles(t *testing.T) {
160-
if _, err := ParseDisco("-invalid-file-name-", secretManagerYamlFullPath, map[string]string{}); err == nil {
161-
t.Fatalf("expected error with missing source file")
162-
}
163-
164-
if _, err := ParseDisco(discoSourceFile, "-invalid-file-name-", map[string]string{}); err == nil {
165-
t.Fatalf("expected error with missing service config yaml file")
166-
}
167-
168-
if _, err := ParseDisco(secretManagerYamlFullPath, secretManagerYamlFullPath, map[string]string{}); err == nil {
169-
t.Fatalf("expected error with invalid source file contents")
193+
for _, cfg := range []config.GeneralConfig{
194+
{SpecificationSource: "-invalid-file-name-", ServiceConfig: secretManagerYamlFullPath},
195+
{SpecificationSource: discoSourceFile, ServiceConfig: "-invalid-file-name-"},
196+
{SpecificationSource: secretManagerYamlFullPath, ServiceConfig: secretManagerYamlFullPath},
197+
} {
198+
if got, err := ParseDisco(&config.Config{General: cfg}); err == nil {
199+
t.Fatalf("expected error with missing source file, got=%v", got)
200+
}
170201
}
171202
}

‎internal/sidekick/internal/parser/openapi.go‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import (
2323
"strings"
2424

2525
"github.com/googleapis/librarian/internal/sidekick/internal/api"
26+
"github.com/googleapis/librarian/internal/sidekick/internal/config"
2627
"github.com/googleapis/librarian/internal/sidekick/internal/parser/httprule"
2728
"github.com/googleapis/librarian/internal/sidekick/internal/parser/svcconfig"
2829
"github.com/pb33f/libopenapi"
@@ -33,7 +34,8 @@ import (
3334
)
3435

3536
// ParseOpenAPI parses an OpenAPI specification and returns an API model.
36-
func ParseOpenAPI(source, serviceConfigFile string, options map[string]string) (*api.API, error) {
37+
func ParseOpenAPI(cfg *config.Config) (*api.API, error) {
38+
source := cfg.General.SpecificationSource
3739
contents, err := os.ReadFile(source)
3840
if err != nil {
3941
return nil, err
@@ -42,7 +44,7 @@ func ParseOpenAPI(source, serviceConfigFile string, options map[string]string) (
4244
if err != nil {
4345
return nil, err
4446
}
45-
serviceConfig, err := loadServiceConfig(serviceConfigFile, options)
47+
serviceConfig, err := loadServiceConfig(cfg)
4648
if err != nil {
4749
return nil, err
4850
}

‎internal/sidekick/internal/parser/openapi_test.go‎

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"github.com/google/go-cmp/cmp/cmpopts"
2323
"github.com/googleapis/librarian/internal/sidekick/internal/api"
2424
"github.com/googleapis/librarian/internal/sidekick/internal/api/apitest"
25+
"github.com/googleapis/librarian/internal/sidekick/internal/config"
2526
"github.com/googleapis/librarian/internal/sidekick/internal/sample"
2627
"google.golang.org/genproto/googleapis/api/annotations"
2728
"google.golang.org/genproto/googleapis/api/serviceconfig"
@@ -525,7 +526,7 @@ func TestOpenAPI_MapInteger(t *testing.T) {
525526

526527
func openapiSecretManagerAPI(t *testing.T) *api.API {
527528
t.Helper()
528-
contents, err := os.ReadFile("../../testdata/openapi/secretmanager_openapi_v1.json")
529+
contents, err := os.ReadFile(openAPIFile)
529530
if err != nil {
530531
t.Fatal(err)
531532
}
@@ -778,7 +779,7 @@ func TestOpenAPI_ServicePlaceholder(t *testing.T) {
778779
}
779780

780781
func TestOpenAPI_MakeApiWithServiceConfig(t *testing.T) {
781-
contents, err := os.ReadFile("../../testdata/openapi/secretmanager_openapi_v1.json")
782+
contents, err := os.ReadFile(openAPIFile)
782783
if err != nil {
783784
t.Fatal(err)
784785
}
@@ -798,7 +799,7 @@ func TestOpenAPI_MakeApiWithServiceConfig(t *testing.T) {
798799
}
799800

800801
func TestOpenAPI_MakeApiServiceConfigOverridesDescription(t *testing.T) {
801-
contents, err := os.ReadFile("../../testdata/openapi/secretmanager_openapi_v1.json")
802+
contents, err := os.ReadFile(openAPIFile)
802803
if err != nil {
803804
t.Fatal(err)
804805
}
@@ -821,7 +822,7 @@ func TestOpenAPI_MakeApiServiceConfigOverridesDescription(t *testing.T) {
821822
}
822823

823824
func TestOpenAPI_SyntheticMessageWithExistingBody(t *testing.T) {
824-
contents, err := os.ReadFile("../../testdata/openapi/secretmanager_openapi_v1.json")
825+
contents, err := os.ReadFile(openAPIFile)
825826
if err != nil {
826827
t.Fatal(err)
827828
}
@@ -1270,6 +1271,21 @@ func TestOpenAPI_Deprecated(t *testing.T) {
12701271
})
12711272
}
12721273

1274+
func TestOpenAPI_ParseBadFiles(t *testing.T) {
1275+
for _, general := range []config.GeneralConfig{
1276+
{SpecificationSource: "-invalid-file-name-", ServiceConfig: secretManagerYamlFullPath},
1277+
{SpecificationSource: openAPIFile, ServiceConfig: "-invalid-file-name-"},
1278+
{SpecificationSource: secretManagerYamlFullPath, ServiceConfig: secretManagerYamlFullPath},
1279+
} {
1280+
cfg := &config.Config{
1281+
General: general,
1282+
}
1283+
if got, err := ParseOpenAPI(cfg); err == nil {
1284+
t.Fatalf("expected error with missing source file, got=%v", got)
1285+
}
1286+
}
1287+
}
1288+
12731289
const openAPISingleMessagePreamble = `
12741290
{
12751291
"openapi": "3.0.3",

‎internal/sidekick/internal/parser/parser.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -29,11 +29,11 @@ func CreateModel(config *config.Config) (*api.API, error) {
2929
var model *api.API
3030
switch config.General.SpecificationFormat {
3131
case "disco":
32-
model, err = ParseDisco(config.General.SpecificationSource, config.General.ServiceConfig, config.Source)
32+
model, err = ParseDisco(config)
3333
case "openapi":
34-
model, err = ParseOpenAPI(config.General.SpecificationSource, config.General.ServiceConfig, config.Source)
34+
model, err = ParseOpenAPI(config)
3535
case "protobuf":
36-
model, err = ParseProtobuf(config.General.SpecificationSource, config.General.ServiceConfig, config.Source)
36+
model, err = ParseProtobuf(config)
3737
case "none":
3838
return nil, nil
3939
default:

‎internal/sidekick/internal/parser/parser_test.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,8 @@ var (
2929
discoSourceFile = path.Join(testdataDir, discoSourceFileRelative)
3030
secretManagerYamlRelative = "google/cloud/secretmanager/v1/secretmanager_v1.yaml"
3131
secretManagerYamlFullPath = path.Join(testdataDir, "googleapis", secretManagerYamlRelative)
32+
openAPIFile = path.Join(testdataDir, "openapi", "secretmanager_openapi_v1.json")
33+
protobufFile = path.Join("testdata", "scalar.proto")
3234
)
3335

3436
func TestCreateModelDisco(t *testing.T) {

‎internal/sidekick/internal/parser/protobuf.go‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -37,12 +37,13 @@ import (
3737

3838
// ParseProtobuf reads Protobuf specifications and converts them into
3939
// the `api.API` model.
40-
func ParseProtobuf(source, serviceConfigFile string, options map[string]string) (*api.API, error) {
41-
request, err := newCodeGeneratorRequest(source, options)
40+
func ParseProtobuf(cfg *config.Config) (*api.API, error) {
41+
source := cfg.General.SpecificationSource
42+
request, err := newCodeGeneratorRequest(source, cfg.Source)
4243
if err != nil {
4344
return nil, err
4445
}
45-
serviceConfig, err := loadServiceConfig(serviceConfigFile, options)
46+
serviceConfig, err := loadServiceConfig(cfg)
4647
if err != nil {
4748
return nil, err
4849
}

‎internal/sidekick/internal/parser/protobuf_test.go‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"github.com/google/go-cmp/cmp/cmpopts"
2323
"github.com/googleapis/librarian/internal/sidekick/internal/api"
2424
"github.com/googleapis/librarian/internal/sidekick/internal/api/apitest"
25+
"github.com/googleapis/librarian/internal/sidekick/internal/config"
2526
"github.com/googleapis/librarian/internal/sidekick/internal/sample"
2627
"google.golang.org/genproto/googleapis/api/annotations"
2728
"google.golang.org/genproto/googleapis/api/serviceconfig"
@@ -1802,6 +1803,22 @@ func TestProtobuf_Deprecated(t *testing.T) {
18021803
})
18031804
}
18041805

1806+
func TestProtobuf_ParseBadFiles(t *testing.T) {
1807+
requireProtoc(t)
1808+
for _, general := range []config.GeneralConfig{
1809+
{SpecificationSource: "-invalid-file-name-", ServiceConfig: secretManagerYamlFullPath},
1810+
{SpecificationSource: protobufFile, ServiceConfig: "-invalid-file-name-"},
1811+
{SpecificationSource: secretManagerYamlFullPath, ServiceConfig: secretManagerYamlFullPath},
1812+
} {
1813+
cfg := &config.Config{
1814+
General: general,
1815+
}
1816+
if got, err := ParseProtobuf(cfg); err == nil {
1817+
t.Fatalf("expected error with missing source file, got=%v", got)
1818+
}
1819+
}
1820+
}
1821+
18051822
func newTestCodeGeneratorRequest(t *testing.T, filename string) *pluginpb.CodeGeneratorRequest {
18061823
t.Helper()
18071824
options := map[string]string{

‎internal/sidekick/internal/parser/service_config.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,11 +26,11 @@ import (
2626
"gopkg.in/yaml.v3"
2727
)
2828

29-
func loadServiceConfig(serviceConfigFile string, source map[string]string) (*serviceconfig.Service, error) {
30-
if serviceConfigFile == "" {
31-
return nil, nil
29+
func loadServiceConfig(cfg *config.Config) (*serviceconfig.Service, error) {
30+
if name := cfg.General.ServiceConfig; name != "" {
31+
return readServiceConfig(findServiceConfigPath(name, cfg.Source))
3232
}
33-
return readServiceConfig(findServiceConfigPath(serviceConfigFile, source))
33+
return nil, nil
3434
}
3535

3636
func readServiceConfig(serviceConfigPath string) (*serviceconfig.Service, error) {

0 commit comments

Comments
 (0)