Error when reading "old" streams

[Candesco]

Changes to TVirtualNodeState have made the current stream format incompatible to the old one. TBaseChunkBody now has a size of 19 bytes, instead of 17.

[Candesco]

This error was introduced with V6.2, when vsInitializing was added.

[joachimmarder]

I never used the streaming possibilities to persist a virtual treeview, and given the fact that no one reported this issue for a long time I guess it is not widely used. I don't think that that streaming a virtual treeview for persistence is is a good fit for the virtual paradigm.

I don't think there is a proper solution for this issue now, because there are many streams that have been written with V6.2 - V6.7 and we can't distinguish them from those written with <=V6.1.

[Candesco]

I don't think that this feature isn't used. I personally know of two other developers who used them in the past. I guess people just don't upgrade those projects. As you could guess by me finding this issue just now. ;-)

I agree that this can't be resolved in a clean way. However, it has to be resolved somehow. And since there will not be a clean solution for all cases, I'd suggest to fix the error itself by increasing the stream version number and introducing code that can be used to read a stream that doesn't contain vsInitializing. I would be willing to provide all that through a pull request. Furthermore, I'd set all the custom types to 32bit, so that an increase won't be a problem for any future versions.

Just tell me if that sounds good to you.

[joachimmarder]

I'd suggest to fix the error itself by increasing the stream version number

This would have been a correct and good solution in V6.2, but I don't see any sense in doing it now, because the stream format did not change in V6.2 - V7.0.

introducing code that can be used to read a stream that doesn't contain vsInitializing.

How do you intend to detect which kind of stream you have?

I would be willing to provide all that through a pull request.

I am happy to accept this pull request as long as the code quality is OK. One problem with this kind of migration code is that you have it in the class forever, but it is not widely used.

Furthermore, I'd set all the custom types to 32bit, so that an increase won't be a problem for any future versions.

This again would have been a great idea in V6.2, doing it now without an actual cause seems overhead to me. But this may depend on the answers to my questions above.

[Candesco]

This would have been a correct and good solution in V6.2, but I don't see any sense in doing it now, because the stream format did not change in V6.2 - V7.0.

The stream version 2 was in use for a very long time. V6.2 is 2 years old. And I am convinced that many haven't yet migrated. But the main reason, for me, would be to avoid having to use any migration code more than once.

How do you intend to detect which kind of stream you have?

There really are 3 possibilites.

1: Right after TVirtualNodeStates comes Align: Byte. This value is usually 50, but anything above 1 would mean we can recognize an old stream perfectly, because it would result in an invalid value for States. Even the 1 should never be there, because that is a temporary state that is only used for a very short time and it would be very strange to save to a stream during that time. So, basically every Align value above 0 will help us.

2: A "scan read" would tell us if the stream contains anything unusal (e.g. strange chunk types or the endposition differs from the one in the header) in which case we start a subroutine that reads again with the 17 byte header.

3: The user himself could specify which chunk size it is.

This again would have been a great idea in V6.2, doing it now without an actual cause seems overhead to me. But this may depend on the answers to my questions above.

I tend to disagree there: The real problem is simply that someone forgot to change the stream version (despite the big warning comment above TVirtualNodeState reminding everyone about it) when he changed the enum. This could very well be happening again in the future and should be avoided.

One more suggestion: If you want, I'd consider redoing the whole stream format as JSON or BSON, which might eliminate all future migration problems and even make the format manageable outside the tree.

[joachimmarder]

Right after TVirtualNodeStates comes Align: Byte. This value is usually 50

Yes, that could work. I was not aware of that detail.

I'd consider redoing the whole stream format as JSON or BSON, which might eliminate all future migration problems and even make the format manageable outside the tree.

I'm open here. I personally neither used the streaming feature nor did I touch the code since I started contributing to this project. My primary concern is to keep the project stable and to avoid almost unused code that is not of general interest but also cannot be easily removed.

In the past years we carved some code into own units to make the main unit VirtualTrees more manageable, see e.g. issue #535. It would great if this could be done here too if you are going to rewrite the code.

[Candesco]

Ok, I'll go with the align-solution then. It should cover most (if not all) cases. But we will need a change to the stream version, so that we can further reduce the risk here.

If you're open to the JSON and suggest a new Unit for this (how about VirtualTrees.Export.pas?), I'll do the necessary changes over the next few days. Do you think the user should have the option to choose between stream and JSON? I think I'd prefer it that way, since the stream can be more compact and faster, but the JSON is way better suited for transportation or projects with a longer lifecycle.

I should be done with it before the end of next week, but can't give you a more exact date because of my other projects.

[joachimmarder]

But we will need a change to the stream version, so that we can further reduce the risk here.

That shouldn't be a big overhead as the new stream version 3 will need to do the same as the align-version needs to do after it detected the new stream format. In case you properly extract the detection of the stream version of V6.2-V6.7 then you can simply assume stream version 3 and go on.

If you're open to the JSON and suggest a new Unit for this (how about VirtualTrees.Export.pas?)

In case you are able to extract both reading and writing code, I would prefer a new unit "VirtualTrees.JSON". Are you planning to use the RAD Studio unit in the System.JSON namepace?

Do you think the user should have the option to choose between stream and JSON?

More options mean more code to maintain. So I think any option should come with a true benefit. But since I don't use that streaming stuff, I may not be the best person to decide this. We can create a new issue for removing the old code for binary streaming and label it with "Open for discussion".

[Candesco]

Are you planning to use the RAD Studio unit in the System.JSON namepace?

Yes and that is one more reason to give the developer a choice to use it, since the System.JSON is still relatively new.

[Candesco]

And are you sure about VirtualTrees.JSON? I find it rather unfitting, since even if we change the streaming now, the unit is not about JSON. Maybe .IO or .Streaming?

[joachimmarder]

I was expecting VirtualTrees.JSON to include only stream read and wite code for JSON. If this assumption was wrong, the name of course does not make sense.

I don't really careabout the name, but I find it important to use more / separate units and not to put all in the main unit.

[Candesco]

Well, you said you'd like to extract some more code to other units. So I'll try to get the whole streaming code out of the main unit. Don't know if I can do that, but we'll see.

I'll get back to you in a few days.

[joachimmarder]

Sounds good. We should try with the JSON code first, and later see what todo with the old streaming code. Especially new code (if is is significant in size) should be added in new units to not repeat the failures of the past.

[Candesco]

I'm sorry for the delay, but work got me way more busy than I expected. I've sent you a pull request with a fix for the basic issue for now.

The rest will come eventually, but I'll need some time to do it. Maybe in about two weeks, but I don't want to over-promise again, so please don't hold me to that.

[joachimmarder]

No need to be sorry for the delay. Please open a new issue for the JSON feature, I will close this issue as it is fixed and it was a bug, but JSON is anew feature. Thanks for the pull request!