-
Notifications
You must be signed in to change notification settings - Fork 124
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
[WIP] Changes for ResourceStore interface to return Record<string, RepresentationMetadata> #1075
[WIP] Changes for ResourceStore interface to return Record<string, RepresentationMetadata> #1075
Conversation
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.
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:
It is actually a semver.major change, because |
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.
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/storage/MonitoringStore.ts
Outdated
|
||
private isInternalResource(result: ModifiedResource[]): boolean { | ||
return result.filter( | ||
(modified: ModifiedResource): boolean => modified.resource.path.includes('/.internal'), |
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 knowledge should not leak here (and the implementation is not correct; http://example.org/foo/.internalstuff
is not internal).
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.
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.
src/storage/MonitoringStore.ts
Outdated
for (const identifier of identifiers) { | ||
this.emit('changed', identifier); | ||
private emitChanged(modified: ModifiedResource[]): ModifiedResource[] { | ||
// Don't emit 'changed' event for internal resources |
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.
We still should; it is up to handlers to see what they do with it.
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.
Understood, we'll remove this filter/condition 👍
src/storage/MonitoringStore.ts
Outdated
const identifier = await this.source.addResource(container, representation, conditions); | ||
this.emitChanged([ container, identifier ]); | ||
this.emitChanged([ changedResource(container), identifier ]); |
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'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
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.
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. 👍
export interface ModifiedResource { | ||
resource: ResourceIdentifier; | ||
modificationType: ModificationType; | ||
} |
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 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.
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.
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[]
?
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.
The first. ResourceIdentifier[]
was a temporary solution until something more extensive was needed.
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.
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.
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.
@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');
})
});
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've begun refactoring this PR to eliminate the ModifiedResource
class and ModificationType
enum.
I'll update this PR once the unit tests are satisfied. 👍
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.
Array or Map of resource and metadata?
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.
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.
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.
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.
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 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)
Brief update: This PR is still in progress and we'll push new code to the branch soonish... 😉 |
Closing, the work continues in #1350 :) |
Related issues
#1074
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.