From 68978c53885a978bedeeafcb7d8d0c025a235012 Mon Sep 17 00:00:00 2001 From: cramja Date: Thu, 17 Jun 2021 08:01:19 -0700 Subject: [PATCH] cleanup code review comments --- cmd/namespace/validate.go | 22 ++++++++--------- cmd/namespace/validate_test.go | 26 ++++++++++++--------- internal/driver/config/namespace_watcher.go | 2 +- 3 files changed, 27 insertions(+), 23 deletions(-) diff --git a/cmd/namespace/validate.go b/cmd/namespace/validate.go index 1e97fc818..84a697b7e 100644 --- a/cmd/namespace/validate.go +++ b/cmd/namespace/validate.go @@ -10,6 +10,7 @@ import ( "github.com/ory/jsonschema/v3" "github.com/ory/x/cmdx" + "github.com/ory/x/configx" "github.com/ory/x/jsonschemax" "github.com/ory/x/logrusx" "github.com/pelletier/go-toml" @@ -29,9 +30,9 @@ Validates namespace definitions. Parses namespace yaml files or configuration files passed via the configuration flag. Returns human readable errors. Useful for debugging.`, RunE: func(cmd *cobra.Command, args []string) error { - cfFlag := cmd.Flag("config") + cfFlag := cmd.Flag(configx.FlagConfig) if cfFlag.Changed { - cfiles, err := cmd.Flags().GetStringSlice("config") + cfiles, err := cmd.Flags().GetStringSlice(configx.FlagConfig) if err != nil { _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "Failed to read config command line flag\n%+v\n", err) return cmdx.FailSilently(cmd) @@ -64,22 +65,22 @@ var configSchema *jsonschema.Schema const schemaPath = "github.com/ory/keto/.schema/config.schema.json" -func readSchema(cmd *cobra.Command) error { +func getSchema(cmd *cobra.Command) (*jsonschema.Schema, error) { if configSchema == nil { c := jsonschema.NewCompiler() if err := c.AddResource(schemaPath, bytes.NewBuffer(config.Schema)); err != nil { _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "Could not add the config schema file to the compiler. This is an internal error that should be reported. Thanks ;)\n%+v\n", err) - return cmdx.FailSilently(cmd) + return nil, cmdx.FailSilently(cmd) } var err error configSchema, err = c.Compile(schemaPath + "#/definitions/namespace") if err != nil { _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "Could not compile the config schema file. This is an internal error that should be reported. Thanks ;)\n%+v\n", err) - return cmdx.FailSilently(cmd) + return nil, cmdx.FailSilently(cmd) } } - return nil + return configSchema, nil } func validateNamespaceFile(cmd *cobra.Command, fn string) (*namespace.Namespace, error) { @@ -92,8 +93,8 @@ func validateNamespaceFile(cmd *cobra.Command, fn string) (*namespace.Namespace, return validateNamespaceBytes(cmd, fn, fc, yaml.Unmarshal) } -func validateNamespaceBytes(cmd *cobra.Command, name string, b []byte, parser config.Parser) (*namespace.Namespace, error) { - err := readSchema(cmd) +func validateNamespaceBytes(cmd *cobra.Command, name string, b []byte, parser func([]byte, interface{}) error) (*namespace.Namespace, error) { + schema, err := getSchema(cmd) if err != nil { return nil, err } @@ -104,7 +105,7 @@ func validateNamespaceBytes(cmd *cobra.Command, name string, b []byte, parser co return nil, cmdx.FailSilently(cmd) } - if err := configSchema.ValidateInterface(val); err != nil { + if err := schema.ValidateInterface(val); err != nil { fmt.Fprintf(cmd.ErrOrStderr(), "File %s was not a valid namespace file. Reasons:\n", name) jsonschemax.FormatValidationErrorForCLI(cmd.ErrOrStderr(), config.Schema, err) return nil, cmdx.FailSilently(cmd) @@ -126,7 +127,6 @@ func validateConfigFile(cmd *cobra.Command, fn string) error { return cmdx.FailSilently(cmd) } - // TODO: handle other config formats (json, toml, etc) var val map[string]interface{} dot := strings.LastIndex(fn, ".") if dot == -1 { @@ -199,7 +199,7 @@ func validateConfigFile(cmd *cobra.Command, fn string) error { } return nil default: - _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "unknown type for key 'namespace' in config file: %v\n", t) + _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "unknown type for key 'namespaces' in config file: %v\n", t) return cmdx.FailSilently(cmd) } } else { diff --git a/cmd/namespace/validate_test.go b/cmd/namespace/validate_test.go index 35bd46cc8..dc5c6c386 100644 --- a/cmd/namespace/validate_test.go +++ b/cmd/namespace/validate_test.go @@ -9,6 +9,7 @@ import ( "github.com/ory/x/configx" "github.com/spf13/cobra" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) const fileMode = 0644 @@ -50,13 +51,13 @@ func TestValidateConfigNamespaces(t *testing.T) { t.Run("case=read valid config with embedded namespaces", func(t *testing.T) { dir := t.TempDir() fnyaml := dir + "/keto.yaml" - assert.NoError(t, ioutil.WriteFile(fnyaml, []byte(configEmbeddedYaml), fileMode)) + require.NoError(t, ioutil.WriteFile(fnyaml, []byte(configEmbeddedYaml), fileMode)) fnjson := dir + "/keto.json" - assert.NoError(t, ioutil.WriteFile(fnjson, []byte(configEmbeddedJson), fileMode)) + require.NoError(t, ioutil.WriteFile(fnjson, []byte(configEmbeddedJson), fileMode)) fntoml := dir + "/keto.toml" - assert.NoError(t, ioutil.WriteFile(fntoml, []byte(configEmbeddedToml), fileMode)) + require.NoError(t, ioutil.WriteFile(fntoml, []byte(configEmbeddedToml), fileMode)) stdOut := cmd.ExecNoErr(t, "validate", "-c", fnyaml+","+fnjson+","+fntoml) assert.Equal(t, "Congrats, all files are valid!\n", stdOut) @@ -64,25 +65,28 @@ func TestValidateConfigNamespaces(t *testing.T) { t.Run("case=config passed as varg fails", func(t *testing.T) { fn := t.TempDir() + "/keto.yaml" - assert.NoError(t, ioutil.WriteFile(fn, []byte(configEmbeddedYaml), fileMode)) + require.NoError(t, ioutil.WriteFile(fn, []byte(configEmbeddedYaml), fileMode)) - cmd.ExecExpectedErr(t, "validate", fn) + // interprets config file as namespace file when `-c` flag is not passed + stdOut := cmd.ExecExpectedErr(t, "validate", fn) + assert.Regexp(t, "additionalProperties ((\"namespaces\", \"dsn\")|(\"dsn\", \"namespaces\", \"dsn\")) not allowed", stdOut) }) t.Run("case=read config with invalid embedded namespace", func(t *testing.T) { fn := t.TempDir() + "/keto.yaml" - assert.NoError(t, ioutil.WriteFile(fn, []byte(`{"namespaces":[{"id":"x","name":"x"}]}`), fileMode)) + require.NoError(t, ioutil.WriteFile(fn, []byte(`{"namespaces":[{"id":"x","name":"x"}]}`), fileMode)) stdOut := cmd.ExecExpectedErr(t, "validate", "--config", fn) assert.Contains(t, stdOut, "not a valid namespace file") + assert.Contains(t, stdOut, "id:") }) t.Run("case=read config with namespace as dir reference", func(t *testing.T) { nsdir := t.TempDir() fn := t.TempDir() + "/config.yaml" - assert.NoError(t, ioutil.WriteFile(nsdir+"/ns0.yaml", []byte(nsyaml), fileMode)) - assert.NoError(t, ioutil.WriteFile(nsdir+"/ns1.json", []byte(nsjson), fileMode)) - assert.NoError(t, ioutil.WriteFile(fn, []byte(fmt.Sprintf(configReference, nsdir)), fileMode)) + require.NoError(t, ioutil.WriteFile(nsdir+"/ns0.yaml", []byte(nsyaml), fileMode)) + require.NoError(t, ioutil.WriteFile(nsdir+"/ns1.json", []byte(nsjson), fileMode)) + require.NoError(t, ioutil.WriteFile(fn, []byte(fmt.Sprintf(configReference, nsdir)), fileMode)) cmd.PersistentArgs = []string{} stdOut := cmd.ExecNoErr(t, "validate", "-c", fn) @@ -95,8 +99,8 @@ func TestValidateConfigNamespaces(t *testing.T) { t.Run("case=read config with dir reference bad namespaces", func(t *testing.T) { nsdir := t.TempDir() fn := t.TempDir() + "/config.yaml" - assert.NoError(t, ioutil.WriteFile(nsdir+"/ns0.yaml", []byte("name: badId\nid: foo"), fileMode)) - assert.NoError(t, ioutil.WriteFile(fn, []byte(fmt.Sprintf(configReference, nsdir)), fileMode)) + require.NoError(t, ioutil.WriteFile(nsdir+"/ns0.yaml", []byte("name: badId\nid: foo"), fileMode)) + require.NoError(t, ioutil.WriteFile(fn, []byte(fmt.Sprintf(configReference, nsdir)), fileMode)) cmd.PersistentArgs = []string{} stdOut := cmd.ExecExpectedErr(t, "validate", "-c", fn) diff --git a/internal/driver/config/namespace_watcher.go b/internal/driver/config/namespace_watcher.go index 9ffdade2c..04c4c11e6 100644 --- a/internal/driver/config/namespace_watcher.go +++ b/internal/driver/config/namespace_watcher.go @@ -118,7 +118,7 @@ func eventHandler(ctx context.Context, nw *NamespaceWatcher, done <-chan int, in if n == nil { return } else if n.namespace == nil { - // parse failed + // parse failed, rolling back to previous working version if existing, ok := nw.namespaces[e.Source()]; ok { existing.Contents = n.Contents } else {