rdfjs / data-model-spec

RDF/JS: Data model specification – A specification of a low level interface definition representing RDF data independent of a serialized format in a JavaScript environment.
http://rdf.js.org/data-model-spec/
73 stars 12 forks source link

immutable objects #81

Open bergos opened 8 years ago

bergos commented 8 years ago

Are objects created by the DataFactory immutable? How should we write that in the spec? I think we said we don't define the properties read-only to avoid forcing the usage of Object.defineProperty in the implementations.

blake-regalia commented 8 years ago

I agree that Terms should generally be immutable, but I'm not totally convinced this is a justified limitation to impose in the specification. For example, pretend that an implementation wants to be extremely simple to use - so they define a setter method on Literal Terms to allow changing its value like so: someLiteral.value = 'new value';. Then perhaps they automatically perform SPARQL updates on behalf of the change.

timbl commented 8 years ago

I oppose being able to change the value. it should be an error or an error not caught
For example a store may have index entries which become orphaned, and future queries for the new value will fail. This applies imho to simple types literals and symbols and bNodes. Complex types like Collections and Graphs and datasets may have to be more complicated

bergos commented 8 years ago

@timbl Yes, the indexing stuff is one reason. Another reason is the behavior when a Term instance is used multiple times in one or more Quads/Datasets. Should a value change update all Quads/Datasets?

@blake-regalia Interesting use case. If we define the objects should be handled like immutable objects without throwing an error, that implementation would still pass the tests.

I think we have three options. I will add a comment for each, please use the +1 feature to vote for it.

bergos commented 8 years ago
bergos commented 8 years ago
bergos commented 8 years ago
dmitrizagidulin commented 8 years ago

I agree with the reasoning that objects should be immutable. I do think that if we add that to the spec, though, we also need to define (or at least discuss, in the specs) a way to update term values in an immutable fashion. (Meaning, the usual functional programming style of doing something like Term .fromTerm( oldTerm ) or Term .newTerm( oldTerm ), which would return a copy of the old term, with the value changed, but with everything else remaining the same (like the language and datatype of a literal), and with indexes updated accordingly).

RubenVerborgh commented 8 years ago

Immutability seems the way to go. However, I think implementations should be allowed to throw an error when the user tries to manipulate. It could be confusing if users try to do triple.subject = otherSubject but nothing happens. But it does not have to be made mandatory for me (because indeed, that would force accessors).

With regard to @dmitrizagidulin's proposal, I would propose having this in the constructor rather than as a separate method, or perhaps leaving this entirely up to the library. One can always default to .triple(other.subject, other.predicate, myObject).

bergos commented 8 years ago

We agreed in the call that Quads and Terms should be immutable but we don't define the behavior for write access. The High Level API may define more details.

dan-f commented 8 years ago

I'm not sure if this issue is only about Quads and Terms, but I'd also like to mention that I feel strongly that the Dataset, whether or not it is standardized, should be immutable.

k00ni commented 5 years ago

What is the agreement here?

@bergos wrote:

Are objects created by the DataFactory immutable?

But the prefered poll option is:

define objects immutable without throwing an error (with 4 votes currently)

And in the end

We agreed in the call that Quads and Terms should be immutable but we don't define the behavior for write access.

Please clarify: do you mean (1) that each instance of Term or one of its subclasses is immutable? Or (2) just the ones created by the DataFactory?

Point (1) implies that instances from the following are immutable too: Quad, Triple, NamedNode, Literal, BlankNode, Variable and DefaultGraph.

Point (2) implies that instances have to behave different, if created by DataFactory, dont they? Meaning, that they may throw an exception if the values gets changed anyway.

(ref #130)

bergos commented 5 years ago

Using the datafactory to create Term + subclasses and Quad is the only official way. And all of them are immutable.

Factory functions (e.g., quad()) or methods (e.g., store.createQuad()) create instances.

That's the sentence for "only way" in the spec, but I think we could improve it a little bit.

vhf commented 5 years ago

However, I think implementations should be allowed to throw an error when the user tries to manipulate. It could be confusing if users try to do triple.subject = otherSubject but nothing happens.

This feels like an important point. In my opinion using a lib implementing a spec should save oneself the tough task of learning all details of the spec.

It can have far reaching consequences, for instance using a lib to work with rdfjs datasets I can see myself doing something like:

someDataset.forEach((quad) => {
  if (quad.subject.equals(foo)) {
    quad.subject = bar
  }
})

without knowing it violates the spec because there would be no error/warning, and later on seeing something unexpectedly break making it very hard to trace the bug back to the spec.

iddan commented 5 years ago

Any term object can go through Object.freeze(), Object.seal(), and all its properties can be nonwritable.

awwright commented 5 years ago

I've got a lot of code with the assumption that nodes are immutable. This would be a good guarantee.

What if this has a negative performance impact? (IMO I don't think we should be worrying about performance impact, as this is subject to frequent change, premature optimization being the root of evil, etc, etc)