View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0004473 | JEDI VCL | 00 JVCL Components | public | 2008-09-23 02:01 | 2008-10-07 07:35 |
| Reporter | ivobauer | Assigned To | jfudickar | ||
| Priority | normal | Severity | major | Reproducibility | always |
| Status | resolved | Resolution | fixed | ||
| Product Version | Daily / GIT | ||||
| Target Version | Fixed in Version | ||||
| Summary | 0004473: TJvCustomAppStorage.GetStoredValues method implementation is seriously flawed | ||||
| Description | Provided that you have always the same hierarchy of folders and values, GetStoredValues method of TJvCustomAppStorage is supposed to always produce the same results regardless of a concrete storage type used, be it TJvAppRegistryStorage, TJvIniFileStorage, TJvXmlFileStorage, or any other. But in reality, the produced results vary between different concrete storage types used, which is bad. | ||||
| Steps To Reproduce | Please see the attached sample application which tries to write the same hierarchy of folders and values into TJvAppRegistryStorage, TJvIniFileStorage and TJvXmlFileStorage. Storage's Path property is set to empty string before writing is performed, so a root storage path is assumed. Every write operation specifies a relative path to the current storage path. After the data are written, the sample application tries to obtain all stored values from a given storage using GetStoredValues method. I'm passing empty string as the Path parameter indicating that I want to start searching in the current storage path (which is a root path). As Options parameter I'm passing this set [aeoValues, aeoRecursive] which means that I'm interested only in values, not folders and the searching should be recusrive through the hierarchy of folders/values. I get the expected results only with the registry storage. INI file storage returns only the values from the root level path. XML file storage returns all stored values correctly, but in addition to those it also returns all folders even they were not requested. | ||||
| Additional Information | I also tried to set storage's Path property to some other value than empty string, for example to one of the folders in the root folder, but then the GetStoredValues method returns incorrect results even for the registry storage where it previously worked. | ||||
| Tags | No tags attached. | ||||
|
2008-09-23 02:01
|
GetStoredValuesDemo.zip (387,064 bytes) |
|
|
Hi Ivo, sadly there is no easy to solve your problem. In my first view, the GetStoredValues Implementation of the storage engines is not correct. I had to think about this. Greetings Jens |
|
|
Hi Jens, Thanks for the acknowledgement of this issue. Just a few thoughts: GetStoredValues method uses InternalGetStoredValues method which in turn uses EnumFolders and EnumValues virtual abstract methods that, once overridden in the descendant classes, perform the low-level work for a particular storage. I didn't examined the code in detail, but obviously I would start by looking at the actual implementation of EnumFolders/EnumValues methods. INI file storage needs to take a special care about DefaultSection property - any values that are stored inside that section are actually stored in the root folder (which for INI file is the file itself). Any other sections must be considered as top level folders. XML file storage needs to take a special care when distinguishing between a folder and a value, which it currently does not. |
|
|
Hi Ivo, i've done a lot of investigation yesterday evening. I've also committed the first two fixes to the repository. But i think we must have a look at the implementation and logic of the code. For me, for example the logic of the options is not implemented correct and similar in the three engines. Greetings Jens |
|
|
Hi Ivo, i've updated the appstorage components in the svn repository (Revision : 11918) Please have a look on the changes. For me everything looks fine now. Greetings Jens |
|
|
Hi Ivo, please update this issue and if possible add a new sample showing the last problems. Greetings Jens |
|
|
Hi Jens, I will post a note here that describes the remaining issues along with a new sample application later today (tonight). Thanks for reopening this issue! |
|
|
Attached you will find the new demo (GetStoredValuesDemo2) that demonstrates 2 new issues I'd like to point out: Issue 0000001: When you pass [aeoFolders, aeoRecursive] as Options param to GetStoredValues(it could be reproduced with other option sets too, but I omitted them for the sake of clarity), it produces correct results for both registry storage and XML file storage, but fails completely with INI file storage. A quick look at the test results (see the attached demo 2) suggests that a call to GetStoredValues for the INI file storage does not take into consideration folders that contain only sub-folders but no values themselves. Please see the registry and INI file storage test results for the correct reference. Issue #2: As you can see from both demos, a call to GetStoredValues automatically prepends the top-level values and top-level folders with backslash (all other nested folders and values are not prepended) unless aeoReportRelative option is specified. I propose a modification to GetStoredValues method so that it prepends everything (all folders and values) with backslash *regardless* of presence or absence of aeoReportRelative option. The reasoning behind this is that (according to the comments in the source code) folders and/or values are reported *relatively* to the requested path if aeoReportRelative option is present or *relatively* to the storage current (root) path if aeoReportRelative option is not specified. In other words, regardless of whether aeoReportRelative option is present or absent, reported folders/values are always relative to some path. As relative path names usually start with a backslash (this is also in accordance with the comments in the source code), I hereby suggest to always prepend every folder and value with backslash. |
|
2008-09-27 15:12
|
GetStoredValuesDemo2.zip (386,560 bytes) |
|
|
Ugh, in the above note, it should read Issue No.1 and No.2, but it somehow got converted to an URI pointing to the Mantis issues of that numbers. |
|
|
Ivo: As soon as you put a # (hash) symbol in front of a number, it shows up as a link. As to the INI files problems, the code for them was never thought out to support folders and paths as the very nature of INI files is to be flat. If anyone manages to have compatibility here, that would be great, but it's not that easy as you just discovered. |
|
|
Hi Olivier, Re:(1) Thanks for the explanation. I now understand how issue numbers are turned into hyperlinks. :-) Re:(2) I know that INI files were not supposed to store a tree-like hierarchy, but it definitely is feasible (albeit more difficult compared to other storage types) to implement as we already know (using paths in section names). Look, JVCL offers simply awesome level of abstraction in TJvCustomAppStorage that lets you use every possible concrete descendants of that class interchangeably, export/import between them in a ridiculously easy way. But for this to work, GetStoredValues method must be implemented correctly for all descendants. Jens has already fixed the most annoying major issues and I'm sure that he is capable of fixing that minor ones as well. ;-) |
|
|
Hi Ivo, thanks for the compliments :-) I hope i can fix the most issues, it should be possible. Greetings Jens |
|
|
Hi Ivo, i've added a new revision (11938) to the svn. Please have a look on it. Greetings Jens |
|
|
Hi Jens, Thanks for then new revision, much appreciated. I'll have a look at it and perform a thorough testing as soon as time permits (tomorrow). I'll report my results here. |
|
|
I've just finished extensive tests with the latest SVN revision and I'm pleased to say that I'm no longer able to reproduce any of the known problems with GetStoredValues method, which means they all must have been fixed. Great work, Jens! I would like to thank you for your effort and time. For me, this issue can be marked as resolved. |
|
|
Hi Ivo, you're welcome. Thanks for the info and now it's solved. Greetings Jens |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2008-09-23 02:01 | ivobauer | New Issue | |
| 2008-09-23 02:01 | ivobauer | File Added: GetStoredValuesDemo.zip | |
| 2008-09-23 16:41 | jfudickar | Note Added: 0014650 | |
| 2008-09-23 16:43 | jfudickar | Status | new => acknowledged |
| 2008-09-23 22:08 | ivobauer | Note Added: 0014651 | |
| 2008-09-23 22:41 | jfudickar | Note Added: 0014652 | |
| 2008-09-24 14:13 | jfudickar | Status | acknowledged => resolved |
| 2008-09-24 14:13 | jfudickar | Resolution | open => fixed |
| 2008-09-24 14:13 | jfudickar | Assigned To | => jfudickar |
| 2008-09-24 14:13 | jfudickar | Note Added: 0014667 | |
| 2008-09-25 02:43 | jfudickar | Status | resolved => feedback |
| 2008-09-25 02:43 | jfudickar | Resolution | fixed => reopened |
| 2008-09-25 02:46 | jfudickar | Note Added: 0014687 | |
| 2008-09-25 06:25 | ivobauer | Note Added: 0014691 | |
| 2008-09-27 15:10 | ivobauer | Note Added: 0014704 | |
| 2008-09-27 15:12 | ivobauer | File Added: GetStoredValuesDemo2.zip | |
| 2008-09-27 15:15 | ivobauer | Note Added: 0014705 | |
| 2008-09-29 01:44 | obones | Note Added: 0014710 | |
| 2008-09-29 03:26 | ivobauer | Note Added: 0014711 | |
| 2008-09-29 04:13 | jfudickar | Note Added: 0014714 | |
| 2008-09-29 04:17 | jfudickar | Status | feedback => assigned |
| 2008-10-05 15:06 | jfudickar | Note Added: 0014753 | |
| 2008-10-05 15:06 | jfudickar | Status | assigned => feedback |
| 2008-10-05 15:16 | ivobauer | Note Added: 0014756 | |
| 2008-10-07 06:26 | ivobauer | Note Added: 0014775 | |
| 2008-10-07 07:33 | jfudickar | Status | feedback => resolved |
| 2008-10-07 07:33 | jfudickar | Resolution | reopened => fixed |
| 2008-10-07 07:33 | jfudickar | Note Added: 0014778 |
