data-model-spec icon indicating copy to clipboard operation
data-model-spec copied to clipboard

immutable objects

Open bergos opened this issue 7 years ago • 15 comments

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.

bergos avatar Aug 12 '16 13:08 bergos

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.

blake-regalia avatar Aug 12 '16 18:08 blake-regalia

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

timbl avatar Aug 15 '16 07:08 timbl

@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 avatar Aug 15 '16 11:08 bergos

  • don't define objects immutable

bergos avatar Aug 15 '16 11:08 bergos

  • define objects immutable and calling the setter method should throw an error

bergos avatar Aug 15 '16 11:08 bergos

  • define objects immutable without throwing an error

bergos avatar Aug 15 '16 11:08 bergos

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).

dmitrizagidulin avatar Aug 15 '16 15:08 dmitrizagidulin

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).

RubenVerborgh avatar Aug 15 '16 16:08 RubenVerborgh

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.

bergos avatar Oct 13 '16 17:10 bergos

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.

dan-f avatar Oct 27 '16 17:10 dan-f

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)

k00ni avatar Nov 24 '18 15:11 k00ni

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.

bergos avatar Nov 27 '18 22:11 bergos

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.

vhf avatar Nov 28 '18 16:11 vhf

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

iddan avatar Jan 25 '19 20:01 iddan

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)

awwright avatar Jan 29 '19 20:01 awwright