-
Notifications
You must be signed in to change notification settings - Fork 26
fix: Spec unmarshalling now supports defaults and validation #181
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
Changes from all commits
7636ec7
17c5525
ceb9927
dc44a08
6cb925f
09a6b9f
b7b4093
15f2bae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,9 +30,6 @@ func (d *Destination) SetDefaults() { | |
| if d.Path == "" { | ||
| d.Path = d.Name | ||
| } | ||
| if d.Version == "" { | ||
| d.Version = "latest" | ||
| } | ||
| if d.Registry == RegistryGithub && !strings.Contains(d.Path, "/") { | ||
| d.Path = "cloudquery/" + d.Path | ||
| } | ||
|
|
@@ -43,10 +40,23 @@ func (d *Destination) UnmarshalSpec(out interface{}) error { | |
| if err != nil { | ||
| return err | ||
| } | ||
| dec := json.NewDecoder(nil) | ||
| dec := json.NewDecoder(bytes.NewReader(b)) | ||
| dec.UseNumber() | ||
| dec.DisallowUnknownFields() | ||
| return json.Unmarshal(b, out) | ||
| return dec.Decode(out) | ||
| } | ||
|
|
||
| func (d *Destination) Validate() error { | ||
| if d.Name == "" { | ||
| return fmt.Errorf("name is required") | ||
| } | ||
| if d.Version == "" { | ||
| return fmt.Errorf("version is required") | ||
| } | ||
| if !strings.HasPrefix(d.Version, "v") { | ||
| return fmt.Errorf("version must start with v") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then change this error message to |
||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (m WriteMode) String() string { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,8 @@ package specs | |
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/google/go-cmp/cmp" | ||
| ) | ||
|
|
||
| type testDestinationSpec struct { | ||
|
|
@@ -24,23 +26,7 @@ func TestWriteModeFromString(t *testing.T) { | |
| } | ||
| } | ||
|
|
||
| func TestDestinationSetDefaults(t *testing.T) { | ||
| destination := Destination{ | ||
| Name: "testDestination", | ||
| } | ||
| destination.SetDefaults() | ||
| if destination.Registry != RegistryGithub { | ||
| t.Fatalf("expected RegistryGithub, got %v", destination.Registry) | ||
| } | ||
| if destination.Path != "cloudquery/testDestination" { | ||
| t.Fatalf("expected destination.Path (%s), got %s", destination.Name, destination.Path) | ||
| } | ||
| if destination.Version != "latest" { | ||
| t.Fatalf("expected latest, got %s", destination.Version) | ||
| } | ||
| } | ||
|
|
||
| func TestDestinationUnmarshalSpec(t *testing.T) { | ||
| func TestDestinationSpecUnmarshalSpec(t *testing.T) { | ||
| destination := Destination{ | ||
| Spec: map[string]interface{}{ | ||
| "connection_string": "postgres://user:pass@host:port/db", | ||
|
|
@@ -54,3 +40,137 @@ func TestDestinationUnmarshalSpec(t *testing.T) { | |
| t.Fatalf("expected postgres://user:pass@host:port/db, got %s", spec.ConnectionString) | ||
| } | ||
| } | ||
|
|
||
| var destinationUnmarshalSpecTestCases = []struct { | ||
| name string | ||
| spec string | ||
| err string | ||
| source *Source | ||
| }{ | ||
| { | ||
| "invalid_kind", | ||
| `kind: nice`, | ||
| "failed to decode spec: unknown kind nice", | ||
| nil, | ||
|
Comment on lines
+51
to
+54
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: let's add keys to these struct instantiations
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel for this test driven use-case it reduces the boilerplate imo and makes it easier to add new tests. just opinion ofc.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense, though as someone reading it for the first time, I found it hard to know what all these parameters were for, and had to scroll up and down a few times. I think it improves readability if you have the keys right there |
||
| }, | ||
| { | ||
| "invalid_type", | ||
| `kind: source | ||
| spec: | ||
| name: 3 | ||
| `, | ||
| "failed to decode spec: json: cannot unmarshal number into Go struct field Source.name of type string", | ||
| &Source{ | ||
| Name: "test", | ||
| Tables: []string{"*"}, | ||
| }, | ||
| }, | ||
| { | ||
| "unknown_field", | ||
| `kind: source | ||
| spec: | ||
| namea: 3 | ||
| `, | ||
| `failed to decode spec: json: unknown field "namea"`, | ||
| &Source{ | ||
| Name: "test", | ||
| Tables: []string{"*"}, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| func TestDestinationUnmarshalSpec(t *testing.T) { | ||
| for _, tc := range destinationUnmarshalSpecTestCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| var err error | ||
| var spec Spec | ||
| err = SpecUnmarshalYamlStrict([]byte(tc.spec), &spec) | ||
| if err != nil { | ||
| if err.Error() != tc.err { | ||
| t.Fatalf("expected:%s got:%s", tc.err, err.Error()) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| source := spec.Spec.(*Source) | ||
| if cmp.Diff(source, tc.source) != "" { | ||
| t.Fatalf("expected:%v got:%v", tc.source, source) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| var destinationUnmarshalSpecValidateTestCases = []struct { | ||
| name string | ||
| spec string | ||
| err string | ||
| destination *Destination | ||
| }{ | ||
| { | ||
| "required_name", | ||
| `kind: destination | ||
| spec:`, | ||
| "name is required", | ||
| nil, | ||
| }, | ||
| { | ||
| "required_version", | ||
| `kind: destination | ||
| spec: | ||
| name: test | ||
| `, | ||
| "version is required", | ||
| nil, | ||
| }, | ||
| { | ||
| "required_version_format", | ||
| `kind: destination | ||
| spec: | ||
| name: test | ||
| version: 1.1.0 | ||
| `, | ||
| "version must start with v", | ||
| nil, | ||
| }, | ||
| { | ||
| "success", | ||
| `kind: destination | ||
| spec: | ||
| name: test | ||
| version: v1.1.0 | ||
| `, | ||
| "", | ||
| &Destination{ | ||
| Name: "test", | ||
| Registry: RegistryGithub, | ||
| Path: "cloudquery/test", | ||
| Version: "v1.1.0", | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| func TestDestinationUnmarshalSpecValidate(t *testing.T) { | ||
| for _, tc := range destinationUnmarshalSpecValidateTestCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| var err error | ||
| var spec Spec | ||
| err = SpecUnmarshalYamlStrict([]byte(tc.spec), &spec) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| destination := spec.Spec.(*Destination) | ||
| destination.SetDefaults() | ||
| err = destination.Validate() | ||
| if err != nil { | ||
| if err.Error() != tc.err { | ||
| t.Fatalf("expected:%s got:%s", tc.err, err.Error()) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| if cmp.Diff(destination, tc.destination) != "" { | ||
| t.Fatalf("expected:%v got:%v", tc.destination, destination) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,10 @@ | ||
| package specs | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "encoding/json" | ||
| "fmt" | ||
| "strings" | ||
|
|
||
| "github.com/xeipuuv/gojsonschema" | ||
| ) | ||
|
|
||
| // Source is the spec for a source plugin | ||
|
|
@@ -40,12 +40,12 @@ func (s *Source) SetDefaults() { | |
| if s.Path == "" { | ||
| s.Path = s.Name | ||
| } | ||
| if s.Version == "" { | ||
| s.Version = "latest" | ||
| } | ||
| if s.Registry == RegistryGithub && !strings.Contains(s.Path, "/") { | ||
| s.Path = "cloudquery/" + s.Path | ||
| } | ||
| if s.Tables == nil { | ||
| s.Tables = []string{"*"} | ||
| } | ||
| } | ||
|
|
||
| // UnmarshalSpec unmarshals the internal spec into the given interface | ||
|
|
@@ -54,12 +54,24 @@ func (s *Source) UnmarshalSpec(out interface{}) error { | |
| if err != nil { | ||
| return err | ||
| } | ||
| dec := json.NewDecoder(nil) | ||
| dec := json.NewDecoder(bytes.NewReader(b)) | ||
| dec.UseNumber() | ||
| dec.DisallowUnknownFields() | ||
| return json.Unmarshal(b, out) | ||
| return dec.Decode(out) | ||
| } | ||
|
|
||
| func (*Source) Validate() (*gojsonschema.Result, error) { | ||
| return nil, nil | ||
| func (s *Source) Validate() error { | ||
| if s.Name == "" { | ||
| return fmt.Errorf("name is required") | ||
| } | ||
| if s.Version == "" { | ||
| return fmt.Errorf("version is required") | ||
| } | ||
| if !strings.HasPrefix(s.Version, "v") { | ||
| return fmt.Errorf("version must start with v") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we not support
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nope. per cloudquery/cloudquery#2054 . I think it's the right approach such as with any other package managers ( |
||
| } | ||
| if len(s.Destinations) == 0 { | ||
| return fmt.Errorf("at least one destination is required") | ||
| } | ||
| return nil | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, we should do full SemVer validation as the following string
vv1.0.0passes this validation