From 909d47be1ec8043ca4cf362c3d00c20159543ccd Mon Sep 17 00:00:00 2001 From: abe-winter Date: Tue, 17 Sep 2024 16:20:46 -0400 Subject: [PATCH] APP-6182 validate model triplets in meta.json (#4369) --- cli/module_registry.go | 42 ++++++++++++++++++++++++++++++++++++- cli/module_registry_test.go | 17 +++++++++++++++ resource/api.go | 21 +++++++++++++------ 3 files changed, 73 insertions(+), 7 deletions(-) diff --git a/cli/module_registry.go b/cli/module_registry.go index c31b85d1332..dd766899f09 100644 --- a/cli/module_registry.go +++ b/cli/module_registry.go @@ -31,11 +31,15 @@ import ( "go.viam.com/rdk/module" "go.viam.com/rdk/module/modmanager" modmanageroptions "go.viam.com/rdk/module/modmanager/options" + "go.viam.com/rdk/resource" "go.viam.com/rdk/utils" ) // moduleUploadChunkSize sets the number of bytes included in each chunk of the upload stream. -var moduleUploadChunkSize = 32 * 1024 +var ( + moduleUploadChunkSize = 32 * 1024 + rdkAPITypes = []string{resource.APITypeServiceName, resource.APITypeComponentName} +) // moduleVisibility determines whether modules are public or private. type moduleVisibility string @@ -46,6 +50,14 @@ const ( moduleVisibilityPublic moduleVisibility = "public" ) +type unknownRdkAPITypeError struct { + APIType string +} + +func (err unknownRdkAPITypeError) Error() string { + return fmt.Sprintf("API with unknown type '%s', expected one of %s", err.APIType, strings.Join(rdkAPITypes, ", ")) +} + // ModuleComponent represents an api - model pair. type ModuleComponent struct { API string `json:"api"` @@ -199,6 +211,8 @@ func UpdateModuleAction(c *cli.Context) error { return err } + validateModels(c.App.ErrWriter, &manifest) + response, err := client.updateModule(moduleID, manifest) if err != nil { return err @@ -283,6 +297,8 @@ func UploadModuleAction(c *cli.Context) error { return err } + validateModels(c.App.ErrWriter, &manifest) + _, err = client.updateModule(moduleID, manifest) if err != nil { return errors.Wrap(err, "Module update failed. Please correct the following issues in your meta.json") @@ -316,6 +332,30 @@ func UploadModuleAction(c *cli.Context) error { return nil } +// call validateModelAPI on all models in manifest and warn if violations. +func validateModels(errWriter io.Writer, manifest *moduleManifest) { + for _, model := range manifest.Models { + if err := validateModelAPI(model.API); err != nil { + warningf(errWriter, "error validating API string %s: %s", model.API, err) + } + } +} + +// return a useful error if the model string looks wrong. +func validateModelAPI(modelAPI string) error { + api, err := resource.ParseAPIString(modelAPI) + if err != nil { + return errors.Wrap(err, "unparseable model string") + } + if err := api.Validate(); err != nil { + return errors.Wrap(err, "failed to validate API") + } + if !slices.Contains(rdkAPITypes, api.Type.Name) { + return unknownRdkAPITypeError{APIType: api.Type.Name} + } + return nil +} + // UpdateModelsAction figures out the models that a module supports and updates it's metadata file. func UpdateModelsAction(c *cli.Context) error { logger := logging.NewLogger("x") diff --git a/cli/module_registry_test.go b/cli/module_registry_test.go index 8716281d9e8..f839a3b263d 100644 --- a/cli/module_registry_test.go +++ b/cli/module_registry_test.go @@ -47,3 +47,20 @@ func TestUpdateModelsAction(t *testing.T) { test.That(t, sameModels(metaModels.Models, expectedMetaModels.Models), test.ShouldBeTrue) } + +func TestValidateModelAPI(t *testing.T) { + err := validateModelAPI("rdk:component:x") + test.That(t, err, test.ShouldBeNil) + err = validateModelAPI("rdk:service:x") + test.That(t, err, test.ShouldBeNil) + err = validateModelAPI("rdk:unknown:x") + test.That(t, err, test.ShouldHaveSameTypeAs, unknownRdkAPITypeError{}) + err = validateModelAPI("other:unknown:x") + test.That(t, err, test.ShouldHaveSameTypeAs, unknownRdkAPITypeError{}) + err = validateModelAPI("rdk:component") + test.That(t, err, test.ShouldNotBeNil) + err = validateModelAPI("other:component:$x") + test.That(t, err, test.ShouldNotBeNil) + err = validateModelAPI("other:component:x_") + test.That(t, err, test.ShouldBeNil) +} diff --git a/resource/api.go b/resource/api.go index a6a17a7e61a..7a4110f2132 100644 --- a/resource/api.go +++ b/resource/api.go @@ -125,18 +125,27 @@ func (a API) MarshalJSON() ([]byte, error) { return json.Marshal(a.String()) } +// ParseAPIString builds an API{} struct from a colon-delimited triple. +func ParseAPIString(apiStr string) (API, error) { + ret := API{} + matches := apiRegexValidator.FindStringSubmatch(apiStr) + if matches == nil { + return ret, fmt.Errorf("not a valid API config string. Input: `%v`", apiStr) + } + return APINamespace(matches[1]).WithType(matches[2]).WithSubtype(matches[3]), nil +} + // UnmarshalJSON parses either a string of the form namespace:type:subtype or a json object into an // API object. func (a *API) UnmarshalJSON(data []byte) error { var apiStr string if err := json.Unmarshal(data, &apiStr); err == nil { - // If the value is a string, regex match for a colon partitioned triplet. - if !apiRegexValidator.MatchString(apiStr) { - return fmt.Errorf("not a valid API config string. Input: `%v`", string(data)) + // If the value is a string, parse it. + parsed, err := ParseAPIString(apiStr) + if err != nil { + return err } - - matches := apiRegexValidator.FindStringSubmatch(apiStr) - *a = APINamespace(matches[1]).WithType(matches[2]).WithSubtype(matches[3]) + *a = parsed return nil }