RubenVerborgh / solid-server-architecture

Proposed architecture for a Solid server
https://rubenverborgh.github.io/solid-server-architecture/solid-architecture-v1-3-0.pdf
13 stars 2 forks source link

Consider removing BodyParser #17

Closed michielbdejong closed 4 years ago

michielbdejong commented 5 years ago

The BodyParser is used in case the body of a http request represents the operation rather than the resource. However, the knowledge of how to execute a Patch task resides in a specific type of ResourceStore, so parsing it in the first step, then passing it through the Operation and to that ResourceStore instance as a parsed object requires coordination between the first parsing step and the specific ResourceStore implementation. Wouldn't it be cleaner to keep the details of the Patch private inside the ResourceStore that knows how to execute it, and pass the Representation of the Patch (i.e. the body) as-is?

RubenVerborgh commented 5 years ago

The BodyParser

Note that there is no such thing as the BodyParser. Rather, this is a base class of which different specific implementations will exist. Such as SparqlUpdateParser, LineBasedPatchParser, etc. You can see that mechanism here: https://github.com/RubenVerborgh/solid-server-ts/blob/a93717ec02389ba10c79975fc2bbdbe8d18336f0/src/http/RequestBodyParser.ts / https://github.com/RubenVerborgh/solid-server-ts/blob/a93717ec02389ba10c79975fc2bbdbe8d18336f0/src/http/ResourceStoreRequestHandler.ts#L146

in case the body of a http request represents the operation rather than the resource.

More precisely: in cases when the body contains additional instruction about the operation to be executed.

the knowledge of how to execute a Patch task resides in a specific type of ResourceStore

Almost correct; some patchers can be reused across implementations (as mentioned in https://rubenverborgh.github.io/solid-server-architecture/solid-architecture-v1-1-0.pdf#page=5)

parsing it in the first step, then passing it through the Operation and to that ResourceStore instance as a parsed object requires coordination between the first parsing step and the specific ResourceStore implementation

The required coordination is limited to a data object. So a specific BodyParser implementation will create a specific patch, such as GraphPatternPatch or LineBasedPatch. This patch is sent down (as a generic Patch object) to the ResouceStore. At that point, the ResouceStore has to consider whether it can and how it will apply the patch; this knowledge is highly specific to the store.

The bottom line is that no syntactical or other details of the HTTP request leak down; only a parsed version of the patch.

Wouldn't it be cleaner to keep the details of the Patch private inside the ResourceStore

The details of how to execute the patch are in ResourceStore implementations, whereas the details of parsing the patch are in specific BodyParser implementations. Just like the details of the execution of an operation are in an Operation implementation, whereas the parsing happens elsewhere. This decouples parsing from execution, and HTTP/LDP from storage.

and pass the Representation of the Patch (i.e. the body) as-is?

The patch needs to be parsed to determine the required permissions. (See https://github.com/solid/solid-architecture/blob/4be883518315e7bf64dd87d01802616b1b8e9859/server/request-flow.md#step-2-parse-the-request-to-the-personal-data-store)

Also, then every store would need to know how to parse requests, whereas the same patch formats can be used with different stores.

acoburn commented 5 years ago

Just worth noting that there are various types of PATCH operations that may, by some implementations, be supported. For example, we can expect that the Solid spec will require support for SPARQL-Update, but an implementation could decide to also support other mechanisms, such as LDPatch. Having a parser layer that produces a particular object type (e.g. LineBasedPatch or LinkedDataPatch, as determined by the implementation) provides that additional flexibility.

RubenVerborgh commented 4 years ago

Resolving this for now; can be reopened if new concerns arrive.