Validate JSON properties with relative-path format #83

mtojek · 2020-11-16T19:19:38Z

We decided to proceed with mutex and FormatCheckers. I've adjusted the PR. The solution should thread-safe (but uses locking).

~~I'm opening this as draft to discuss possible design ideas.~~

Resolves: #31

I found FormatCheckers in gojsonschema that could be used to verify properties with relative paths (screenshot and image src). Unfortunately the FormatCheckers is a global object, not thread-safe and can be applied only to statics.

For reference:
xeipuuv/gojsonschema#257
xeipuuv/gojsonschema#300

I'm wondering whether we should guard the piece of code with Validate() and refreshFormatCheckersContext with a mutex or should we use a different approach:

Package domain-aware validation (screenshots and images have sources). So far we didn't need to strongly depend on package domain and it would be nice not to follow this path
Fork the package and modify the existing JSON schema draft (to support type: string & subType: relative-path.
See:
http://json-schema.org/draft/2019-09/json-schema-validation.html#rfc.section.7
http://json-schema.org/draft/2019-09/release-notes.html#format-vocabulary
Depend on fork: Local Format Checker xeipuuv/gojsonschema#256

My opinion:

I wouldn't like to implement the new entire schema loader that focuses only on sub-type (path). KISS approach: define type-based Go validation functions (compiled into the validator). So far we need to apply them only to package manifests (Image type).

elasticmachine · 2020-11-16T19:21:56Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Build Cause: [Pull request #83 updated]
Start Time: 2020-11-19T08:10:08.385+0000
Duration: 2 min 13 sec

ycombinator · 2020-11-17T01:51:07Z

Yeah, I'm in favor of KISS. What you have here is fine for now (I realize it's not threadsafe). We can worry about that when it starts to become an issue.

ycombinator · 2020-11-17T01:55:06Z

code/go/internal/validator/folder_item_spec.go

@@ -93,3 +94,9 @@ func (s *folderItemSpec) validate(fs http.FileSystem, folderSpecPath string, ite
 	}
 	return errs
 }
+
+func refreshFormatCheckersContext(currentPath string) {


Given that this is a global instruction, should we just move it into init to make that more explicit/obvious? It will also guarantee that the relative-path format checker is initialized exactly once at package initialization time.

Let me describe the case here:

The method refreshFormatCheckersContext is called just before calling Validate to modify the global state every time it's executed against a package, because there is no way to sneak a context (with a package path).

Also, we start depending on the format property, which is not the exact case here as we don't verify the format only, but also checks the existing path. In case of an error the library fails with a generic error that the format is not correct.

Ah yes, of course! I totally missed the currentPath argument 🤦.

Okay, I understand what's going on here now and why you have to do it this way. This is good as-is but I made a related comment at the location where this function is being called from.

ycombinator · 2020-11-17T08:36:51Z

code/go/internal/validator/folder_item_spec.go

@@ -78,6 +78,7 @@ func (s *folderItemSpec) validate(fs http.FileSystem, folderSpecPath string, ite

 	// validation with schema
 	documentLoader := gojsonschema.NewBytesLoader(itemData)
+	refreshFormatCheckersContext(filepath.Dir(itemPath))


Given that we are essentially creating a new custom format checker for every filepath.Dir(itemPath), perhaps we should also remove this format checker using defer gojsonschema.FormatCheckers.Remove(...)?

Additionally I would add a comment here noting that this call is not thread-safe so, in the future, if we need to introduce concurrency within a single elastic-package lint invocation, we can add mutexes here or something like that.

I can add a mutex to surround this place, but it's just mitigation.

Just wanted to make sure that we understand the risk and consequences here (incl. error messages).

Up to you if you want to add the mutex now or later. I don't think it's going to make much of a performance difference to have it now itself but if you want to KISS feel free to leave it out until we need it.

Ok, it wasn't hard to add the mutex, so I did it. I've also adjusted the error message.

ycombinator · 2020-11-18T06:01:04Z

code/go/internal/validator/folder_item_spec.go

 	}
 	return errs
 }
+
+func loadRelativePathFormatChecker(currentPath string) {
+	gojsonschema.FormatCheckers.Add("relative-path", RelativePathChecker{


Consider making relative-path a constant, now that we are using it in a few places in code.

ycombinator · 2020-11-18T06:03:05Z

versions/1/manifest.spec.yml

@@ -114,6 +114,7 @@ spec:
          src:
            description: Relative path to the icon's image file.
            type: string
+            format: relative-path


This should be documented in CONTRIBUTING.md for anyone who wants to add elements to the spec in the future that could use relative-path validation.

ycombinator · 2020-11-18T11:47:35Z

CONTRIBUTING.md

+Currently, the following custom formats are available:
+
+* `relative-path`: Relative path to the package root directory. The format checker verifies if the path is correct and


Is it relative to the package root directory or is it relative to where the folder item spec file is defined?

It is relative to the package root (as noted).

ycombinator

LGTM.

Validate JSON properties with relative-path format

1ad4fc0

mtojek requested a review from ycombinator November 16, 2020 19:20

mtojek marked this pull request as draft November 16, 2020 19:20

ycombinator reviewed Nov 17, 2020

View reviewed changes

Validate image sources

384d577

mtojek self-assigned this Nov 17, 2020

mtojek requested a review from ycombinator November 17, 2020 11:59

mtojek marked this pull request as ready for review November 17, 2020 11:59

mtojek mentioned this pull request Nov 17, 2020

Validate that Kibana asset IDs start with lowercase package names #65

Closed

3 tasks

ycombinator reviewed Nov 18, 2020

View reviewed changes

Address PR comments

fb706be

mtojek requested a review from ycombinator November 18, 2020 11:07

Merge branch 'master' into 31-validate-icon-scr

760bff5

ycombinator reviewed Nov 18, 2020

View reviewed changes

mtojek requested a review from ycombinator November 18, 2020 11:59

ycombinator approved these changes Nov 19, 2020

View reviewed changes

Merge branch 'master' into 31-validate-icon-scr

82ffdf5

mtojek merged commit 49ecd87 into elastic:master Nov 19, 2020

rw-access pushed a commit to rw-access/package-spec that referenced this pull request Mar 23, 2021

Enable go-licenser (elastic#83)

5f815c0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate JSON properties with relative-path format #83

Validate JSON properties with relative-path format #83

mtojek commented Nov 16, 2020 •

edited

Loading

elasticmachine commented Nov 16, 2020 •

edited

Loading

Build stats

ycombinator commented Nov 17, 2020

ycombinator Nov 17, 2020

mtojek Nov 17, 2020 •

edited

Loading

ycombinator Nov 17, 2020

ycombinator Nov 17, 2020

mtojek Nov 17, 2020

ycombinator Nov 17, 2020

mtojek Nov 17, 2020

ycombinator Nov 18, 2020 •

edited

Loading

mtojek Nov 18, 2020

ycombinator Nov 18, 2020

mtojek Nov 18, 2020

ycombinator Nov 18, 2020

mtojek Nov 18, 2020

ycombinator left a comment

		Currently, the following custom formats are available:

		* `relative-path`: Relative path to the package root directory. The format checker verifies if the path is correct and

Validate JSON properties with relative-path format #83

Validate JSON properties with relative-path format #83

Conversation

mtojek commented Nov 16, 2020 • edited Loading

elasticmachine commented Nov 16, 2020 • edited Loading

💚 Build Succeeded

Build stats

ycombinator commented Nov 17, 2020

Choose a reason for hiding this comment

mtojek Nov 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ycombinator Nov 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ycombinator left a comment

Choose a reason for hiding this comment

mtojek commented Nov 16, 2020 •

edited

Loading

elasticmachine commented Nov 16, 2020 •

edited

Loading

mtojek Nov 17, 2020 •

edited

Loading

ycombinator Nov 18, 2020 •

edited

Loading