XStream

FileStreamStrategy uses toString() of key as a file name that causes lots of problems

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 1.3
  • Fix Version/s: 1.3.1
  • Component/s: None
  • Labels:
    None
  • JDK version and platform:
    Sun 1.5 for Windows and Linux

Description

FileStreamStrategy is a implementation of StreamStrategy interface that is used by XmlArrayList, XmlSet and XmlMap for serialization of their elements as XML files. The file names are created using toString() of map key. Forbidden characters are escaped.

This method causes the following problems:
1. Length of file name in any file system is limited. Long key (even long string) causes FileStreamStrategy to create file with very long name that may fail due to the file system limitations. See FileStreamStrategyTest.testPutLongKey(). I ran this test on windows and I do not know what are the limitations on other platforms. But I detected this bug first on Linux (Fedora), so it is relevant for other platforms too.
2. Lists and Sets hold values only while Maps hold pairs, where both keys and values are important. FileStreamStrategy translates key of any type to string (using toString() method) and therefore loses its real type. Attempt to enumerate keys returns strings instead of real type. See FileStreamStrategyTest.testIntegerKey()
3. Direct call of toString() makes impossible usage of null keys. See FileStreamStrategyTest.testNullKey()
4. Method toString() does not guarantee uniqueness of string representation of the object. For example class may have 2 fields foo and bar while toString() prints only foo as more important. In this case 2 different objects (equals returns false and hashCode() returns different values) will be overridden because produced file names are identical. See FileStreamStrategyTest.testFalseToString()

I added the tests mentioned above and changed the implementation of FileStreamStrategy. Now it creates file names like ClassName@HashCode (like Object.toString() does). I could create other implementation of StreamStrategy or extend FileStreamStrategy, but I decided to change the existing implementation because I think that limitations and bugs listed above are too serious.

5. My implementation has one limitation. hashCode() method does not guarantee unique value. Two objects o1 and o2 when o1.equals(o2) is false may have the same hashCode(). So in this case 2 different files should be created. But what to do with these files' names? Adding indexes 0, 1, ... may cause serious performance problems for big collections because in this case every time we add item to collection we have to call File.list() with filter and count the returned items.

So I decided not to fix FileStreamStrategy. It remains with this limitations and can be used when hashCode of key returns unique number (e.g. for numbers or other custom types that guarantee the hash code uniqueness). I added other subclass
DirFileStreamStrategy and test case DirFileStreamStrategyTest that contains test testBadKey() that reproduces the problem.

Other changes
--------------------
1. These fixes allowed me to simplify FileStreamStrategy that does not contain now sophisticated logic of encoding/decoding key string representation in order to use it as a file name. Appropriate tests are removed from FileStreamStrategyTest as well.
2. XmlSet is now simplified. Now it wraps XmlMap and uses elements as the map's keys exactly like java.util.HashSet does.

Please see attached zip file with relevant source files. I did not use SVN when working on this patch. I just downloaded the latest release source code and changed it. I'd be happy to work with SVN repository directly in future.
I hope my changes will be approved and I will be happy to contribute my code to this project.

Please contact me: alexander_radzin@yahoo.com

Attachments

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Joerg Schaible added a comment -

Hi Alexander,

your issue has some valid critics and I will therefore comment on the different topics:

  1. Length of the file name: It is in the nature of a key, that it should not exceed length to ultimo. Think of a DB where you also may define a key with a limit although a String might be much longer. The same applies here. If someone wants to use the keys of a map as key, it should be part of the application's design to have some constraints on the key length.
  2. I was not aware that the implementation silently assumes that all keys are strings and that it will unmarshal any key really as String value. This has been fixed now. I followed to some extend your approach to use the class as prefix.
  3. Null values can be used now.
  4. Here the same applies as for the first point. It is part of the application's design to ensure uniqueness of its keys. However, this time it is required that the key type can be converted into a meaningful String by XStream using a SingleValueConverter.

Basically I did also not fix the FileStreamStrategy (apart from the null key), but I deprecated it (and the interface StreamStrategy) in favour of the new interface PersistenceStrategy and FilePersistenceStrategy, which will express better that a file storage is only one possibility. The new implementation is backward compatible though. However, if someone wants to create own files for the keys, it is no problem now to implement something like this. It is simply not the kind of standard use case.

The other topics:

  1. Modern file systems do write utf-8 characters therefore I've only encoded characters that are actually forbidden for the most populat file systems (using %XX).
  2. It is the intention of XmlSet and XmlArrayList that the values are stored in separate files and the (generated) indices are "lightweight".

Thanks a lot for your contribution and thoughts about XStream's persistent classes, it was highly welcome.

  • Jörg
Show
Joerg Schaible added a comment - Hi Alexander, your issue has some valid critics and I will therefore comment on the different topics:
  1. Length of the file name: It is in the nature of a key, that it should not exceed length to ultimo. Think of a DB where you also may define a key with a limit although a String might be much longer. The same applies here. If someone wants to use the keys of a map as key, it should be part of the application's design to have some constraints on the key length.
  2. I was not aware that the implementation silently assumes that all keys are strings and that it will unmarshal any key really as String value. This has been fixed now. I followed to some extend your approach to use the class as prefix.
  3. Null values can be used now.
  4. Here the same applies as for the first point. It is part of the application's design to ensure uniqueness of its keys. However, this time it is required that the key type can be converted into a meaningful String by XStream using a SingleValueConverter.
Basically I did also not fix the FileStreamStrategy (apart from the null key), but I deprecated it (and the interface StreamStrategy) in favour of the new interface PersistenceStrategy and FilePersistenceStrategy, which will express better that a file storage is only one possibility. The new implementation is backward compatible though. However, if someone wants to create own files for the keys, it is no problem now to implement something like this. It is simply not the kind of standard use case. The other topics:
  1. Modern file systems do write utf-8 characters therefore I've only encoded characters that are actually forbidden for the most populat file systems (using %XX).
  2. It is the intention of XmlSet and XmlArrayList that the values are stored in separate files and the (generated) indices are "lightweight".
Thanks a lot for your contribution and thoughts about XStream's persistent classes, it was highly welcome.
  • Jörg
Hide
Permalink
Joerg Schaible added a comment -

Set correct fix version.

Show
Joerg Schaible added a comment - Set correct fix version.
Hide
Permalink
Joerg Schaible added a comment -

Fixed for upcoming release.

Show
Joerg Schaible added a comment - Fixed for upcoming release.

People

Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Codehaus. Try JIRA - bug tracking software for your team.