possible bug in VersionedObject.java

Reference Implementation: Java (archive)
1

Please correct me if I am wrong, but I think I found a bug in the Java-code.

In this source we find following code snippet

// in order to skip map timeVersionMap to table
void setVersions(Set<Version<T>> versions) {
idVersionMap = new HashMap<ObjectVersionID, Version<T>>();
timeVersionMap = new TreeMap<DvDateTime, Version<T>>();
for(Version<T> version : versions) {
addVersion(version);
}
}

I discover unpredictable behavior ot this, and in some cases leading to an error.

Because the Versions in the Set-collection are iterated through and put to the “addVersion” method, where the trunkcounter is checked in this way.

if (!version.getUid().versionTreeID().isBranch()) {
int trunkNo = Integer.parseInt(version.getUid().versionTreeID().trunkVersion());
if (trunkNo != trunkCounter + 1) {
throw new IllegalArgumentException("invlalid trunk no in uid");
} else {
trunkCounter++;
latestTrunkUid = version.getUid();
}
}

As you can see, the trunkcounter is aprt of the versiontreeId, which is part of the Uid of a particular version, and thus, known before adding it to the versioned-object, and checked in the versionedObject.
Normally this is no problem, because, the versions are added sequentially with commit…Version, and the trunkcounter is the responsibility of the programmer writing an application, using the kernel.
If the trunkcounter is not OK, it is his/her problem, he/she should do his/her job better.

But in the case of “setVersions”, see above, it is the responsibility of kernelcode to deliver the particular versions ordered in the right way.
And this is where the bug comes in, this is not guaranteed by using the Set in this way, because the Set (HashSet) iterator, iterates through its members on hashcode, or some other internal way, but not on trunkcounter (which is part of the Uid).
So there is no guarantee the versions are deliverd in the right order, and this makes the method “setVersions” unpredictable, and it can lead to unnecessary exceptions in the method “addVersion”

Some thoughts about a solution
In case I am right, a possible solution to this problem would be to use a SortedSet, which possible can have a performance penalty, but on the other hand, VersionedObjects typically have a few Versions (f.e. less then 100), and the setVersions-method is only use in POJO-style-coding, which is for example while reading a VersionedObject from a database, which already can be queried in “order by”, so the ordering has been done by the highly optimized database-engine.

I would appreciate when, if it is a problem, it will be solved, so I can keep my code as much as possible in line with the ref_java_impl.

regards
Bert

2

Please correct me if I am wrong, but I think I found a bug in the Java-code.

In this source we find following code snippet

// in order to skip map timeVersionMap to table
void setVersions(Set<Version<T>> versions) {
idVersionMap = new HashMap<ObjectVersionID, Version<T>>();
timeVersionMap = new TreeMap<DvDateTime, Version<T>>();
for(Version<T> version : versions) {
addVersion(version);
}
}

I discover unpredictable behavior ot this, and in some cases leading to an error.

Because the Versions in the Set-collection are iterated through and put to the “addVersion” method, where the trunkcounter is checked in this way.

if (!version.getUid().versionTreeID().isBranch()) {
int trunkNo = Integer.parseInt(version.getUid().versionTreeID().trunkVersion());
if (trunkNo != trunkCounter + 1) {
throw new IllegalArgumentException("invlalid trunk no in uid");
} else {
trunkCounter++;
latestTrunkUid = version.getUid();
}
}

As you can see, the trunkcounter is aprt of the versiontreeId, which is part of the Uid of a particular version, and thus, known before adding it to the versioned-object, and checked in the versionedObject.
Normally this is no problem, because, the versions are added sequentially with commit…Version, and the trunkcounter is the responsibility of the programmer writing an application, using the kernel.
If the trunkcounter is not OK, it is his/her problem, he/she should do his/her job better.

But in the case of “setVersions”, see above, it is the responsibility of kernelcode to deliver the particular versions ordered in the right way.
And this is where the bug comes in, this is not guaranteed by using the Set in this way, because the Set (HashSet) iterator, iterates through its members on hashcode, or some other internal way, but not on trunkcounter (which is part of the Uid).
So there is no guarantee the versions are deliverd in the right order, and this makes the method “setVersions” unpredictable, and it can lead to unnecessary exceptions in the method “addVersion”

Some thoughts about a solution
In case I am right, a possible solution to this problem would be to use a SortedSet, which possible can have a performance penalty, but on the other hand, VersionedObjects typically have a few Versions (f.e. less then 100), and the setVersions-method is only use in POJO-style-coding, which is for example while reading a VersionedObject from a database, which already can be queried in “order by”, so the ordering has been done by the highly optimized database-engine.

Hi Bert,

I think your analysis on this is correct. The current setVersions / allVersions / allVersionIDs methods are rudimentary and would need fixes. I suggest we use SortedMap instead of the current HashMap for attribute idVersionMap. This should fix the issue you reported.

It will be great if you can supply a testcase on this to verify the fix.

Cheers,
Rong

3

Rong Chen schreef:

Please correct me if I am wrong, but I think I found a bug in the Java-code.

In this source we find following code snippet

// in order to skip map timeVersionMap to table
void setVersions(Set<Version<T>> versions) {
idVersionMap = new HashMap<ObjectVersionID, Version<T>>();
timeVersionMap = new TreeMap<DvDateTime, Version<T>>();
for(Version<T> version : versions) {
addVersion(version);
}
}

I discover unpredictable behavior ot this, and in some cases leading to an error.

Because the Versions in the Set-collection are iterated through and put to the “addVersion” method, where the trunkcounter is checked in this way.

if (!version.getUid().versionTreeID().isBranch()) {
int trunkNo = Integer.parseInt(version.getUid().versionTreeID().trunkVersion());
if (trunkNo != trunkCounter + 1) {
throw new IllegalArgumentException("invlalid trunk no in uid");
} else {
trunkCounter++;
latestTrunkUid = version.getUid();
}
}

As you can see, the trunkcounter is aprt of the versiontreeId, which is part of the Uid of a particular version, and thus, known before adding it to the versioned-object, and checked in the versionedObject.
Normally this is no problem, because, the versions are added sequentially with commit…Version, and the trunkcounter is the responsibility of the programmer writing an application, using the kernel.
If the trunkcounter is not OK, it is his/her problem, he/she should do his/her job better.

But in the case of “setVersions”, see above, it is the responsibility of kernelcode to deliver the particular versions ordered in the right way.
And this is where the bug comes in, this is not guaranteed by using the Set in this way, because the Set (HashSet) iterator, iterates through its members on hashcode, or some other internal way, but not on trunkcounter (which is part of the Uid).
So there is no guarantee the versions are deliverd in the right order, and this makes the method “setVersions” unpredictable, and it can lead to unnecessary exceptions in the method “addVersion”

Some thoughts about a solution
In case I am right, a possible solution to this problem would be to use a SortedSet, which possible can have a performance penalty, but on the other hand, VersionedObjects typically have a few Versions (f.e. less then 100), and the setVersions-method is only use in POJO-style-coding, which is for example while reading a VersionedObject from a database, which already can be queried in “order by”, so the ordering has been done by the highly optimized database-engine.

Hi Bert,

I think your analysis on this is correct. The current setVersions / allVersions / allVersionIDs methods are rudimentary and would need fixes. I suggest we use SortedMap instead of the current HashMap for attribute idVersionMap. This should fix the issue you reported.

It will be great if you can supply a testcase on this to verify the fix.

That is a bit a problem, because, a normal Set iterates using the hashcode, this means that you need my hashcode-routine, which is different from the original one, and you need my objects which trigger the error. It was difficult for me to debug this, because, these are complicated situations.

I think, SortedSet can repair this problem, but it is a complicated solution (I use it now), a better solution would be to remove/change deep logic from POJO-methods. The problem is that the addVersion-method demands a special order (depending on the VersionTreeID’s).
This could be removed at that point, or sort the entries after being retreived from the setVersions-method parameter, but before passing it to the addVersion-routine. Sorting should then happen on VersionTreeID. But this is performance-eating, (not much (because, there are never many versions in a versionedobject, but still).
So the best solution, maybe is to write another addVersion-method which is special for use in the POJO setVersion-method. That only checks afterwards if the Trunkcounter is OK, so the order will not be important then.

In the meantime, I have a SortedSet setting the Versions in the right order, but this happens outside the RM-code, this works for me.

thanks,
Bert