feat: rework resource store to return extra info #1350

BelgianNoise · 2022-06-24T10:39:55Z

📁 Related issues

This PR is mostly based on work of @ixuz from #1075 .
Seemed like a good idea to branch of the current code as the linked PR is still based on v2.0.0 .

✍️ Description

(Copy from other PR)

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 PR is backwards compatible as it only changes internal behavior.

✅ PR check list

Before this pull request can be merged, a core maintainer will check whether

this PR is labeled with the correct semver label
- semver.patch: Backwards compatible bug fixes.
- semver.minor: Backwards compatible feature additions.
- semver.major: Breaking changes. This includes changing interfaces or configuration behaviour.
the correct branch is targeted. Patch updates can target main, other changes should target the latest versions/* branch.
the RELEASE_NOTES.md document in case of relevant feature or config changes.
any relevant documentation was updated to reflect the changes in this PR.

BelgianNoise · 2022-06-24T12:00:43Z

Original comment: #1075 (comment) .

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

@RubenVerborgh Im not sure what you mean exactly.

If, for example, there are multiple identifiers that were updated: Do we want to emit an updated event for every identifier that was changed (indicated by SOLID_AS.Activity AS.Update) or only one updated event containing an object of type Record<string, RepresentationMetadata>.

joachimvh · 2022-06-27T09:09:23Z

If, for example, there are multiple identifiers that were updated: Do we want to emit an updated event for every identifier that was changed (indicated by SOLID_AS.Activity AS.Update) or only one updated event containing an object of type Record<string, RepresentationMetadata>.

The first. But the comment of @ixuz below there seems to indicate that there were problems with that idea so I don't think it's relevant anymore?

ixuz · 2022-06-27T10:48:25Z

If, for example, there are multiple identifiers that were updated: Do we want to emit an updated event for every identifier that was changed (indicated by SOLID_AS.Activity AS.Update) or only one updated event containing an object of type Record<string, RepresentationMetadata>.

The first. But the comment of @ixuz below there seems to indicate that there were problems with that idea so I don't think it's relevant anymore?

Looking back at this and my previous comment, since then I've changed my mind and think the first option is reasonable.

I recognize that some contextual information about the overall change may be lost(as each identifier is emitted individually), but on the other hand, we have not made use of that [extra contextual] information anyways in our other PR that leverages the feature of this PR. We can even see here that we're effectively breaking up the event into individual identifiers before passing to the next handler 😅

Let's go with the first option. 👍

Co-authored-by: Anton Wiklund <ixuz07@gmail.com>

ixuz

Great! Looks good to me! 👍

ixuz · 2022-07-01T12:39:10Z

Now I think this PR is ready for merge, please have another look @joachimvh
Thanks 🙂

joachimvh

No major changes requested, just some minor issues here and there. I did not read the comments/reviews above so if anything there was relevant to one of my comments do let me know.

One important note regarding the output type of the ResourceStore: in #1363 I introduce a Map that can safely use ResourceIdentifiers as keys to make sure the same objects get reused and to be future-proof if we ever extend what such an object contains. This should also be used as the output type of the ResourceStore here instead of the Record<string, RepresentationMetadata> currently used. So the interface would be a Map<ResourceIdentifier, RepresentationMetadata>. That PR is not merged yet though so you currently can't do that so keep using the records until then. Depending on which PR gets merged first it can either be added here or I'll just to a small PR myself afterwards to do the change as the changes for this should be quite minimal

RELEASE_NOTES.md

documentation/resource-store.md

src/storage/ResourceStore.ts

src/storage/DataAccessorBasedStore.ts

src/http/ldp/PostOperationHandler.ts

src/http/representation/ResourceIdentifier.ts

src/storage/MonitoringStore.ts

src/util/Vocabularies.ts

test/unit/http/ldp/PostOperationHandler.test.ts

…hub.com:CommunitySolidServer/CommunitySolidServer into feat/rework-resource-store-to-return-extra-info

joachimvh · 2022-07-05T10:12:48Z

src/storage/MonitoringStore.ts

-      this.emit('changed', identifier);
+  private emitChanged(changes: Record<string, RepresentationMetadata>): Record<string, RepresentationMetadata> {
+    for (const [ key, value ] of Object.entries(changes)) {
+      const activity = value.get(SOLID_AS.terms.Activity)?.value;


Just realized this is going to error if there are multiple values for Activity. It can also potentially send invalid values in case the source store set a wrong value there. There should probably be checks for these cases to make sure this store only emits correct values.

You might have missed this comment since I made it after the review. But thinking on this again we should take into account other people making faulty stores so this can be ignored.

…suggestion

BelgianNoise · 2022-07-05T12:18:56Z

@joachimvh The CI seems bugged for me. It doesn't run and/or doesn't report. Could you take a look at that?

joachimvh · 2022-07-05T12:41:01Z

@joachimvh The CI seems bugged for me. It doesn't run and/or doesn't report. Could you take a look at that?

It's because there is a merge conflict. The structure of the documentation folder changed recently so this will have to be modified in this PR.

…-resource-store-to-return-extra-info

joachimvh

Just some minor refactor comments but then this should be ready to merge

documentation/markdown/resource-store.md

src/storage/ResourceStore.ts

joachimvh · 2022-07-06T09:57:01Z

src/storage/MonitoringStore.ts

-      this.emit('changed', identifier);
+  private emitChanged(changes: Record<string, RepresentationMetadata>): Record<string, RepresentationMetadata> {
+    for (const [ key, value ] of Object.entries(changes)) {
+      const activity = value.get(SOLID_AS.terms.Activity)?.value;


You might have missed this comment since I made it after the review. But thinking on this again we should take into account other people making faulty stores so this can be ignored.

…rn-extra-info

joachimvh

Thanks. I assume the plan is to now look at the notifications spec?

joachimvh · 2022-07-06T12:39:15Z

@BelgianNoise apparently there is still a conflict, can you look into what it is?

Edit: you can ignore this, I was looking at the wrong setting.

ixuz · 2022-07-06T13:02:25Z

Thanks. I assume the plan is to now look at the notifications spec?

Indeed that's the next step in our plan 👍

BelgianNoise added 16 commits June 24, 2022 12:25

feat: change return types in ResourceStore.ts

da08aff

feat: change return types in BaseResourceStore.ts

8a763bd

feat: change return types in LockingResourceStore.ts

4284a94

feat: change return types in RoutingResourceStore.ts

217793e

feat: change return types in MonitoringStore.ts

bd40939

feat: change return types in PassthroughStore.ts

909aae9

feat: change return types in ReadOnlyStore.ts

9ab01fb

feat: change return types in PatchHandler.ts

aabc653

feat: change return types in PatchingStore.ts

2e646e0

feat: change return types in RepresentationPatchHandler.ts

1e4ed37

feat: create createResourceIdentifier() function for convenience

7d49951

feat: adapt PostOperationHandler.ts to new typing

39187c8

feat: change return types in RepresentationConvertingStore.ts

bbe96fe

feat: adapt DataAccessorBasedStore.ts implementation to new typings

24cfb86

feat: adapt UnsecureWebSocketsProtocol.ts to new typing

659fc8b

chore: add temporary comments

8b4b626

BelgianNoise assigned ixuz and BelgianNoise Jun 24, 2022

BelgianNoise added the semver.minor Requires a minor version bump label Jun 24, 2022

ixuz mentioned this pull request Jun 24, 2022

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

Closed

BelgianNoise added 3 commits June 24, 2022 13:17

fix: return correct Location header on POST request with slug

ce811a0

fix: npm run lint command needs more packages

ed9952b

fix: linting errors

6ec74c2

BelgianNoise added 3 commits June 24, 2022 14:05

chore: revert ed9952b

ec737d4

test: adapt PostOperationHandler tests

5fc9f5f

test: adapt UnsecureWebSocketsProtocol tests

c0cb42e

test: adapt DataAccessorBasedStore tests

15b4d57

chore: Update documentation/resource-store.md

83347d2

Co-authored-by: Anton Wiklund <ixuz07@gmail.com>

BelgianNoise requested a review from ixuz July 1, 2022 09:02

ixuz approved these changes Jul 1, 2022

View reviewed changes

joachimvh requested changes Jul 4, 2022

View reviewed changes

BelgianNoise added 5 commits July 5, 2022 08:57

chore: very small changes

4fa0930

chore: simplify metadata creation

8f6ff56

fix: DataAccessorBasedStore improvement and bugfix

c785a17

chore: improve resource-store.md

1e9fb18

Merge branch 'feat/rework-resource-store-to-return-extra-info' of git…

fef57fa

…hub.com:CommunitySolidServer/CommunitySolidServer into feat/rework-resource-store-to-return-extra-info

joachimvh reviewed Jul 5, 2022

View reviewed changes

BelgianNoise added 2 commits July 5, 2022 13:05

chore: adapt MonitoringStore event names, update docs and apply code …

f41aa11

…suggestion

chore: use ResourceStoreResponse type

cc425e2

BelgianNoise requested a review from joachimvh July 5, 2022 12:18

Merge remote-tracking branch 'origin/versions/5.0.0' into feat/rework…

962853b

…-resource-store-to-return-extra-info

joachimvh requested changes Jul 6, 2022

View reviewed changes

BelgianNoise and others added 4 commits July 6, 2022 12:34

fix: typo

7e2f375

chore: rename ResourceStoreResponse type to ChangeMap

c26bba9

chore: adapt .gitignore to name change

bca1fcd

Merge branch 'versions/5.0.0' into feat/rework-resource-store-to-retu…

4195492

…rn-extra-info

BelgianNoise requested a review from joachimvh July 6, 2022 11:32

joachimvh approved these changes Jul 6, 2022

View reviewed changes

joachimvh merged commit e0954cf into versions/5.0.0 Jul 6, 2022

joachimvh deleted the feat/rework-resource-store-to-return-extra-info branch July 6, 2022 12:40

joachimvh mentioned this pull request Jul 11, 2022

Use IdentifierMap for ResourceStore responses #1383

Merged

BelgianNoise mentioned this pull request Aug 22, 2022

feat: notification webhooks #1388

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: rework resource store to return extra info #1350

feat: rework resource store to return extra info #1350

BelgianNoise commented Jun 24, 2022 •

edited

Loading

BelgianNoise commented Jun 24, 2022 •

edited

Loading

joachimvh commented Jun 27, 2022

ixuz commented Jun 27, 2022

ixuz left a comment

ixuz commented Jul 1, 2022 •

edited

Loading

joachimvh left a comment •

edited

Loading

joachimvh Jul 5, 2022

joachimvh Jul 6, 2022

BelgianNoise commented Jul 5, 2022

joachimvh commented Jul 5, 2022

joachimvh left a comment

joachimvh Jul 6, 2022

joachimvh left a comment

joachimvh commented Jul 6, 2022 •

edited

Loading

ixuz commented Jul 6, 2022

feat: rework resource store to return extra info #1350

feat: rework resource store to return extra info #1350

Conversation

BelgianNoise commented Jun 24, 2022 • edited Loading

📁 Related issues

✍️ Description

✅ PR check list

BelgianNoise commented Jun 24, 2022 • edited Loading

joachimvh commented Jun 27, 2022

ixuz commented Jun 27, 2022

ixuz left a comment

Choose a reason for hiding this comment

ixuz commented Jul 1, 2022 • edited Loading

joachimvh left a comment • edited Loading

Choose a reason for hiding this comment

joachimvh Jul 5, 2022

Choose a reason for hiding this comment

joachimvh Jul 6, 2022

Choose a reason for hiding this comment

BelgianNoise commented Jul 5, 2022

joachimvh commented Jul 5, 2022

joachimvh left a comment

Choose a reason for hiding this comment

joachimvh Jul 6, 2022

Choose a reason for hiding this comment

joachimvh left a comment

Choose a reason for hiding this comment

joachimvh commented Jul 6, 2022 • edited Loading

ixuz commented Jul 6, 2022

BelgianNoise commented Jun 24, 2022 •

edited

Loading

BelgianNoise commented Jun 24, 2022 •

edited

Loading

ixuz commented Jul 1, 2022 •

edited

Loading

joachimvh left a comment •

edited

Loading

joachimvh commented Jul 6, 2022 •

edited

Loading