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

Representation super-class feels a bit esoteric #19

Closed michielbdejong closed 4 years ago

michielbdejong commented 5 years ago

I can see how the use of Representation works well as an ode to REST, but I think in this diagram it doesn't necessarily add much. It's used in three ways that don't necessarily have much in common apart from the fact that they 'represent' something:

A Representation has a ResourceIdentifier, yet the methods ResourceStore#setRepresentation etc. also have their own separate resourceIdentifier parameter. How do these two relate? What happens if you set a representation.metaData.encoding or representation.metaData.contentType on a QuadRepresentation, will that be ignored? I think that's a sign that QuadRepresentation and BinaryRepresentation maybe don't have as much in common as to warrant using a super class to relate them.

What would your diagram look like if you were to:

RubenVerborgh commented 5 years ago

I can see how the use of Representation works well as an ode to REST,

Nah, I don't really do odes or serenades; I use abstractions when they are meaningful.

I think in this diagram it doesn't necessarily add much.

You're right; it doesn't add, it removes.

The Representation abstraction was introduced because I deemed it beneficial for some components to not know whether something is binary, triples, or even something else. In this case, we don't want LdpHandler to know more about the representations it is handling. Without Representation, all this knowledge would need to be present in LdpHandler. Now, LdpHandler can handle all of these things as just representations.

Compare this to the discussion we had in https://github.com/inrupt/{PRIVATE-REPO}/issues/5#issuecomment-519487952, where I explain the consequences of not doing so.

This abstraction is a crucial part of the architecture, so it's crucial that we both have the same understanding of why it was added.

It's used in three ways that don't necessarily have much in common apart from the fact that they 'represent' something

Exactly! And that's the good thing about it.

Now they do have a couple of things in common, like a media type, size, etc. Which is the level of abstraction that LdpHandler needs.

The Patch / Representation subclass/superclass relationship is not crucial. It is ontologically true (patches are representations), and it has practical uses (content-type etc.), but there's no strong loss in severing that tie.

A Representation has a ResourceIdentifier

Optionally (note the ?).

It's there for convenience; not crucial and can be removed.

yet the methods ResourceStore#setRepresentation etc. also have their own separate resourceIdentifier parameter. How do these two relate?

What happens if you set a representation.metaData.encoding or representation.metaData.contentType on a QuadRepresentation, will that be ignored?

Note that these fields are optional indeed. Setting them would not be meaningful for these cases and would be ignored. language could be though, profiles as well.

I think that's a sign that QuadRepresentation and BinaryRepresentation maybe don't have as much in common as to warrant using a super class to relate them.

Indeed, so we don't use a superclass; they only share an interface.

It's not the number of fields they share that makes them useful, but rather that there are key places where they need to be treated equivalently without the handler having to know their difference. And they do share crucial fields such as dataType and data.

  • use the 'Representation' super-class only as a parent of the BinaryRepresentation

Then Representation would no longer be meaningful; it would just be BinaryRepresentation.

Then the RepresentationConvertor pattern would break, and stores would be forced to be able to emit specific representations. Then we would need to parse and reparse again somewhere else, which is precisely what we are trying to avoid when we can.

  • use rdf.js DataSet instead of QuadRepresentation

Then it would no longer be a stream, would need to support additions and deletions, and as such likely be an in-memory or storage-backed implementation. Then representations would need to be loaded into memory, limiting performance and the size of files we can handle, whereas there are currently no limits to that.

  • use Body (basically, Readable) instead of PatchRepresentation

Not much. As described above, that relation is not crucial. We would just need to re-add fields that are already in Representation.

RubenVerborgh commented 5 years ago

Ah wait, there is a reason that Patch extends the more generic Representation. Namely, LdpHandler does not know about patches! It only knows that it asking a parser to have a look at the request body (which is a representation), not that it is a patch.

So if we want to keep knowledge about specific methods outside of LdpHandler (and we do), then we should not tell it about Patch.

RubenVerborgh commented 4 years ago

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