Optional options parameter for all source, sink and store methods
Closed this issue · 10 comments
Many source, sink and store implementations may require addition options like base, HTTP headers (e.g. ETag), credentials and many more. We should add the optional Object options parameter in the spec. The base property should be also described in the spec.
The constructor can and should be used for most options, but for some properties, it doesn't make sense.
The base option could be set in the constructor, because usually creating a new instance of a parser is very lightweight. But I would expect the option in the parse (.read) method.
I implemented a store for access control, that just wraps another store. Creating the access control store isn't lightweight. I'm doing some caching and the cache is attached to the store. The user (WebID) is forwarded to the store using the options object. If only the constructor can be used to set the options I need to know the implementation and additional properties. Usually I do that in the configuration. A fork method would be an option, but also doesn't look very nice:
with options:
store.read(null, null, null, {user: req.session.user}).pipe(res)
with constructor:
var store = new conf.Store(_.defaults(conf.storeOptions), {user: req.session.user})
store.read().pipe(res)
with fork:
var s = 'fork' in store ? store.fork({user: req.session.user})
s.read().pipe(res)
It easy to add a new implementation specific parameter, but I would like to reserve the parameter to make sure it's used in the same way in all implementations. Beside the base I would handle it like the additional properties for Quads.
+0 to adding options parameter, no strong opinion.
-1, I heavily object. It's bad design if a user of store needs to pass an optional options object, needs to know whether to pass that object, and if so, what it should contain. It's a haystack being passed around (see Law of Demeter).
This is the perfect option:
var store = new conf.Store({user: req.session.user});
store.read().pipe(res);(minus the defaults; that the store should do itself) because construction can be separated from usage.
Creating the access control store isn't lightweight.
Creating objects should always be lightweight.. They should only be initialized on actual use; e.g., when a call to read is made. That does not mean the parameter should be passed to read.
The problem of whether to pass that object or not remains for the constructor. The use of conf.Store and conf.storeOptions was intended, because with your proposed solution the construction and the usage can't be separated, the configuration must be forwarded. I used a simple example, but in my actual use case it looks like this:
initPersistence()
.then(initEvents)
.then(initAccessControl)
.then(function (store) {})
Every promise creates a store or just forwards a store if events or access control is not required. That is done once on server startup. If the constructor is the only option to forward information, the complete logic must be moved from the initialization to the per request code. To make that also clear, req.session.user doesn't contain SPARQL endpoint credentials or something like that, it's the identity (WebID) of the current user accessing a web page.
The initialization is done during server startup to avoid a delayed response or even a timeout. Also if the store instance needs to be created on every request, the cache must be global. That's also not very nice.
I agree on all the design patterns you mentioned, but I implemented it that way to avoid other problems. I'm open to other solutions, but the options I want to forward are really per .read/.write call.
The problem of whether to pass that object or not remains for the constructor.
And that's exactly how it should be. Because objects can be constructed by a component that knows how to do this, but using them should not require that knowledge.
If the constructor is the only option to forward information, the complete logic must be moved from the initialization to the per request code.
Not the complete logic; it just needs to be invoked from there. Here's how you model that:
function read(s, p, o) {
var stream = new Stream();
this.getInternalStorage().then(function (storage)
storage.read(s, p, o).pipe(stream); // or however the internal storage works
});
return stream;
}
function getInternalStorage() {
return this.initPersistence()
.then(initEvents)
.then(initAccessControl);
}The initialization is done during server startup to avoid a delayed response or even a timeout.
You can force initialization. (In the example above, by calling getInternalStorage.)
Also if the store instance needs to be created on every request, the cache must be global.
The instance does not need to be created on every request.
the options I want to forward are really per
.read/.writecall.
So they are different for each call? That does not seem common usage at all. Could you clarify a use case?
So they are different for each call? That does not seem common usage at all. Could you clarify a use case?
It depends on the application, but different for 5-10 calls I would say. It's a modular LDP server with WebID and other authentication methods. The store interface is used to access a persistent triplestore, the event handling and the access control. Access control is implemented in the LDP server, not the triplestore. Page 2 of this paper shows the server stack.
@bergos can you link to code that implements this access controlled store and which requries to pass WebID of agent making request to match() ?
In the context of adding support for source-side filtering, an additional options parameter could be used in the following way:
.match(null, null, null, null, { filters: [ {term: 'object', operation: 'gt', value: '"5.6"^^xsd:decimal'} ] })
Alternatively, this could also work:
.match([ { gt: "5"^^xsd:decimal } ], 'predicate', null, 'graph')
EDIT: added alternative.