Project review

[joachimvh]

Here are some comments I had after going through the repository.

package.json

I don't think it's necessary to have the project version be the same as the CSS version it supports, although you can of course. But if you ever want to do a major release of this package while CSS has not changed version there is no way to do it correctly. The supported CSS version can always be deduced from the package.json dependencies.

The repository field is wrong.

Folder structure

I was confused at first that both the config and src folders have the same structure as those in CSS as I thought it implied you were replacing files/configs from CSS. Might be a bit overkil to have such deep folder nesting for 2-3 files.

Documentation

There are some minor grammar issues here and there so would suggest a re-read. Would also advise to not use "obviously" in your text to not make your readers feel stupid 😉.

ShapeValidator.ts

Is missing tsdoc for the class and parameters. The other classes could also use some more documentation.

ShaclValidator.ts

noShapePresent should be a constant outside of the class, or probably just removed since it only occurs once.

canHandle should throw an HttpError (probably NotImplementedHttpError).

let representationData; should have a type

Why does an InternalServerError imply that you are trying to create containers in a constrained container? How does the conversion process trigger this since that shouldn't create anything? Even if this happens, the check should be more specific, e.g. checking the contents of the error.

error instanceof NotImplementedHttpError -> NotImplementedHttpError.isInstance. This gets around certain components.js edge case issues.

Skipping on auxiliary files should happen in the canHandle, not after cloning, since cloning is expensive.

No need to impkement handleSafe yourself in this case as the canHandle call is negligible. The way it is currently done is also wrong, this is how it's supposed to work: https://github.com/CommunitySolidServer/CommunitySolidServer/blob/da99ff30f63b673ea272fc918c69c9ff4a5c0401/src/util/handlers/AsyncHandler.ts#L38-L41

shape-validator-component/src/storage/validators/ShaclValidator.ts

Lines 108 to 111 in 3f60a0a

	for (const targetClass of targetClasses) {
	targetClassesPresent = targetClassesPresent || dataStore.countQuads(null, null, targetClass, null) > 0;
	}
	if (!targetClassesPresent) {

Can be if (targetClasses.some(targetClass => dataStore.countQuads(null, null, targetClass, null) > 0) {.

shape-validator-component/src/storage/validators/ShaclValidator.ts

Lines 112 to 113 in 3f60a0a

	throw new BadRequestHttpError(`${'Data not accepted as no nodes in the body conform ' +
	'to any of the target classes of '}${shapeURL}`);

Not sure why there is a string in a string there.

ShapeValidationStore.ts

The fact that this class takes so many inputs makes me think that it might be doing too much. Also the fact that the setRepresentation call is 2 completely separate things (which should be separate function calls). It actually feels like the entire metadata part should not be here. We already have a place where we validate metadata that is being changed, this would be a better fit there. Perhaps CSS would need a more generic way to add classes that validate PATCH results.

Ordering of the calls should be changed so no unneccessary actions are done. No need to calculate current representation and shapes if newShapes.length > 1. Similarly, no need to do any work if newShapes.length === 0.

shape-validator-component/src/storage/ShapeValidationStore.ts

Lines 85 to 86 in 3f60a0a

	// In case the parent being http://localhost:3123/.internal/setup/ getting the representation would result into a
	// NotFoundHttpError

How do you trigger this because this feels like it should be fixed in CSS?

Only the getRepresentation call should be in the try/catch that prevents the NotFoundHttpError. Although preferably we can just prevent this situation from happening as mentioned above.

[woutslabbinck]

ShapeValidationStore

The fact that this class takes so many inputs makes me think that it might be doing too much. Also the fact that the setRepresentation call is 2 completely separate things (which should be separate function calls). It actually feels like the entire metadata part should not be here. We already have a place where we validate metadata that is being changed, this would be a better fit there. Perhaps CSS would need a more generic way to add classes that validate PATCH results.

If I understand correctly, that is something that I can only properly fix when the metadata part is updated in the CSS, right?

shape-validator-component/src/storage/ShapeValidationStore.ts

Lines 85 to 86 in 3f60a0a

	// In case the parent being http://localhost:3123/.internal/setup/ getting the representation would result into a
	// NotFoundHttpError

How do you trigger this because this feels like it should be fixed in CSS?

Move const parentRepresentation = await this.source.getRepresentation(parentIdentifier, {}); out of the try-catch and the server does not start anymore due to an NotFoundHttpError.
At that moment the parent identifier is http://localhost:3000/.internal/setup/.
The current identifier is http://localhost:3000/.internal/setup/Y3VycmVudC1iYXNlLXVybA==.

ShaclValidator

No need to impkement handleSafe yourself in this case as the canHandle call is negligible. The way it is currently done is also wrong, this is how it's supposed to work: https://github.com/CommunitySolidServer/CommunitySolidServer/blob/da99ff30f63b673ea272fc918c69c9ff4a5c0401/src/util/handlers/AsyncHandler.ts#L38-L41

Well, if I don't have my own handleSafe there, the server does not start due to:

2023-01-16T15:56:24.764Z [AppRunner] {Primary} error: Could not start the server: No ldp:constrainedBy predicate.
Could not start the server
Cause: No ldp:constrainedBy predicate.
NotImplementedHttpError: No ldp:constrainedBy predicate.
at ShaclValidator.canHandle (.../shape-validator-component/dist/storage/validators/ShaclValidator.js:27:19)
at ShaclValidator.handleSafe (.../shape-validator-component/node_modules/@solid/community-server/dist/util/handlers/AsyncHandler.js:28:20)
at ShapeValidationStore.setRepresentation (.../shape-validator-component/dist/storage/ShapeValidationStore.js:62:38)
at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
at async runWithTimeout (.../shape-validator-component/node_modules/@solid/community-server/dist/util/locking/WrappedExpiringReadWriteLocker.js:51:24)
at async GreedyReadWriteLocker.withWriteLock (.../shape-validator-component/node_modules/@solid/community-server/dist/util/locking/GreedyReadWriteLocker.js:48:20)
at async MonitoringStore.setRepresentation (.../shape-validator-component/node_modules/@solid/community-server/dist/storage/MonitoringStore.js:30:33)
at async JsonResourceStorage.set (.../shape-validator-component/node_modules/@solid/community-server/dist/storage/keyvalue/JsonResourceStorage.js:49:9)
at async EncodingPathStorage.set (.../shape-validator-component/node_modules/@solid/community-server/dist/storage/keyvalue/EncodingPathStorage.js:28:9)
at async ContainerInitializer.handle (.../shape-validator-component/node_modules/@solid/community-server/dist/init/ContainerInitializer.js:37:9)

[joachimvh]

If I understand correctly, that is something that I can only properly fix when the metadata part is updated in the CSS, right?

This would require a change in the PATCH-related classes of CSS yes. Specifically generalizing ImmutableMetadataPatcher a bit, or making it easier to inject a similar class where that one is used. You could create a class that does the necessary checks in this repository and use an override to update the contents of the urn:solid-server:default:PatchHandler_RDFStore, but it would be better suited in the CSS repo.

Well, if I don't have my own handleSafe there, the server does not start due to:

The thing is that your handleSafe is wrong though. handleSafe is supposed to be a shorthand for first calling canHandle, and if that one does not error to call handle. But if canHandle throws an error handleSafe is also supposed to throw that error, which your implementation doesn't.

The problem is that your store always calls validator.handleSafe when data is being added, but your validator only supports certain representations. E.g., no auxiliary resources, no containers without constraints, etc. But those resources still need to pass through your store to be written to the backend. One solution would be to have your validator be a WaterfallHandler with the second entry there being a validator that accepts everything. So then you say that everything that is not what you are specifically looking for is always valid.

Move const parentRepresentation = await this.source.getRepresentation(parentIdentifier, {}); out of the try-catch and the server does not start anymore due to an NotFoundHttpError.
At that moment the parent identifier is http://localhost:3000/.internal/setup/.
The current identifier is http://localhost:3000/.internal/setup/Y3VycmVudC1iYXNlLXVybA==.

Checks out, the /setup/ container will not exist at that point yet, since setRepresentation can create multiple containers at the same time. This is something that will occur every time someone tries to do a PUT to e.g. /a/b/c if /a/ and/or /a/b/ do not exist yet.

Validate no containers created

Not sure if I missed this part of the implementation or if this is new, but this seems like a convoluted way to make sure no containers are created in constrained containers. At least I think that is the idea, because I don't fully understand this comment:

shape-validator-component/src/storage/ShapeValidationStore.ts

Line 89 in 8938b2f

* Verify that no containers are created when going up in the hierarchy a container is constrained by a shape.

But if that is the idea, I would do the checks before writing the data to the backend. If the input is a container identifier, check if the parent container has constraints, and if so immediately reject the request right there, instead of first writing and then deleting again.

One issue is probably the thing mentioned above about multiple containers being created at the same time. But you could recursively

Bonus: the addResource call can potentially also create containers so you would also have to check there. But you could go up the container chain until you find one that exists.