Skip to content

Commit

Permalink
cleanup code review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@cramja
cramja committed Jun 17, 2021
1 parent 5ab85f9 commit 68978c5
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 23 deletions.
22 changes: 11 additions & 11 deletions cmd/namespace/validate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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)
Expand Down Expand Up @@ -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) {
Expand All @@ -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
}
Expand All @@ -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)
Expand All @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
26 changes: 15 additions & 11 deletions cmd/namespace/validate_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -50,39 +51,42 @@ 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)
})

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)
Expand All @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion internal/driver/config/namespace_watcher.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down

0 comments on commit 68978c5

Please sign in to comment.