Validate content of folder items #41
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| package validator | ||
|
|
||
| import ( | ||
| "github.com/creasty/defaults" | ||
| "github.com/pkg/errors" | ||
| ) | ||
|
|
||
| type commonSpec struct { | ||
| AdditionalContents bool `yaml:"additionalContents"` | ||
| Contents []folderItemSpec `yaml:"contents"` | ||
| } | ||
|
|
||
|
|
||
| func setDefaultValues(spec *commonSpec) error { | ||
| err := defaults.Set(spec) | ||
| if err != nil { | ||
| return errors.Wrap(err, "could not set default values") | ||
| } | ||
|
|
||
| if len(spec.Contents) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| for i := range spec.Contents { | ||
| err = setDefaultValues(&spec.Contents[i].commonSpec) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
| return nil | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,166 @@ | ||
| package validator | ||
|
|
||
| import ( | ||
| "encoding/json" | ||
| "fmt" | ||
| "gopkg.in/yaml.v3" | ||
| "io/ioutil" | ||
| "net/http" | ||
| "os" | ||
| "path/filepath" | ||
| "regexp" | ||
|
|
||
| "github.com/pkg/errors" | ||
| "github.com/xeipuuv/gojsonschema" | ||
| ) | ||
|
|
||
| type folderItemSpec struct { | ||
| Description string `yaml:"description"` | ||
| ItemType string `yaml:"type"` | ||
| ContentMediaType string `yaml:"contentMediaType"` | ||
| Name string `yaml:"name"` | ||
| Pattern string `yaml:"pattern"` | ||
| Required bool `yaml:"required"` | ||
| Ref string `yaml:"$ref"` | ||
| Visibility string `yaml:"visibility" default:"public"` | ||
| commonSpec `yaml:",inline"` | ||
| } | ||
|
|
||
| type itemSchemaSpec struct { | ||
| Spec map[string]interface{} `json:"spec" yaml:"spec"` | ||
| } | ||
|
|
||
| func (s *folderItemSpec) matchingFileExists(files []os.FileInfo) (bool, error) { | ||
| if s.Name != "" { | ||
| for _, file := range files { | ||
| if file.Name() == s.Name { | ||
| return s.isSameType(file), nil | ||
| } | ||
| } | ||
| } else if s.Pattern != "" { | ||
| for _, file := range files { | ||
| isMatch, err := regexp.MatchString(s.Pattern, file.Name()) | ||
| if err != nil { | ||
| return false, errors.Wrap(err, "invalid folder item spec pattern") | ||
| } | ||
| if isMatch { | ||
| return s.isSameType(file), nil | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return false, nil | ||
| } | ||
|
|
||
| func (s *folderItemSpec) isSameType(file os.FileInfo) bool { | ||
| switch s.ItemType { | ||
| case itemTypeFile: | ||
| return !file.IsDir() | ||
| case itemTypeFolder: | ||
| return file.IsDir() | ||
| } | ||
|
|
||
| return false | ||
| } | ||
|
|
||
| func (s *folderItemSpec) validate(fs http.FileSystem, folderSpecPath string, itemPath string) ValidationErrors { | ||
| if s.Ref == "" { | ||
| return nil // no item's schema defined | ||
| } | ||
|
|
||
| schemaPath := filepath.Join(filepath.Dir(folderSpecPath), s.Ref) | ||
| schemaData, err := loadItemSchema(fs, schemaPath) | ||
| if err != nil { | ||
| return ValidationErrors{errors.Wrapf(err, "loading item schema failed (path %s)", schemaPath)} | ||
| } | ||
|
|
||
| // loading item content | ||
| itemData, err := loadItemContent(itemPath, s.ContentMediaType) | ||
| if err != nil { | ||
| return ValidationErrors{errors.Wrapf(err, "loading item content failed (path %s)", itemPath)} | ||
| } | ||
|
|
||
| // validation with schema | ||
| errs := validateData(schemaData, itemData) | ||
| if errs != nil { | ||
| return errs | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func loadItemSchema(fs http.FileSystem, itemSchemaPath string) ([]byte, error) { | ||
| itemSchemaFile, err := fs.Open(itemSchemaPath) | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "opening schema file failed") | ||
| } | ||
| defer itemSchemaFile.Close() | ||
|
|
||
| itemSchemaData, err := ioutil.ReadAll(itemSchemaFile) | ||
ycombinator marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if err != nil { | ||
| return nil, errors.Wrap(err, "reading schema file failed") | ||
| } | ||
|
|
||
| if len(itemSchemaData) == 0 { | ||
| return nil, errors.New("schema file is empty") | ||
| } | ||
|
|
||
| var schema itemSchemaSpec | ||
| err = yaml.Unmarshal(itemSchemaData, &schema) | ||
| if err != nil { | ||
| return nil, errors.Wrapf(err, "schema unmarshalling failed (path: %s)", itemSchemaPath) | ||
| } | ||
|
|
||
| schemaData, err := json.Marshal(&schema.Spec) | ||
|
There was a problem hiding this comment. Not for this PR but we could probably convert the schema YAML files to JSON as part of the There was a problem hiding this comment. Yes, this could be an improvement in the future (convertion is always error prone), although I admin that YAML here is much more human readable and easier to interact. There was a problem hiding this comment. Indeed, which is why we'd keep the source spec files in YAML but only convert them to JSON when |
||
| if err != nil { | ||
| return nil, errors.Wrapf(err, "marshalling schema to JSON format failed") | ||
| } | ||
| return schemaData, nil | ||
| } | ||
|
|
||
| func loadItemContent(itemPath, mediaType string) ([]byte, error) { | ||
| itemData, err := ioutil.ReadFile(itemPath) | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "reading item file failed") | ||
| } | ||
|
|
||
| if len(itemData) == 0 { | ||
| return nil, errors.New("file is empty") | ||
| } | ||
|
|
||
| switch mediaType { | ||
| case "application/x-yaml": | ||
| var c interface{} | ||
| err = yaml.Unmarshal(itemData, &c) | ||
| if err != nil { | ||
| return nil, errors.Wrapf(err, "unmarshalling YAML file failed (path: %s)", itemPath) | ||
| } | ||
|
|
||
| itemData, err = json.Marshal(&c) | ||
| if err != nil { | ||
| return nil, errors.Wrapf(err, "converting YAML file to JSON failed (path: %s)", itemPath) | ||
| } | ||
| case "application/json": // no need to convert the item content | ||
| default: | ||
| return nil, fmt.Errorf("unsupported media type (%s)", mediaType) | ||
| } | ||
| return itemData, nil | ||
| } | ||
|
|
||
| func validateData(schemaData, itemData []byte) ValidationErrors { | ||
| schemaLoader := gojsonschema.NewBytesLoader(schemaData) | ||
| documentLoader := gojsonschema.NewBytesLoader(itemData) | ||
| result, err := gojsonschema.Validate(schemaLoader, documentLoader) | ||
| if err != nil { | ||
| return ValidationErrors{err} | ||
| } | ||
|
|
||
| if result.Valid() { | ||
| return nil // item content is valid according to the loaded schema | ||
| } | ||
|
|
||
| var errs ValidationErrors | ||
| for _, re := range result.Errors() { | ||
| errs = append(errs, fmt.Errorf("field %s: %s", re.Field(), re.Description())) | ||
| } | ||
| return errs | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Empty file.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| - name: source | ||
| title: Source | ||
| group: 2 | ||
| type: group | ||
| fields: | ||
| - name: geo.city_name | ||
| level: core | ||
| type: keyword | ||
| description: City name. | ||
| ignore_above: 1024 | ||
| - name: geo.location | ||
| level: core | ||
| type: geo_point | ||
| description: Longitude and latitude. | ||
| - name: geo.region_iso_code | ||
| level: core | ||
| type: keyword | ||
| description: Region ISO code. | ||
| ignore_above: 1024 | ||
| - name: geo.region_name | ||
| level: core | ||
| type: keyword | ||
| description: Region name. | ||
| ignore_above: 1024 |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
What is the schema is defined inline and not referenced via
$ref?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.
not sure if I follow you...
this might be bad wording, should be: schema reference not provided. Does it sound better?
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.
Sorry, I was referring to needing something like this: https://github.com/elastic/package-spec/blob/master/code/go/internal/validator/folder.go#L125-L141. Maybe this is not the right place for it but then it should be added where this method is called from.
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.
Ah, now I see... I totally missed the case where schema is defined inline, didn't know about this. I will improve the implementation.
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.
@ycombinator Hm.. I dived into this topic and got a bit confused. Do we really need it? What would a use case for this? This
$refrefers to a place in folder schema in which we refer to item's schema. It's not part of the JSON-schema, so I believe we can enforce to keep it in a separate file (to separate concerns and prevent from creating a huge all-in-one files).tl;dr
$refin folder schema is always external.WDYT?
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.
So in the folder schema, there can be two
types of items:folder(for describing the structure of a sub-folder within the folder) andfile(for describing the structure of a file within the folder).For
type: folder, we already have a couple of places in the spec where use$ref(example) and a couple other places where we inline the definition instead of using$ref(example).For
type: file, I checked, and at the moment we only have places where we use$ref(example). We do not yet have any places where we are inlining the spec (aka schema) for the file itself. However, I can see this being useful for simple file specs (example).So I think there is a use case for it and we should support it.
Also, if we allowing inline specs for sub-folder items but not for file items, we are introducing an inconsistency. We could, of course, resolve this inconsistency by going the other way: that is, we can say that we only allow
$refs for item specs, regardless of whether those are folder items or file items; that we do not accept inline specs for either item type. I think this is slightly inconvenient when the item has a small spec but I'd be okay with this approach too. If you prefer this, then let's make a separate PR to first remove support for inline specs fortype: folderitems and adjust the spec files accordingly.