Some inline comments on areas that can use improvement for 1.1, focusing on expansion and context/term processing.

gkellogg · gkellogg · commit 53aa216e8e67 · 2020-04-09T10:43:47.000-07:00
diff --git a/core/src/main/java/com/github/jsonldjava/core/Context.java b/core/src/main/java/com/github/jsonldjava/core/Context.java
@@ -163,13 +163,15 @@ public Context parse(Object localContext, List<String> remoteContexts) throws Js
      * @throws JsonLdError
      *             If there is an error parsing the contexts.
      */
+    // GK: Note that parsing may also depend on some options: `override protected and `propagate`
     private Context parse(Object localContext, List<String> remoteContexts,
             boolean parsingARemoteContext) throws JsonLdError {
         if (remoteContexts == null) {
             remoteContexts = new ArrayList<String>();
         }
         // 1. Initialize result to the result of cloning active context.
         Context result = this.clone(); // TODO: clone?
+        // GK: note if localContext is a Map containing `@propagate` that value overrides the `propagate` option.
         // 2)
         if (!(localContext instanceof List)) {
             final Object temp = localContext;
@@ -180,6 +182,8 @@ private Context parse(Object localContext, List<String> remoteContexts,
         for (final Object context : ((List<Object>) localContext)) {
             // 3.1)
             if (context == null) {
+                // GK: Note, if active context has any protected terms, and `override protected` is not true, this should fail with 'invalid context nullification'.
+                // GK: Note, if `propagate` is false, the previous context should be associated with this new (null) context for potential rollback.
                 result = new Context(this.options);
                 continue;
             } else if (context instanceof Context) {
@@ -188,6 +192,7 @@ private Context parse(Object localContext, List<String> remoteContexts,
             // 3.2)
             else if (context instanceof String) {
                 String uri = (String) result.get(JsonLdConsts.BASE);
+                // GK: Note, the context needs to be resolved against the location of the file containing the reference, not the base association from the context. The spec defines a `context base` for this purpose.
                 uri = JsonLdUrl.resolve(uri, (String) context);
                 // 3.2.2
                 if (remoteContexts.contains(uri)) {
@@ -273,6 +278,8 @@ else if (context instanceof String) {
                 }
             }
 
+            // GK: There are more keys to be checked: `@import`, `@direction`, `@propagate` and `@version`.
+            // GK: You'll want some `processingMode` method to use when doing conditional checks; default value is `json-ld-1.1`, but can be overridden using an API option.
             // 3.7
             final Map<String, Boolean> defined = new LinkedHashMap<String, Boolean>();
             for (final String key : ((Map<String, Object>) context).keySet()) {
@@ -321,12 +328,14 @@ private void createTermDefinition(Map<String, Object> context, String term,
 
         defined.put(term, false);
 
+        // GK: Note, `@type` can also contain `@protected` in addition to `@container`. If `@container` is there, its value can only be `@set` (or `['@set']`).
         if (JsonLdUtils.isKeyword(term)
                 && !(options.getAllowContainerSetOnType() && JsonLdConsts.TYPE.equals(term)
                         && !(context.get(term)).toString().contains(JsonLdConsts.ID))) {
             throw new JsonLdError(Error.KEYWORD_REDEFINITION, term);
         }
 
+        // GK: Note, you'll need to retain any previous definition to make sure, if protected, that any new definition is compatible with it before ending this method.
         this.termDefinitions.remove(term);
         Object value = context.get(term);
         if (value == null || (value instanceof Map
@@ -415,6 +424,7 @@ private void createTermDefinition(Map<String, Object> context, String term,
         definition.put(JsonLdConsts.REVERSE, false);
 
         // 13)
+        // GK: Note, there are some required checks to be sure that if the associated term expands to an IRI, it is compatible with `@id` and some other checks.
         if (val.get(JsonLdConsts.ID) != null && !term.equals(val.get(JsonLdConsts.ID))) {
             if (!(val.get(JsonLdConsts.ID) instanceof String)) {
                 throw new JsonLdError(Error.INVALID_IRI_MAPPING,
@@ -458,6 +468,7 @@ else if (term.indexOf(":") >= 0) {
         }
 
         // 16)
+        // GK: Note, `@container` can take on many more values, and be an array. Best always cast to an array and check to see if the container includes any useful value. There are also some checks to make sure that the content of `@context` is consistent.
         if (val.containsKey(JsonLdConsts.CONTAINER)) {
             final String container = (String) val.get(JsonLdConsts.CONTAINER);
             if (!JsonLdConsts.LIST.equals(container) && !JsonLdConsts.SET.equals(container)
@@ -485,6 +496,8 @@ else if (term.indexOf(":") >= 0) {
             }
         }
 
+        // GK: Note, other keys to check for are `@index`, `@context` (which requires a recursive call to Context.parse to make sure it's valid), `@direction`, `@nest`, and `@prefix`.
+        // GK: Note, this is where to check if the previous definition exists and is protected, and we're not overriding protected, that the two definitions are essentially compatible.
         // 18)
         this.termDefinitions.put(term, definition);
         defined.put(term, true);
@@ -588,6 +601,7 @@ String compactIri(String iri, Object value, boolean relativeToVocab, boolean rev
 
         // 2)
         if (relativeToVocab && getInverse().containsKey(iri)) {
+            // GK: Sadly, term selection has become much more involved in 1.1.
             // 2.1)
             String defaultLanguage = (String) this.get(JsonLdConsts.LANGUAGE);
             if (defaultLanguage == null) {
diff --git a/core/src/main/java/com/github/jsonldjava/core/JsonLdApi.java b/core/src/main/java/com/github/jsonldjava/core/JsonLdApi.java
@@ -514,6 +514,7 @@ public Object expand(Context activeCtx, String activeProperty, Object element)
             return null;
         }
 
+        // GK: This would be the point to set `propertyScopedContext` to the `@context` entry for any term definition associated with `activeProperty`.
         // 3)
         if (element instanceof List) {
             // 3.1)
@@ -546,14 +547,19 @@ else if (element instanceof Map) {
             // access helper
             final Map<String, Object> elem = (Map<String, Object>) element;
             // 5)
+            // This would be the place to revert the active context from any previous type-scoped context if the active context has a `previousContext` entry (with some exceptions when called from a map, or if it's a value object or a subject reference).
+            // GK: If we found a `propertyScopedContext` above, we can parse it to create a new activeCtx using the `override protected` option
             if (elem.containsKey(JsonLdConsts.CONTEXT)) {
                 activeCtx = activeCtx.parse(elem.get(JsonLdConsts.CONTEXT));
             }
+            // GK: This would be the place to remember this version of activeCtx as `typeScopedContext`.
             // 6)
             Map<String, Object> result = newMap();
             // 7)
             final List<String> keys = new ArrayList<String>(elem.keySet());
             Collections.sort(keys);
+            // GK: This is the place to check for a type-scoped context by checking any key that expands to `@type` to see the current context has a term that equals that key where the term definition includes `@context`, updating the activeCtx as you go (but using termScopedContext when checking the keys).
+            // GK: 1.1 made the following loop somewhat recursive, due to nesting, so might want to extract into a method.
             for (final String key : keys) {
                 final Object value = elem.get(key);
                 // 7.1)
@@ -766,6 +772,7 @@ else if (JsonLdConsts.REVERSE.equals(expandedProperty)) {
                                 }
                             }
                         }
+                        // GK: Also, `@included`, `@graph`, and `@direction`
                         // 7.4.11.4)
                         continue;
                     }
@@ -815,9 +822,12 @@ else if (JsonLdConsts.LANGUAGE.equals(activeCtx.getContainer(key))
                     }
                 }
                 // 7.6)
+                // GK: Also a place to see if key is `@json` for JSON literals.
                 else if (JsonLdConsts.INDEX.equals(activeCtx.getContainer(key))
                         && value instanceof Map) {
                     // 7.6.1)
+                    // GK: `@index` also supports property indexing, if the term definition includes `@index`.
+                    // GK: A map can also include `@none`.
                     expandedValue = new ArrayList<Object>();
                     // 7.6.2)
                     final List<String> indexKeys = new ArrayList<String>(
@@ -865,6 +875,7 @@ else if (JsonLdConsts.INDEX.equals(activeCtx.getContainer(key))
                         ((Map<String, Object>) expandedValue).put(JsonLdConsts.LIST, tmp);
                     }
                 }
+                // GK: Other container possibilities including `@graph`, `@id`, and `@type` along with variations.
                 // 7.10)
                 if (activeCtx.isReverseProperty(key)) {
                     // 7.10.1)
diff --git a/core/src/main/java/com/github/jsonldjava/core/JsonLdUtils.java b/core/src/main/java/com/github/jsonldjava/core/JsonLdUtils.java
@@ -26,6 +26,7 @@ static boolean isKeyword(Object key) {
         if (!isString(key)) {
             return false;
         }
+        // GK: Note that this set is somewhat dependent on the processing mode.
         return "@base".equals(key) || "@context".equals(key) || "@container".equals(key)
                 || "@default".equals(key) || "@embed".equals(key) || "@explicit".equals(key)
                 || "@graph".equals(key) || "@id".equals(key) || "@index".equals(key)

Original file line number	Diff line number	Diff line change
`@@ -514,6 +514,7 @@ public Object expand(Context activeCtx, String activeProperty, Object element)`
`514`	`514`	`return null;`
`515`	`515`	`}`
`516`	`516`
	`517`	+ // GK: This would be the point to set `propertyScopedContext` to the `@context` entry for any term definition associated with `activeProperty`.
`517`	`518`	`// 3)`
`518`	`519`	`if (element instanceof List) {`
`519`	`520`	`// 3.1)`
`@@ -546,14 +547,19 @@ else if (element instanceof Map) {`
`546`	`547`	`// access helper`
`547`	`548`	`final Map<String, Object> elem = (Map<String, Object>) element;`
`548`	`549`	`// 5)`
	`550`	+ // This would be the place to revert the active context from any previous type-scoped context if the active context has a `previousContext` entry (with some exceptions when called from a map, or if it's a value object or a subject reference).
	`551`	+ // GK: If we found a `propertyScopedContext` above, we can parse it to create a new activeCtx using the `override protected` option
`549`	`552`	`if (elem.containsKey(JsonLdConsts.CONTEXT)) {`
`550`	`553`	`activeCtx = activeCtx.parse(elem.get(JsonLdConsts.CONTEXT));`
`551`	`554`	`}`
	`555`	+ // GK: This would be the place to remember this version of activeCtx as `typeScopedContext`.
`552`	`556`	`// 6)`
`553`	`557`	`Map<String, Object> result = newMap();`
`554`	`558`	`// 7)`
`555`	`559`	`final List<String> keys = new ArrayList<String>(elem.keySet());`
`556`	`560`	`Collections.sort(keys);`
	`561`	+ // GK: This is the place to check for a type-scoped context by checking any key that expands to `@type` to see the current context has a term that equals that key where the term definition includes `@context`, updating the activeCtx as you go (but using termScopedContext when checking the keys).
	`562`	`+ // GK: 1.1 made the following loop somewhat recursive, due to nesting, so might want to extract into a method.`
`557`	`563`	`for (final String key : keys) {`
`558`	`564`	`final Object value = elem.get(key);`
`559`	`565`	`// 7.1)`
`@@ -766,6 +772,7 @@ else if (JsonLdConsts.REVERSE.equals(expandedProperty)) {`
`766`	`772`	`}`
`767`	`773`	`}`
`768`	`774`	`}`
	`775`	+ // GK: Also, `@included`, `@graph`, and `@direction`
`769`	`776`	`// 7.4.11.4)`
`770`	`777`	`continue;`
`771`	`778`	`}`
`@@ -815,9 +822,12 @@ else if (JsonLdConsts.LANGUAGE.equals(activeCtx.getContainer(key))`
`815`	`822`	`}`
`816`	`823`	`}`
`817`	`824`	`// 7.6)`
	`825`	+ // GK: Also a place to see if key is `@json` for JSON literals.
`818`	`826`	`else if (JsonLdConsts.INDEX.equals(activeCtx.getContainer(key))`
`819`	`827`	`&& value instanceof Map) {`
`820`	`828`	`// 7.6.1)`
	`829`	+ // GK: `@index` also supports property indexing, if the term definition includes `@index`.
	`830`	+ // GK: A map can also include `@none`.
`821`	`831`	`expandedValue = new ArrayList<Object>();`
`822`	`832`	`// 7.6.2)`
`823`	`833`	`final List<String> indexKeys = new ArrayList<String>(`
`@@ -865,6 +875,7 @@ else if (JsonLdConsts.INDEX.equals(activeCtx.getContainer(key))`
`865`	`875`	`((Map<String, Object>) expandedValue).put(JsonLdConsts.LIST, tmp);`
`866`	`876`	`}`
`867`	`877`	`}`
	`878`	+ // GK: Other container possibilities including `@graph`, `@id`, and `@type` along with variations.
`868`	`879`	`// 7.10)`
`869`	`880`	`if (activeCtx.isReverseProperty(key)) {`
`870`	`881`	`// 7.10.1)`
Original file line number	Diff line number	Diff line change
`@@ -26,6 +26,7 @@ static boolean isKeyword(Object key) {`
`26`	`26`	`if (!isString(key)) {`
`27`	`27`	`return false;`
`28`	`28`	`}`
	`29`	`+ // GK: Note that this set is somewhat dependent on the processing mode.`
`29`	`30`	`return "@base".equals(key) \|\| "@context".equals(key) \|\| "@container".equals(key)`
`30`	`31`	`\|\| "@default".equals(key) \|\| "@embed".equals(key) \|\| "@explicit".equals(key)`
`31`	`32`	`\|\| "@graph".equals(key) \|\| "@id".equals(key) \|\| "@index".equals(key)`