Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

APP-6182 validate model triplets in meta.json #4369

Merged
merged 3 commits into from
Sep 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 41 additions & 1 deletion cli/module_registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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"`
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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) {
Copy link
Member

@zaporter-work zaporter-work Sep 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[opt not a priority at all] use api.IsComponent() || api.IsService() like the other uses in rdk

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the rdkAPITypes slice lived in RDK instead of CLI, it would be a canonical place to store the check; it's self-updating in a way that the || expression isn't

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or better yet, if it lived inside api.Validate()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or like generally would be nice if go enums exposed the entire list of values at the language level

we sort of get this from the linter but only in switch statements

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it lived inside api.Validate()

yes good point

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")
Expand Down
17 changes: 17 additions & 0 deletions cli/module_registry_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
21 changes: 15 additions & 6 deletions resource/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
Loading