feat: Return correct status codes for invalid requests

[joachimvh]

📁 Related issues

Closes #364

✍️ Description

StaticThrowHandler is an implementation of the class discussed in the related issue. This PR resolves the cases of returning a 405 in case of unsupported HTTP methods and returning a 415 in case of unsupported PATCH formats. There are probably other situations that also should be updated now, but those can have their own issue when they pop up.

Major change since a class is renamed (would otherwise be a minor).

Extracting access modes for a PATCH request is now split over 2 classes, one which checks the method and one which checks the body. This allows us to differentiate between 405 and 415. This will probably also be useful once #1060 is added.

The 405 fallback is added both when extracting modes and when calling an operation handler. The 415 fallback is added in modes and in the patcher. In practice, only the modes fallback is relevant, but the other instances are needed in case the server is configured with a different ModesExtractor (e.g., a debug extractor that never sets any mode).

The example case of #364 of returning a 406 in case conneg is not possible is not solved by this PR since the current implementation of the ChainedConverter does not allow for this solution (since its canHandle is too permissive for this).

[RubenVerborgh]

Super; just some (hopefully) simplification suggestions.

[RubenVerborgh]

Hmm that took some parsing. Should we really do wrapping here?

I wonder if we can come to a solution with a MethodFilter or so, which basically is a generic handler that returns canHandle true only for listed methods.

I initially thought a SequenceHandler with a MethodFilter followed by SparqlUpdateModesExtractor, but then I saw that The canHandle check of this handler will always succeed (why though? we could at least check the first step?) so fair enough. But still, suggestion below on WrappedMethodModesExtractor.

[joachimvh]

From what I recall, SequenceHandler and ParallelHandler were specifically made for situations where we had a bunch of handlers that always had to be executed, with SequenceHandler being used if the order mattered (e.g., middleware first).

I do agree that this change does add a bit of verbosity to the config, but perhaps this is just a result of how it is written here? Maybe it would be clearer if the WrappedMethodModesExtractor (or its generic alternative) would be defined in a separate block? In that case the main ModesExtractor would be a WaterfallHandler with 3 simple entries.

[RubenVerborgh]

check

[RubenVerborgh]

So at the very least, I would turn this into ConditionalOperationHandler, since there seems to be no need to use the specific ModesExtractor subclass.

I wonder if we can have a pattern similar to IfNeededConverter, which can be used both in a wrapped and a chained way (note the default value for the constructor argument).

[joachimvh]

Can definitely make this generic (will also need this for the N3 Patch PR anyway). Personally I'm more of a fan of the wrapped solution instead of the fallthrough solution. I think that was also my opinion for the IfNeededConverter 😄

[RubenVerborgh]

Maybe some comments to the extent of "extract permissions based on the method" / "extract permissions based on the request body".