[WIP] Changes for ResourceStore interface to return Record<string, RepresentationMetadata> #1075

ixuz · 2021-11-30T09:18:26Z

Related issues

Description

Preparation for implementation of Solid Notifications according to https://solid.github.io/notifications/protocol
The proposal refers to https://www.w3.org/TR/activitystreams-core/ for the activity types.
In order to provide meaningful notifications to subscribers we have differentiated between the types of resource changes, such as CREATED, CHANGED and DELETED.
This required an introduction of a ModificationResult interface that wraps the changed ResourceIdentifiers.

This PR is backwards compatible as it only changes internal behavior.

Preparaton for implementation of Solid Notifications according to https://solid.github.io/notifications/protocol The proposal refers to https://www.w3.org/TR/activitystreams-core/ for the activity types. In order to provide meaningful notifications to subscribers we have differentiated between the types of resource changes, such as CREATED, CHANGED and DELETED. This required an introduction of a ModificationResult interface that wraps the changed ResourceIdentifiers.

RubenVerborgh · 2021-11-30T10:02:04Z

Hi @ixuz,

Thanks for contributing, really great to see the work that you're doing over there!

As proposed in #1074 (comment), let's have a call on what the overall architecture will look like.

We need to understand all the consequences; for instance:

This PR is backwards compatible as it only changes internal behavior.

It is actually a semver.major change, because ResourceStore (and in fact all other modified classes) are part of the public API. So current third-party modules using ResourceStore break after this change, necessitating a major version increment.

RubenVerborgh

The direction this solution is going, reminds me of @joachimvh's suggestion to return "a metadata object containing metadata describing which resources got changed", which is now the open issue #632.

Concretely, the current proposal is to return Promise<ModifiedResource[]>, but we probably want this to be Promise<ChangeDescription> (name to be determined) in which we detail the changes. This will provide us with a solution that provides us with future extension points.

To be discussed at our architectural meeting.

src/http/representation/ResourceIdentifier.ts

RubenVerborgh · 2021-11-30T11:30:24Z

src/storage/MonitoringStore.ts

+
+  private isInternalResource(result: ModifiedResource[]): boolean {
+    return result.filter(
+      (modified: ModifiedResource): boolean => modified.resource.path.includes('/.internal'),


That knowledge should not leak here (and the implementation is not correct; http://example.org/foo/.internalstuff is not internal).

Right 😅 Well I suppose we'll delete this isInternalResource function anyway since it's not the concern of this particular class to filter such(/.internal) things.

RubenVerborgh · 2021-11-30T11:30:42Z

src/storage/MonitoringStore.ts

-    for (const identifier of identifiers) {
-      this.emit('changed', identifier);
+  private emitChanged(modified: ModifiedResource[]): ModifiedResource[] {
+    // Don't emit 'changed' event for internal resources


We still should; it is up to handlers to see what they do with it.

Understood, we'll remove this filter/condition 👍

src/storage/ResourceStore.ts

RubenVerborgh · 2021-11-30T11:35:08Z

src/storage/MonitoringStore.ts

    const identifier = await this.source.addResource(container, representation, conditions);
-    this.emitChanged([ container, identifier ]);
+    this.emitChanged([ changedResource(container), identifier ]);


I'd suggest a different design, namely:

Emit specific created, deleted, updated events, with the identifier of the resource as a parameter

Still also emit the changed event, with the identifier as a first parameter, and the kind of change as the second

That way:

The change mechanism keeps on working as-is, and listeners can keep on being interested in any change

Listeners can choose to only follow certain events

Emit specific created, deleted, updated events, with the identifier of the resource as a parameter

We initially tried implementing the event emission in that way, but we realised that valuable information would be lost when when an operation affects multiple resources in different ways(e.g. some created and some updated). Due to this we thought wrapping all these changes into an object would be better. Therefore we implemented the ModifiedResource to wrap all performed resource changes. I now recognize based on @joachimvh review comment that this could have been better achieved by using the already existing class RepresentationMetadata.

Still also emit the changed event, with the identifier as a first parameter, and the kind of change as the second

Good point, for backwards compatability. 👍

src/storage/MonitoringStore.ts

joachimvh · 2021-11-30T11:58:14Z

src/storage/ResourceStore.ts

+export interface ModifiedResource {
+  resource: ResourceIdentifier;
+  modificationType: ModificationType;
+}


I only quickly skimmed so this is not a full review. But I noticed this.

When the ResourceStore functions where changed to return changed identifiers it was already considered to return metadata instead of arrays: #632 . This might be a good time to do this instead of inventing a completely new type just for this.

Edit: I missed that Ruben also already mentioned this above.

Thank you @joachimvh for the reivew: I'm not sure if I fully understand your comment here.

Are you suggesting that instead of adding the new ModifiedResource type, we can instead use the existing RepresentationMetadata for this purpose of conveying how resources have been affected/changed (created, modified or deleted)?

~~Or do you mean returning ModifiedResource/RepresentationMetadata was considered already in the past and ultimately decided against in favor of returning a ResourceIdentifier[]?~~

The first. ResourceIdentifier[] was a temporary solution until something more extensive was needed.

Alright then I understand, I admit I would need to look into how the RepresentationMetadata can be used for this. Let's talk about this next Thursday on the architecture meeting. Thank you.

@joachimvh I wrote up this test, to see how the information in a ModifiedResource[] could be converted into a RepresentationMetadata representation. Do I populate the RepresentationMetadata correctly? Or am I misunderstanding how the expected metadata layout should be?

See this test:

import { literal, namedNode, quad } from '@rdfjs/data-model'; import { createResourceIdentifier, ResourceIdentifier, RepresentationMetadata } from "../../../src"; describe('RepresentationMetadataExample', () => { it('can map ModifiedResource[] to RepresentationMetadata', async () => { // In this test, I'm trying to see how the current implementation in // PR(https://github.com/solid/community-server/pull/1075) can be adjusted to // convey the same information using the RepresentationMetadata data model. enum ModificationType { created, changed, deleted } interface ModifiedResource { resource: ResourceIdentifier, modificationType: ModificationType } // Here's an example set of resource modifications const exampleModifiedResources: ModifiedResource[] = [ { resource: createResourceIdentifier('http://resource.identifier1'), modificationType: ModificationType.created }, { resource: createResourceIdentifier('http://resource.identifier2'), modificationType: ModificationType.changed }, { resource: createResourceIdentifier('http://resource.identifier3'), modificationType: ModificationType.deleted }, ]; // A lookup map for convertion of ModificationType to https://www.w3.org/TR/activitystreams type const mapToActivityStreamsType = { [ModificationType.created]: 'Create', [ModificationType.changed]: 'Update', [ModificationType.deleted]: 'Delete', } // Convert and convey the same information as RepresentationMetadata const representationMetadata = new RepresentationMetadata(); exampleModifiedResources.forEach(mr => { representationMetadata.addQuads([ quad(namedNode(mr.resource.path), namedNode('@context'), literal('https://www.w3.org/ns/activitystreams')), quad(namedNode(mr.resource.path), namedNode('type'), literal(mapToActivityStreamsType[mr.modificationType])), ]); }); // Check that the expected quads exists in the result expect(representationMetadata.quads('http://resource.identifier1', '@context')[0].object.value).toBe('https://www.w3.org/ns/activitystreams'); expect(representationMetadata.quads('http://resource.identifier1', 'type')[0].object.value).toBe('Create'); expect(representationMetadata.quads('http://resource.identifier2', '@context')[0].object.value).toBe('https://www.w3.org/ns/activitystreams'); expect(representationMetadata.quads('http://resource.identifier2', 'type')[0].object.value).toBe('Update'); expect(representationMetadata.quads('http://resource.identifier3', '@context')[0].object.value).toBe('https://www.w3.org/ns/activitystreams'); expect(representationMetadata.quads('http://resource.identifier3', 'type')[0].object.value).toBe('Delete'); }) });

I've begun refactoring this PR to eliminate the ModifiedResource class and ModificationType enum.
I'll update this PR once the unit tests are satisfied. 👍

Array or Map of resource and metadata?

True, map would also be an option, with the key being the resource identifier. I don't really have a strong opinion here either way since currently you would have to iterate through all the metadata objects being returned.

A Map of representation metadata sounds quite appealing to me since as far as I know the order of entries doesn't matter, and the Map provides a nice and quick lookup.

I would use a simple object and not an actual Map though. So a Record<string, RepresentationMetadata>.

…ModificationType to be an enum, and removed unnecessary factory functions 'createdResource', 'changedResource' & 'deletedResource' from 'ResourceStore'

…LID_AS' internal vocab

…orage-modification-type

…tadata[] upon add, modify and deletion of resources (work in progress)

ixuz · 2022-05-24T16:37:37Z

Brief update: This PR is still in progress and we'll push new code to the branch soonish... 😉

ixuz · 2022-06-24T10:46:33Z

Closing, the work continues in #1350 :)

RubenVerborgh self-assigned this Nov 30, 2021

RubenVerborgh mentioned this pull request Nov 30, 2021

[WIP] Notifications #1074

Closed

10 tasks

RubenVerborgh added the semver.major Requires a major version bump label Nov 30, 2021

RubenVerborgh reviewed Nov 30, 2021

View reviewed changes

src/http/representation/ResourceIdentifier.ts Outdated Show resolved Hide resolved

RubenVerborgh reviewed Nov 30, 2021

View reviewed changes

src/storage/ResourceStore.ts Show resolved Hide resolved

RubenVerborgh reviewed Nov 30, 2021

View reviewed changes

src/storage/ResourceStore.ts Outdated Show resolved Hide resolved

RubenVerborgh reviewed Nov 30, 2021

View reviewed changes

src/storage/MonitoringStore.ts Outdated Show resolved Hide resolved

joachimvh reviewed Nov 30, 2021

View reviewed changes

fix: Renamed factory functions to follow naming 'create...', changed …

461a936

…ModificationType to be an enum, and removed unnecessary factory functions 'createdResource', 'changedResource' & 'deletedResource' from 'ResourceStore'

ixuz mentioned this pull request Nov 30, 2021

[WIP] Feat/notification gw handler and webhook #1080

Closed

ixuz added 4 commits February 10, 2022 15:23

Merge branch 'main' into feat/storage-modification-type

df0cb79

feat: New 'AS' vocabulary for Activity Streams 2.0 terms, and new 'SO…

949a9ed

…LID_AS' internal vocab

Merge branch 'main' of github.com:solid/community-server into feat/st…

92bc844

…orage-modification-type

fix: reworking the ResourceStore interface to return RepresentationMe…

25cd50a

…tadata[] upon add, modify and deletion of resources (work in progress)

ixuz marked this pull request as draft February 15, 2022 17:17

ixuz changed the title ~~feat: Add storage modification types [CREATED, CHANGED, DELETED]~~ [WIP] Changes for ResourceStore interface to return RepresentationMetadata[] upon add/modify/deletion of resources Feb 15, 2022

ixuz changed the title ~~[WIP] Changes for ResourceStore interface to return RepresentationMetadata[] upon add/modify/deletion of resources~~ [WIP] Changes for ResourceStore interface to return RepresentationMetadata[] Feb 15, 2022

test: unit tests pass, but integration tests don't (work in progress)

44f19de

ixuz changed the title ~~[WIP] Changes for ResourceStore interface to return RepresentationMetadata[]~~ [WIP] Changes for ResourceStore interface to return Record<string, RepresentationMetadata> Mar 11, 2022

BelgianNoise mentioned this pull request Jun 24, 2022

feat: rework resource store to return extra info #1350

Merged

4 tasks

ixuz closed this Jun 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Changes for ResourceStore interface to return Record<string, RepresentationMetadata> #1075

[WIP] Changes for ResourceStore interface to return Record<string, RepresentationMetadata> #1075

ixuz commented Nov 30, 2021

RubenVerborgh commented Nov 30, 2021

RubenVerborgh left a comment •

edited

Loading

RubenVerborgh Nov 30, 2021

ixuz Nov 30, 2021

RubenVerborgh Nov 30, 2021

ixuz Nov 30, 2021

RubenVerborgh Nov 30, 2021 •

edited

Loading

ixuz Nov 30, 2021

joachimvh Nov 30, 2021 •

edited

Loading

ixuz Nov 30, 2021 •

edited

Loading

joachimvh Nov 30, 2021

ixuz Nov 30, 2021

ixuz Feb 10, 2022

ixuz Feb 14, 2022

RubenVerborgh Feb 15, 2022

joachimvh Feb 16, 2022

ixuz Feb 16, 2022 •

edited

Loading

joachimvh Feb 16, 2022

ixuz commented May 24, 2022

ixuz commented Jun 24, 2022

[WIP] Changes for ResourceStore interface to return Record<string, RepresentationMetadata> #1075

[WIP] Changes for ResourceStore interface to return Record<string, RepresentationMetadata> #1075

Conversation

ixuz commented Nov 30, 2021

Related issues

Description

RubenVerborgh commented Nov 30, 2021

RubenVerborgh left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RubenVerborgh Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joachimvh Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

ixuz Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ixuz Feb 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ixuz commented May 24, 2022

ixuz commented Jun 24, 2022

RubenVerborgh left a comment •

edited

Loading

RubenVerborgh Nov 30, 2021 •

edited

Loading

joachimvh Nov 30, 2021 •

edited

Loading

ixuz Nov 30, 2021 •

edited

Loading

ixuz Feb 16, 2022 •

edited

Loading