-
Notifications
You must be signed in to change notification settings - Fork 54
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
Locally load schemas #269
Locally load schemas #269
Conversation
Codecov Report
@@ Coverage Diff @@
## main #269 +/- ##
==========================================
+ Coverage 60.18% 60.35% +0.18%
==========================================
Files 38 39 +1
Lines 4329 4399 +70
==========================================
+ Hits 2605 2655 +50
- Misses 1298 1315 +17
- Partials 426 429 +3
|
Tested by turning off wifi and running the tests 😉 |
@@ -2,17 +2,10 @@ | |||
"$schema": "http://json-schema.org/draft-07/schema#", |
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.
did everything else get deleted but not this file?
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.
Good job! We the way we had it before was so janky
@@ -2,17 +2,10 @@ | |||
"$schema": "http://json-schema.org/draft-07/schema#", | |||
"title": "Presentation Submission Claim Format Designations", | |||
"type": "object", | |||
"additionalProperties": false, | |||
"properties": { | |||
"definitions": { |
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's different about this schema that you didn't delete it? Is it because it's not published in the web?
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.
I didn't delete most schemas. I deleted the -deref.json
files. This schema actually had an update which I pulled in. cc: @nitro-neal
schema/jsonschema.go
Outdated
func init() { | ||
httploader.Client = &http.Client{ | ||
Timeout: time.Second * 10, | ||
} | ||
} | ||
|
||
const ( | ||
// defaultSchemaURL is a placeholder that's needed to load any schema | ||
defaultSchemaURL = "schema.json" |
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 I follow why this is needed, can you elaborate?
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.
the library requires you to provide a url for the schema and we don't really care what this is but it needs to be a placeholder https://github.com/santhosh-tekuri/jsonschema/blob/master/compiler.go#L65
schema/loader.go
Outdated
} | ||
|
||
// DisableLocalLoad disables the ability to load schemas locally, clearing current locally loaded schemas | ||
func DisableLocalLoad() { |
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.
Is this being used?
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.
no, but I believe it should remain as an option for users of the lib
schema/loader.go
Outdated
jsonschema.Loaders["https"] = func(url string) (io.ReadCloser, error) { | ||
schema, ok := localSchemas[strings.TrimPrefix(url, "https://")] | ||
if !ok { | ||
return nil, fmt.Errorf("%q not found", url) | ||
} | ||
return io.NopCloser(strings.NewReader(schema)), nil | ||
} | ||
jsonschema.Loaders["http"] = func(url string) (io.ReadCloser, error) { | ||
schema, ok := localSchemas[strings.TrimPrefix(url, "http://")] | ||
if !ok { | ||
return nil, fmt.Errorf("%q not found", url) | ||
} | ||
return io.NopCloser(strings.NewReader(schema)), 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.
I have a couple of suggestions for your consideration:
- Rework
LocalLoader
intoLocalLoaderFactory
. - Remove the
jsonschema.Loader
directives from this file, and theinit
functions (let users of the SDK do it themselves). - Provide a method for creating loaders, something like what's below.
type Loader func(url string) (io.ReadCloser, error)
func (l *LocalLoaderFactory) ProtocolLoader(protocol string) Loader {
return func(url string) (io.ReadCloser, error) {
schema, ok := localSchemas[strings.TrimPrefix(url, protocol + "://")]
if !ok {
return nil, fmt.Errorf("%q not found", url)
}
return io.NopCloser(strings.NewReader(schema)), nil
}
}
Once that is done, then user's of the SDK can decide where to override the jsonschema
loader.
// Somewhere in the ssi-service
loaderFactory := schema.NewLocalLoaderFactory()
jsonschema.Loader["http"] = loaderFactory.ProtocolLoader("http")
jsonschema.Loader["https"] = loaderFactory.ProtocolLoader("https")
Thoughts?
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.
I'm open to this but I think it's important we're able to enable local loading for CI. how do you think we should go about that?
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.
How about defining the TestMain (as described in this https://pkg.go.dev/testing#hdr-Main)? Within that function we can patch the jsonschema lib.
schema/loader.go
Outdated
} | ||
|
||
// AddCachedSchema adds a schema to be cached | ||
func (cl *CachingLoader) AddCachedSchema(schemaURI, schema string) error { |
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.
adjusted this to be a caching loader with a test. no more init.
schema/loader.go
Outdated
// NewCachingLoader returns a new CachingLoader that enables the ability to cache http and https schemas | ||
func NewCachingLoader() CachingLoader { | ||
localSchemas := make(map[string]string) | ||
jsonschema.Loaders["https"] = func(url string) (io.ReadCloser, error) { |
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.
I find it odd that a constructor has side effects. What's the rationale of patching jsonschema
within the constructor?
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.
most, if not all, schemas will be loaded by http/s. having users configure additional loaders leaks details of the library that they shouldn't know or need to care about. this loader will work fine for the 99%
"github.com/stretchr/testify/assert" | ||
) | ||
|
||
func TestCachingLoader(t *testing.T) { |
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.
Consider splitting this large test into multiple smaller unit tests that exercise a single behavior.
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.
split into 2
schema/loader.go
Outdated
func NewCachingLoader() CachingLoader { | ||
localSchemas := make(map[string]string) | ||
jsonschema.Loaders["https"] = func(url string) (io.ReadCloser, error) { | ||
schema, ok := localSchemas[strings.TrimPrefix(url, "https://")] |
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.
This might not be safe for concurrent use (and cause a panic), depending on how users of the SDK exercise it.
A goroutine might be reading from the map, while another one writes to the map by calling AddCachedSchema
.
Consider writing once to the map only, in the constructor. Concurrent reads on a map are safe.
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.
this constructor should only be called once - and I don't imagine callers will be further configuring the loaders...if they do then yes they may have concurrency issues. is this worth caring about?
jsonschema.Loaders["https"] = cl.cachingLoaderForProtocol("https") | ||
} | ||
|
||
func (cl *CachingLoader) cachingLoaderForProtocol(protocol string) func(url string) (io.ReadCloser, error) { |
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.
updated to lock on map writes.
} | ||
|
||
// read the contents and cache and prevent future lookups | ||
contents, err := io.ReadAll(loaded) |
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.
added cache on load from http
schema/loader.go
Outdated
} | ||
|
||
func (cl *CachingLoader) cachingLoaderForProtocol(protocol string) func(url string) (io.ReadCloser, error) { | ||
var m sync.Mutex |
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.
I think this should be a shared lock across all loaders (i.e. move this variable to be a member of CachingLoader
. As it stands, you can have a goroutine writing via a schema fetched from the http
protocol, while another goroutine reads a schema fetched from the https
protocol. Does this make sense?
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.
yes but isn't that fine, since they're different protocols?
schema/loader.go
Outdated
} | ||
|
||
func (cl *CachingLoader) cachingLoaderForProtocol(protocol string) func(url string) (io.ReadCloser, error) { | ||
var m sync.Mutex |
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.
Did you consider using a sync.RWMutex
in order to lock more granularly? It could increase read throughput (though that's a small optimization).
Co-authored-by: Andres Uribe <auribe@tbd.email>
Co-authored-by: Andres Uribe <auribe@tbd.email>
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.
This is awesome, I'm so excited about this change. Thanks for tackling!
schema/loader.go
Outdated
|
||
// NewCachingLoader returns a new CachingLoader that enables the ability to cache http and https schemas | ||
func NewCachingLoader(schemas map[string]string) (*CachingLoader, error) { | ||
// make sure only one process can write to the map at a time |
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: move this to be a comment for the schemas
function.
Co-authored-by: Andres Uribe <auribe@tbd.email>
Fixes #134
Don't think this will work for schemas that are not pre-defined. Meaning, if we're in local mode and try to load a http/s resource that's not defined it'll break. This should be fine...
It is 'turned on' by default with an
init()
block which isn't ideal. Could use some ideas to make this better.