Skip to content
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

Feat: Edit metadata resources #1188

Conversation

woutslabbinck
Copy link
Contributor

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

config/util/auxiliary/strategies/meta.json Outdated Show resolved Hide resolved
src/http/ldp/DeleteOperationHandler.ts Outdated Show resolved Hide resolved
src/http/ldp/PatchOperationHandler.ts Outdated Show resolved Hide resolved
src/http/ldp/PatchOperationHandler.ts Outdated Show resolved Hide resolved
src/http/ldp/PatchOperationHandler.ts Outdated Show resolved Hide resolved
src/storage/patch/RepresentationPatchHandler.ts Outdated Show resolved Hide resolved
src/storage/patch/RepresentationPatchHandler.ts Outdated Show resolved Hide resolved
src/storage/patch/RepresentationPatchHandler.ts Outdated Show resolved Hide resolved
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.

config/storage/backend/regex.json Outdated Show resolved Hide resolved
config/storage/backend/regex.json Outdated Show resolved Hide resolved
config/storage/middleware/stores/patching.json Outdated Show resolved Hide resolved
config/storage/middleware/stores/patching.json Outdated Show resolved Hide resolved
documentation/metadata-editing.md Outdated Show resolved Hide resolved
src/storage/patch/RdfPatcher.ts Outdated Show resolved Hide resolved
src/storage/patch/RdfPatcher.ts Show resolved Hide resolved
src/storage/patch/RepresentationPatchHandler.ts Outdated Show resolved Hide resolved
src/storage/patch/RepresentationPatchHandler.ts Outdated Show resolved Hide resolved
templates/config/defaults.json Outdated Show resolved Hide resolved
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.

src/http/output/metadata/AuxiliaryLinkMetadataWriter.ts Outdated Show resolved Hide resolved
src/http/output/metadata/AuxiliaryLinkMetadataWriter.ts Outdated Show resolved Hide resolved
/**
* 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.

src/pods/generate/TemplatedResourcesGenerator.ts Outdated Show resolved Hide resolved
src/pods/generate/TemplatedResourcesGenerator.ts Outdated Show resolved Hide resolved
);
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