Skip to content

Improve conformance of processingMode and converge on array representation of @container #210

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Jan 12, 2018
Merged

Improve conformance of processingMode and converge on array representation of @container #210

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
15cba77
- Set processingMode from options or first encountered context.
gkellogg Jan 8, 2018
c8f787b
Put processingMode on the context, rather than the API.
gkellogg Jan 8, 2018
d2c7649
Use the `&=` style, instead of single-line if, as it is more concise.
gkellogg Jan 9, 2018
34ea960
Don't be so clever when setting processingMode.
gkellogg Jan 9, 2018
1ae2616
Update based on style comments.
gkellogg Jan 9, 2018
6cd4fae
Include Expansion test 0088 in tests buy updating skip regexp.
gkellogg Jan 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@

### Fixed
- Do not use native types to create IRIs in value expansion.
- Improved error detection for @container variations.

### Changed
- Set processingMode from options or first encountered context.
- Use array representation of @container in processing.

## 0.5.15 - 2017-10-16

Expand Down
27 changes: 14 additions & 13 deletions lib/compact.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,8 @@ api.compact = ({
if(options.compactArrays && rval.length === 1) {
// use single element if no container is specified
const container = _getContextValue(
activeCtx, activeProperty, '@container');
if(container === null) {
activeCtx, activeProperty, '@container') || [];
if(container.length === 0) {
rval = rval[0];
}
}
Expand Down Expand Up @@ -182,8 +182,8 @@ api.compact = ({
if(activeCtx.mappings[compactedProperty] &&
activeCtx.mappings[compactedProperty].reverse) {
const value = compactedValue[compactedProperty];
const container = [].concat(
_getContextValue(activeCtx, compactedProperty, '@container'));
const container = _getContextValue(
activeCtx, compactedProperty, '@container') || [];
const useArray = (
container.includes('@set') || !options.compactArrays);
_addValue(
Expand All @@ -205,8 +205,8 @@ api.compact = ({
if(expandedProperty === '@index') {
// drop @index if inside an @index container
const container = _getContextValue(
activeCtx, activeProperty, '@container');
if(container === '@index') {
activeCtx, activeProperty, '@container') || [];
if(container.includes('@index')) {
continue;
}

Expand Down Expand Up @@ -257,8 +257,8 @@ api.compact = ({
relativeTo: {vocab: true},
reverse: insideReverse
});
const container = [].concat(
_getContextValue(activeCtx, itemActiveProperty, '@container'));
const container = _getContextValue(
activeCtx, itemActiveProperty, '@container') || [];

// get simple @graph or @list value if appropriate
const isSimpleGraph = _isSimpleGraph(expandedItem);
Expand Down Expand Up @@ -340,7 +340,8 @@ api.compact = ({

// add compact value to map object using key from expanded value
// based on the container type
_addValue(mapObject, expandedItem[container], compactedItem);
const c = container.includes('@language') ? '@language' : '@index';
_addValue(mapObject, expandedItem[c], compactedItem);
} else {
// use an array if: compactArrays flag is false,
// @container is @set or @list , value is an empty
Expand Down Expand Up @@ -587,10 +588,10 @@ api.compactValue = ({activeCtx, activeProperty, value}) => {
// get context rules
const type = _getContextValue(activeCtx, activeProperty, '@type');
const language = _getContextValue(activeCtx, activeProperty, '@language');
const container = _getContextValue(activeCtx, activeProperty, '@container');
const container = _getContextValue(activeCtx, activeProperty, '@container') || [];

// whether or not the value has an @index that must be preserved
const preserveIndex = '@index' in value && container !== '@index';
const preserveIndex = '@index' in value && !container.includes('@index');

// if there's no @index to preserve ...
if(!preserveIndex) {
Expand Down Expand Up @@ -717,9 +718,9 @@ api.removePreserve = (ctx, input, options) => {
// recurse through properties
for(let prop in input) {
let result = api.removePreserve(ctx, input[prop], options);
const container = _getContextValue(ctx, prop, '@container');
const container = _getContextValue(ctx, prop, '@container') || [];
if(options.compactArrays && _isArray(result) && result.length === 1 &&
container === null) {
container.length === 0) {
result = result[0];
}
input[prop] = result;
Expand Down
71 changes: 46 additions & 25 deletions lib/context.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,10 +97,20 @@ api.process = ({activeCtx, localCtx, options}) => {
'jsonld.UnsupportedVersion',
{code: 'invalid @version value', context: ctx});
}
if(activeCtx.processingMode && activeCtx.processingMode.indexOf('json-ld-1.1') !== 0) {
throw new JsonLdError(
'@version: ' + ctx['@version'] + ' not compatible with ' + activeCtx.processingMode,
'jsonld.ProcessingModeConflict',
{code: 'processing mode conflict', context: ctx});
}
rval.processingMode = 'json-ld-1.1';
rval['@version'] = ctx['@version'];
defined['@version'] = true;
}

// if not set explicitly, set processingMode to "json-ld-1.0"
rval.processingMode = rval.processingMode || activeCtx.processingMode || 'json-ld-1.0';

// handle @base
if('@base' in ctx) {
let base = ctx['@base'];
Expand Down Expand Up @@ -370,31 +380,42 @@ api.createTermDefinition = (activeCtx, localCtx, term, defined) => {
}

if('@container' in value) {
const container = value['@container'];
if(container !== '@list' && container !== '@set' &&
container !== '@index' && container !== '@language') {
let isValid = false;
const validContainers = ['@list', '@set', '@index', '@language'];
// JSON-LD 1.1 support
if(activeCtx['@version'] === 1.1) {
// || processingMode === 'jsonld-1.1') {
validContainers.push('@graph');
if(container === '@graph' ||
(_isArray(container) && container.length === 2 &&
container.includes('@graph') && container.includes('@set'))) {
isValid = true;
}
}
if(!isValid) {
throw new JsonLdError(
'Invalid JSON-LD syntax; @context @container value must be ' +
'one of the following: ' + validContainers.join(', '),
'jsonld.SyntaxError',
{code: 'invalid container mapping', context: localCtx});
}
// normalize container to an array form
const container = _isString(value['@container']) ? [value['@container']] : (value['@container'] || []);
const validContainers = ['@list', '@set', '@index', '@language'];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This list is just static for 1.0 and 1.1 right? May be more efficient to hoist it out to two static lists and choose which one to use based on mode at runtime. Would avoid reallocation and updating per-call.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's what I do with my implementation, but I was just trying to follow the existing pattern. I can update to make these static arrays.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking through the code, there's little uniformity for how such things are handled. Elsewhere in expand.js, there are inline arrays of keywords, and api.isKeyword is defined to use a switch statement to detect. Creating a top-level static array seems like yet a third way. Is a switch statement faster than Array.includes? Arguably, Set.has would be fast, if it worked consistently in test-karma. One could argue for a large refactor to unify the way all such lists are set and maintained.

Given that the validContainers is code I inherited, I'd rather not keep creeping this PR with things that should be done in a more general refactor.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that a larger refactor to make this stuff more consistent should be done in a separate PR.

let isValid = true;
let hasSet = container.includes('@set');

// JSON-LD 1.1 support
if(activeCtx.processingMode && activeCtx.processingMode.indexOf('json-ld-1.1') === 0) {
// TODO: @id and @type
validContainers.push('@graph');

// check container length
isValid &= container.length <= (hasSet ? 2 : 1);
} else {
// in JSON-LD 1.0, container must not be an array (it must be a string, which is one of the validContainers)
isValid &= !_isArray(value['@container']);

// check container length
isValid &= container.length <= 1;
}

// check against valid containers
isValid &= container.every(c => validContainers.includes(c));

// @set not allowed with @list
isValid &= !(hasSet && container.includes('@list'));

if(!isValid) {
throw new JsonLdError(
'Invalid JSON-LD syntax; @context @container value must be ' +
'one of the following: ' + validContainers.join(', '),
'jsonld.SyntaxError',
{code: 'invalid container mapping', context: localCtx});
}
if(mapping.reverse && container !== '@index' && container !== '@set' &&
container !== null) {

if(mapping.reverse && !container.every(c => ['@index', '@set'].includes(c))) {
throw new JsonLdError(
'Invalid JSON-LD syntax; @context @container value for a @reverse ' +
'type definition must be @index or @set.', 'jsonld.SyntaxError',
Expand Down Expand Up @@ -528,7 +549,7 @@ api.getInitialContext = (options) => {
const base = parseUrl(options.base || '');
return {
'@base': base,
'@version': 1.0,
processingMode: options.processingMode,
mappings: {},
inverse: null,
getInverse: _createInverseContext,
Expand Down
6 changes: 3 additions & 3 deletions lib/expand.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,8 @@ api.expand = ({
if(_isArray(element)) {
let rval = [];
const container = _getContextValue(
activeCtx, activeProperty, '@container');
insideList = insideList || container === '@list';
activeCtx, activeProperty, '@container') || [];
insideList = insideList || container.includes('@list');
for(let i = 0; i < element.length; ++i) {
// expand element
let e = api.expand({
Expand Down Expand Up @@ -313,7 +313,7 @@ api.expand = ({
continue;
}

const container = [].concat(_getContextValue(activeCtx, key, '@container'));
const container = _getContextValue(activeCtx, key, '@container') || [];

if(container.includes('@language') && _isObject(value)) {
// handle language map container (skip if value is not an object)
Expand Down
2 changes: 1 addition & 1 deletion tests/test-common.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ const TEST_TYPES = {
},
'jld:ExpandTest': {
skip: {
specVersion: ['json-ld-1.1']
regex: [/#t[cmn]/, /#t008[0-7]/]
},
fn: 'expand',
params: [
Expand Down