Skip to content

Commit

Permalink
Fix metadata of initial linked source not being accumulated
Browse files Browse the repository at this point in the history
This caused problems related to dataset-level cardinalities
that were found in the initial source being overridden without
proper accumulation with exact cardinalities from later sources

Closes #1156
Closes #1180

May be related to comunica/comunica-feature-link-traversal#102
  • Loading branch information
rubensworks committed Apr 6, 2023
1 parent 8f93714 commit 8595851
Show file tree
Hide file tree
Showing 10 changed files with 83 additions and 39 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,25 @@ export class ActorRdfMetadataAccumulateCardinality extends ActorRdfMetadataAccum
const cardinality: QueryResultCardinality = { ...action.accumulatedMetadata.cardinality };

if (cardinality.dataset) {
if (action.appendingMetadata.cardinality.dataset &&
cardinality.dataset !== action.appendingMetadata.cardinality.dataset) {
// If the accumulated cardinality is dataset-wide,
// and the appending cardinality refers to another dataset,
// remove the dataset scopes.
delete cardinality.dataset;
if (action.appendingMetadata.cardinality.dataset) {
// If the accumulated cardinality is dataset-wide
if (cardinality.dataset !== action.appendingMetadata.cardinality.dataset &&
action.appendingMetadata.subsetOf === cardinality.dataset) {
// If the appending cardinality refers to the subset of a dataset,
// use the cardinality of the subset.
return { metadata: { cardinality: action.appendingMetadata.cardinality }};
}
if (cardinality.dataset !== action.appendingMetadata.cardinality.dataset) {
// If the appending cardinality refers to another dataset,
// remove the dataset scopes.
delete cardinality.dataset;
} else {
// If the appending cardinality is for the same dataset,
// keep the accumulated cardinality unchanged.
return { metadata: { cardinality }};
}
} else {
// If the accumulated cardinality is dataset-wide,
// and the appending cardinality refers to a dataset subset,
// If the appending cardinality refers to a dataset subset,
// keep the accumulated cardinality unchanged.
return { metadata: { cardinality }};
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,15 @@ describe('ActorRdfMetadataAccumulateCardinality', () => {
})).toEqual({ metadata: { cardinality: { type: 'estimate', value: 200, dataset: 'abc' }}});
});

it('should handle appending with the same dataset-wide cardinality', async() => {
expect(await actor.run({
context,
mode: 'append',
accumulatedMetadata: <any> { cardinality: { type: 'estimate', value: 200, dataset: 'abc' }},
appendingMetadata: <any> { cardinality: { type: 'estimate', value: 3, dataset: 'abc' }},
})).toEqual({ metadata: { cardinality: { type: 'estimate', value: 200, dataset: 'abc' }}});
});

it('should handle appending with different dataset-wide cardinalities', async() => {
expect(await actor.run({
context,
Expand All @@ -98,6 +107,15 @@ describe('ActorRdfMetadataAccumulateCardinality', () => {
appendingMetadata: <any> { cardinality: { type: 'estimate', value: 3, dataset: 'def' }},
})).toEqual({ metadata: { cardinality: { type: 'estimate', value: 203 }}});
});

it('should handle appending with different dataset-wide cardinalities that are subsets', async() => {
expect(await actor.run({
context,
mode: 'append',
accumulatedMetadata: <any> { cardinality: { type: 'estimate', value: 200, dataset: 'abc' }},
appendingMetadata: <any> { cardinality: { type: 'estimate', value: 3, dataset: 'def' }, subsetOf: 'abc' },
})).toEqual({ metadata: { cardinality: { type: 'estimate', value: 3, dataset: 'def' }}});
});
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export class ActorRdfResolveHypermediaQpf extends ActorRdfResolveHypermedia
}

public async testMetadata(action: IActionRdfResolveHypermedia): Promise<IActorRdfResolveHypermediaTest> {
const { searchForm } = this.createSource(action.metadata, action.context);
const { searchForm } = this.createSource(action.url, action.metadata, action.context);
if (action.handledDatasets && action.handledDatasets[searchForm.dataset]) {
throw new Error(`Actor ${this.name} can only be applied for the first page of a QPF dataset.`);
}
Expand All @@ -41,11 +41,16 @@ export class ActorRdfResolveHypermediaQpf extends ActorRdfResolveHypermedia
*/
public async run(action: IActionRdfResolveHypermedia): Promise<IActorRdfResolveHypermediaOutput> {
this.logInfo(action.context, `Identified as qpf source: ${action.url}`);
const source = this.createSource(action.metadata, action.context, action.quads);
const source = this.createSource(action.url, action.metadata, action.context, action.quads);
return { source, dataset: source.searchForm.dataset };
}

protected createSource(metadata: Record<string, any>, context: IActionContext, quads?: RDF.Stream): RdfSourceQpf {
protected createSource(
url: string,
metadata: Record<string, any>,
context: IActionContext,
quads?: RDF.Stream,
): RdfSourceQpf {
return new RdfSourceQpf(
this.mediatorMetadata,
this.mediatorMetadataExtract,
Expand All @@ -54,6 +59,7 @@ export class ActorRdfResolveHypermediaQpf extends ActorRdfResolveHypermedia
this.predicateUri,
this.objectUri,
this.graphUri,
url,
metadata,
context,
quads,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ export class RdfSourceQpf implements IQuadSource {
private readonly predicateUri: string;
private readonly objectUri: string;
private readonly graphUri?: string;
private readonly url: string;
private readonly defaultGraph?: RDF.NamedNode;
private readonly context: IActionContext;
private readonly cachedQuads: Record<string, AsyncIterator<RDF.Quad>>;
Expand All @@ -36,6 +37,7 @@ export class RdfSourceQpf implements IQuadSource {
mediatorMetadataExtract: MediatorRdfMetadataExtract,
mediatorDereferenceRdf: MediatorDereferenceRdf,
subjectUri: string, predicateUri: string, objectUri: string, graphUri: string | undefined,
url: string,
metadata: Record<string, any>, context: IActionContext, initialQuads?: RDF.Stream) {
this.mediatorMetadata = mediatorMetadata;
this.mediatorMetadataExtract = mediatorMetadataExtract;
Expand All @@ -44,6 +46,7 @@ export class RdfSourceQpf implements IQuadSource {
this.predicateUri = predicateUri;
this.objectUri = objectUri;
this.graphUri = graphUri;
this.url = url;
this.context = context;
this.cachedQuads = {};
const searchForm = this.getSearchForm(metadata);
Expand Down Expand Up @@ -154,7 +157,7 @@ export class RdfSourceQpf implements IQuadSource {
requestTime: dereferenceRdfOutput.requestTime,
})
.then(({ metadata }) => quads
.setProperty('metadata', { ...metadata, canContainUndefs: false }));
.setProperty('metadata', { ...metadata, canContainUndefs: false, subsetOf: this.url }));

// The server is free to send any data in its response (such as metadata),
// including quads that do not match the given matter.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ describe('ActorRdfResolveHypermediaQpf', () => {
it('should create an RdfSourceQpf', () => {
const context = {};
const quads = empty();
const source = actor.createSource(metadata, context, quads);
const source = actor.createSource('url', metadata, context, quads);
expect(source).toBeInstanceOf(RdfSourceQpf);
expect(source.mediatorMetadata).toBe(mediatorMetadata);
expect(source.mediatorMetadataExtract).toBe(mediatorMetadataExtract);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ describe('RdfSourceQpf', () => {
'p',
'o',
'g',
'url',
metadata,
new ActionContext(),
streamifyArray([
Expand Down Expand Up @@ -104,6 +105,7 @@ describe('RdfSourceQpf', () => {
'p',
's',
'g',
'url',
metadata,
new ActionContext(),
undefined,
Expand All @@ -121,6 +123,7 @@ describe('RdfSourceQpf', () => {
'p',
's',
'g',
'url',
metadata,
new ActionContext(),
streamifyArray([
Expand Down Expand Up @@ -270,7 +273,7 @@ describe('RdfSourceQpf', () => {
const output = source.match(DF.namedNode('s1'), v, DF.namedNode('o1'), v);
const metadataPromise = new Promise(resolve => output.getProperty('metadata', resolve));
await arrayifyStream(output);
expect(await metadataPromise).toEqual({ next: 'NEXT', canContainUndefs: false });
expect(await metadataPromise).toEqual({ next: 'NEXT', canContainUndefs: false, subsetOf: 'url' });
});

// The following test is not applicable anymore.
Expand Down Expand Up @@ -417,6 +420,7 @@ describe('RdfSourceQpf with a custom default graph', () => {
'p',
'o',
'g',
'url',
metadata,
new ActionContext(),
streamifyArray([
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -216,19 +216,17 @@ export abstract class LinkedRdfSourcesAsyncRdfIterator extends BufferedIterator<
// Listen for the metadata of the source
// The metadata property is guaranteed to be set
iterator.getProperty('metadata', (metadata: MetadataQuads) => {
metadata = { ...startSource.metadata, ...metadata };

// Accumulate the metadata object
this.accumulatedMetadata = this.accumulatedMetadata
.then(previousMetadata => (async() => {
if (!previousMetadata) {
return metadata;
previousMetadata = startSource.metadata;
}
return this.accumulateMetadata(previousMetadata, metadata);
})()
.then(accumulatedMetadata => {
// Also merge fields that were not explicitly accumulated
const returnMetadata = { ...metadata, ...accumulatedMetadata };
const returnMetadata = { ...startSource.metadata, ...metadata, ...accumulatedMetadata };

// Create new metadata state
returnMetadata.state = new MetadataValidationState();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,12 @@ describe('ActorRdfResolveQuadPatternHypermedia', () => {
it('should return a quad stream and metadata, with metadata resolving first', async() => {
const { data } = await actor.run({ context, pattern });
expect(await new Promise(resolve => data.getProperty('metadata', resolve)))
.toEqual({ state: expect.any(MetadataValidationState), firstMeta: true, a: 1 });
.toEqual({
state: expect.any(MetadataValidationState),
firstMeta: true,
a: 1,
cardinality: { type: 'estimate', value: Number.POSITIVE_INFINITY },
});
expect(await arrayifyStream(data)).toEqualRdfQuadArray([
quad('s1', 'p1', 'o1'),
quad('s2', 'p2', 'o2'),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ class Dummy extends LinkedRdfSourcesAsyncRdfIterator {
source: <any> {
match() {
const it = new ArrayIterator<RDF.Quad>([], { autoStart: false });
it.setProperty('metadata', { subseq: true });
it.setProperty('metadata', { subseq: true, next: undefined });
if (this.createdSubIterator) {
this.createdSubIterator.emit('data', it);
}
Expand All @@ -65,7 +65,7 @@ class Dummy extends LinkedRdfSourcesAsyncRdfIterator {
source: <any> {
match: () => {
const it = new ArrayIterator<RDF.Quad>([ ...this.data[requestedPage] ], { autoStart: false });
it.setProperty('metadata', { subseq: true });
it.setProperty('metadata', { subseq: true, next: `P${requestedPage + 1}` });
if (this.createdSubIterator) {
this.createdSubIterator.emit('data', it);
}
Expand All @@ -79,7 +79,7 @@ class Dummy extends LinkedRdfSourcesAsyncRdfIterator {
accumulatedMetadata: MetadataQuads,
appendingMetadata: MetadataQuads,
): Promise<MetadataQuads> {
return { ...accumulatedMetadata, next: undefined, ...appendingMetadata };
return { ...accumulatedMetadata, ...appendingMetadata };
}
}

Expand Down Expand Up @@ -123,7 +123,7 @@ class DummyMetaOverride extends Dummy {
source: <any> {
match() {
const it = new ArrayIterator<RDF.Quad>([], { autoStart: false });
it.setProperty('metadata', { subseq: true });
it.setProperty('metadata', { subseq: true, next: undefined });
return it;
},
},
Expand Down Expand Up @@ -334,7 +334,7 @@ describe('LinkedRdfSourcesAsyncRdfIterator', () => {
state: expect.any(MetadataValidationState),
firstPageToken: true,
next: undefined,
requestedPage: 1,
requestedPage: 0,
subseq: true,
});
});
Expand Down Expand Up @@ -395,7 +395,7 @@ describe('LinkedRdfSourcesAsyncRdfIterator', () => {
state: expect.any(MetadataValidationState),
firstPageToken: true,
next: undefined,
requestedPage: 1,
requestedPage: 0,
subseq: true,
});
});
Expand Down Expand Up @@ -472,16 +472,11 @@ describe('LinkedRdfSourcesAsyncRdfIterator', () => {
await new Promise(resolve => it.on('end', resolve));

expect(result).toEqual(quads.flat());
expect((<any> it).startIteratorsForNextUrls).toHaveBeenCalledTimes(9);
expect((<any> it).startIteratorsForNextUrls).toHaveBeenNthCalledWith(1, { first: true }, true);
expect((<any> it).startIteratorsForNextUrls).toHaveBeenNthCalledWith(2, { first: true }, true);
expect((<any> it).startIteratorsForNextUrls).toHaveBeenNthCalledWith(3, { first: true }, false);
expect((<any> it).startIteratorsForNextUrls).toHaveBeenNthCalledWith(4, { first: true }, false);
expect((<any> it).startIteratorsForNextUrls).toHaveBeenNthCalledWith(5, { P1: true }, true);
expect((<any> it).startIteratorsForNextUrls).toHaveBeenNthCalledWith(6, { first: true }, false);
expect((<any> it).startIteratorsForNextUrls).toHaveBeenNthCalledWith(7, { P2: true }, true);
expect((<any> it).startIteratorsForNextUrls).toHaveBeenNthCalledWith(8, { first: true }, false);
expect((<any> it).startIteratorsForNextUrls).toHaveBeenNthCalledWith(9, { P3: true }, true);
expect((<any> it).startIteratorsForNextUrls).toHaveBeenCalledWith({ first: true }, false);
expect((<any> it).startIteratorsForNextUrls).toHaveBeenCalledWith({ first: true }, true);
expect((<any> it).startIteratorsForNextUrls).toHaveBeenCalledWith({ P1: true }, true);
expect((<any> it).startIteratorsForNextUrls).toHaveBeenCalledWith({ P2: true }, true);
expect((<any> it).startIteratorsForNextUrls).toHaveBeenCalledWith({ P3: true }, true);
});

it('handles multiple pages when the first source is pre-loaded', async() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,12 @@ describe('MediatedQuadSource', () => {
quad('s3', 'p3', 'o3'),
quad('s4', 'p4', 'o4'),
]);
expect(await metaPromise).toEqual({ state: expect.any(MetadataValidationState), firstMeta: true, a: 1 });
expect(await metaPromise).toEqual({
state: expect.any(MetadataValidationState),
firstMeta: true,
a: 1,
cardinality: { type: 'estimate', value: Number.POSITIVE_INFINITY },
});
});

it('should set the first source after the first match call', async() => {
Expand Down Expand Up @@ -120,7 +125,7 @@ describe('MediatedQuadSource', () => {
link: { url: 'firstUrl' },
handledDatasets: {},
metadata: { state: new MetadataValidationState(),
cardinality: { type: 'exact', value: 0 },
cardinality: { type: 'exact', value: 1 },
canContainUndefs: false,
a: 2 },
source: {
Expand All @@ -129,7 +134,7 @@ describe('MediatedQuadSource', () => {
quad('s1x', 'p1', 'o1'),
quad('s2x', 'p2', 'o2'),
], { autoStart: false });
it.setProperty('metadata', { firstMeta: true });
it.setProperty('metadata', { firstMeta: true, cardinality: { type: 'exact', value: 1 }});
return it;
},
},
Expand All @@ -145,7 +150,7 @@ describe('MediatedQuadSource', () => {
expect(await metaPromise).toEqual({
state: expect.any(MetadataValidationState),
canContainUndefs: false,
cardinality: { type: 'exact', value: 0 },
cardinality: { type: 'exact', value: 1 },
firstMeta: true,
a: 2,
});
Expand Down

0 comments on commit 8595851

Please sign in to comment.