Skip to content

Feat: Edit metadata resources#1188

Merged
joachimvh merged 119 commits intoCommunitySolidServer:versions/5.0.0from
woutslabbinck:feat/metadata-editing
Aug 4, 2022
Merged

Feat: Edit metadata resources#1188
joachimvh merged 119 commits intoCommunitySolidServer:versions/5.0.0from
woutslabbinck:feat/metadata-editing

Conversation

@woutslabbinck
Copy link
Member

@woutslabbinck woutslabbinck commented Mar 2, 2022

📁 Related issues

Closes #1027

✍️ Description

This PR implements the design choices from issue #1027.

Behaviour changes of a GET request:

  • now allowed to GET a metadata resource

Behaviour changes of a POST request (PostOperationHandler):

  • not allowed to create a metadata resource (via a slug)

Behaviour changes of a PUT request (PutOperationHandler):

  • not allowed to update metadata resources
  • not allowed to update existing containers

Behaviour changes of a PATCH request (PatchOperationHandler && RepresentationPatchHandler)

  • not allowed to change pim:storage or ldp:contains triples
  • not allowed to update metadata of metadata
  • not allowed to update metadata when a corresponding resource does not exist yet

Behaviour changes of a DELETE request (DeleteOperationHandler):

  • not allowed to delete metadata resources

✅ 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.

@woutslabbinck woutslabbinck added the semver.major Requires a major version bump label Mar 2, 2022
@woutslabbinck woutslabbinck requested a review from joachimvh March 2, 2022 11:40
@woutslabbinck woutslabbinck force-pushed the feat/metadata-editing branch from d7cf765 to a8063e9 Compare March 2, 2022 13:45
Copy link
Member

@joachimvh joachimvh left a comment

Choose a reason for hiding this comment

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

Some high level changes are still going to be needed to make this a generic solution.

@woutslabbinck woutslabbinck requested a review from joachimvh March 16, 2022 10:39
@woutslabbinck woutslabbinck requested a review from joachimvh August 1, 2022 13:03
Copy link
Member

@joachimvh joachimvh left a comment

Choose a reason for hiding this comment

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

Most comments are minor refactors. Only one that might still be an issue is the container generation issue.

Did not look at the tests because at this point it's hard to know where still to look there 😅. Let me know if there are tests of specific classes you want me to look at.

@woutslabbinck woutslabbinck requested a review from joachimvh August 3, 2022 14:20
Copy link
Member

@joachimvh joachimvh left a comment

Choose a reason for hiding this comment

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

Very minor comments remaining as you can see. Can fix them myself if you prefer at this point.

* The data of this representation which conforms to the RDF/JS Dataset interface
* (https://rdf.js.org/dataset-spec/#dataset-interface).
*/
dataset: Store;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
dataset: Store;
dataset: Dataset;

How much of a hassle would it be to use this type? Can be imported from rdf-js.

If the answer is that we would need multiple casts in several places then Store can stay, but if it changes (almost) nothing it would be better to use the correct interface here.

);
const immutablePatternMap = new Map<FilterPattern, Quad[]>();
const baseSubject = namedNode(this.metadataStrategy.getSubjectIdentifier(input.identifier).path);

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

Comment on lines +75 to +83
it('does not log warnings when container resource creation fails.', async(): Promise<void> => {
store.setRepresentation.mockRejectedValueOnce(new ConflictHttpError('bad input'));
generatorData = [
{ identifier: { path: 'http://test.com/foo/' }, representation: '/container/' as any },
];
await expect(initializer.handle()).resolves.toBeUndefined();
expect(generator.generate).toHaveBeenCalledTimes(1);
expect(store.setRepresentation).toHaveBeenCalledTimes(1);
});
Copy link
Member

Choose a reason for hiding this comment

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

This test still succeeds even though the specific code for this was removed 😄. This test should have checked if the logger emitted a warning, but since that piece of code is gone again it can just be removed.

Copy link
Member

@joachimvh joachimvh left a comment

Choose a reason for hiding this comment

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

🥳

@joachimvh joachimvh merged commit ca62511 into CommunitySolidServer:versions/5.0.0 Aug 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver.major Requires a major version bump

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants