-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: JSON-LD parsing #755
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Sonarcloud is complaining about duplicate lines within the JSON-LD documents that we are caching. This is a false positive. |
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.
That's very good work! I have a couple of questions, but it looks like this is going in a good direction.
src/parser/contexts/inrupt.ts
Outdated
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 not sure I'm on board with the context being duplicated in the codebase. I'd have expected an allowlist of trusted remote contexts, but for them to still be loaded from the provided URL rather than hard-coded here.
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.
To me it depends on whether the resources that are fetched are subject to change.
If they are subject to change then I agree.
If they are not subject to change; which seems more likely to be the case given that they appear to have versioning within the URI itself, then I'm quite strongly of the view they should be cached. The reasons are as follows:
- Our library will not break if these links temporarily become unavailable.
- Our library will continue to work within locked down enviroments, offline environments and testing environments (notably we would have to mock these context whenever our library is used in jest unit tests with a mocked fetch function; which may be unexpected for consumers).
- Our library will incur a significant performance hit at parsing time as these URLs will need to be fetched after the VC body (noting we can ensure that this is only a problem with the first VC we fetch by caching these contexts for the length of a session).
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.
It is subject to some change, but at a very slow rate. For instance, the inherit
property (with the urn) wasn't initially present. Are there caching options in the library, so that local storage is used to cache contexts once fetched so that they don't need to be retrieved too often?
I absolutely agree that they should be cached somehow, my question is more on version control being used as a cache.
Superseded by #849 |
This PR add support for parsing responses for parsing JSON-LD responses and returning the data as a
DatasetCore
as well as a framed JSON-LD document.Note that this will be a breaking change as there are several access-grant specific context which we do not cache here and instead cache in the access-grants library (see here for the additional context that are needed). In that regard it is also debateable that the inrupt context should also be moved over to the access grants library.