Review exception hierarchy for v3

[jeremy-visionaid]

As part of reviewing OpenMcdf for use in another project, I'd like to suggest and upstream some breaking changes that would be useful for creating an improved new major version.

The first thing I noticed when reviewing the existing code is that exception types are supposed to represent classes of error. However, OpenMcdf defines its own hierarchy where base library types already exist. Therefore, I think it would be good to review the exception heirarchy to throw the idiomatically correct types.

The easier/less contentious recommendations are:
CFDisposedException should be replaced by ObjectDisposedException
CFItemNotFound should be replaced by KeyNotFoundException

CFException could reasonably be used to represent errors in a compound document, but almost all instances of it being thrown aren't to do with the compound document itself, and are generally related to invalid arguments and/or invalid state. So, those instances should be replaced by ArgumentException, ArgumentNullException, ArgumentOutOfRangeException, NotSupportedException and InvalidOperationException as/where appropriate.

CFFileFormatException could derive from FormatException instead of CFException and/or just replace CFException (e.g. like FileFormatException : FormatException)

As part of reviewing the existing error handling, it's also apparent that there are functional errors (i.e. bugs) in the precondition checks. However, I don't think those problems can be fully resolved without also addressing the exception hierarchy first.

[jeremy-visionaid]

I've put an initial effort at improving the exception hierarchy here to get some early feedback:
https://github.com/Visionaid-International-Ltd/openmcdf/tree/exceptions
However, I note that some of them are still the wrong type and I would need to review them again before creating a pull request. Ideally this would also be against a branch for v3 since they are obviously breaking changes.

I've also put some changes that begin fixing the precondition checks towards the end of the chain to give an indication of the kind of errors that I can see.

[ironfede]

Thank you @jeremy-visionaid . What do you think @jeremy-visionaid and @Numpsy if I start a new branch (dev3) to start diverging and introducing breaking changes for the next major version and keep 2.x branch stable commiting to it only non breaking changes?

[jeremy-visionaid]

@ironfede That sounds perfect, thanks! I spent a day making some various changes to explore what else could be done in the codebase for v3. I've assembled some initial thoughts in a draft email where I can see things could be improved. I could create some new issues for other v3 tasks, but maybe you'd prefer just one issue for now to see what you think about the review points and how you'd like to proceed? And then we keep this issue about the exception hierarchy/error handling.

The main actual feature I'd like to work towards implementing for v3 is to allow pending changes to be made to some non-volatile storage (temp/scratch file) rather than in heap allocated memory. The files my existing app works with are very large (routinely over a gigabyte), and we require the ability to stage changes and commit them (save) later. With a few files open at once, the app could easily exceed the available memory/address space of the process (esp. in dotnet where you don't have the full address space available in the first place with multiple heaps, Windows large address awareness issues, etc.).

I think before we branch off for v3 though, we could do some work to apply some more formatting changes and introduce some editorconfig and/or stylecop configuration. If you're open to that, then just applying automatic fixes from the IDE seems like it can knock off near a couple of thousand lines of code, which would make it easier/faster to work on without breaking anything on master/v2.

[ironfede]

I'm going to create branch 3.0.0. @jeremy-visionaid do you think we can start using new editorconfig only in branch 3 without applying to 2.x branches?
Thanks!

[jeremy-visionaid]

@ironfede Thanks for merging those! I think we could/should do one last round of formatting changes on 2.x if you're OK with doing so? Then I can do some non-breaking functional changes to fix some bugs and improve perf/correctness on 2.x before making changes for 3.x (my exceptions branch is just an example of the type of stuff that I found while reviewing the code). It should be easier to back-port things to 2.x if any other bugs turn up while working on 3.x if we get all the formatting stuff out of the way first. Even if we branch now, we could prob still merge/cherry-pick, but I think it likely creates a bit more work. I think I've got one more round of that pretty much ready to go, so I'll do a PR for it and leave it up to you if you like 👍

[jeremy-visionaid]

BTW, what do you think about file-scoped namespaces? It would involve bumping the language version and affects pretty much every line of code, conflicting with basically every other PR. I find having the extra horizontal space easier to work with and a worthwhile change in the long-term despite the one-off breakage/hassle it causes. I could put that at the end and then it's easy to rebase off the PR if you don't want it.

[ironfede]

@ironfede Thanks for merging those! I think we could/should do one last round of formatting changes on 2.x if you're OK with doing so? Then I can do some non-breaking functional changes to fix some bugs and improve perf/correctness on 2.x before making changes for 3.x (my exceptions branch is just an example of the type of stuff that I found while reviewing the code). It should be easier to back-port things to 2.x if any other bugs turn up while working on 3.x if we get all the formatting stuff out of the way first. Even if we branch now, we could prob still merge/cherry-pick, but I think it likely creates a bit more work. I think I've got one more round of that pretty much ready to go, so I'll do a PR for it and leave it up to you if you like 👍

Ok. Please, consider that those last changes will possibly become a 2.4 release leading to feature freeze the 2.x branch in order to be free to break API for 3.x branch.
@Numpsy and @jeremy-visionaid , I think that we can go with .net 8 as a base framework for 3.x branch since 6 will go out of support during this year.

[jeremy-visionaid]

That's great. I've been working under the assumption that my changes for the next few weeks are for 2.x in order to get 2.4 in as good a shape as possible👍Internally, we're hoping to release an application against 3.0 probably around December, but I can release against a fork if we're not done in time. Thanks again!

[Numpsy]

I think that we can go with .net 8 as a base framework for 3.x branch since 6 will go out of support during this year.

Sounds good (I'm currently using it in a .NET 6 app, but I won't need to update to v3 until after .NET 6 has gone out of support so 8 should be good there)

[jeremy-visionaid]

Closing as complete as part of #194

[Numpsy]

Small question related to public exception types - Is it intended that KeyNotFoundException from places like

openmcdf/OpenMcdf/DirectoryEntries.cs

Line 32 in 532b8b8

throw new KeyNotFoundException($"Directory entry {streamId} was not found.");

be exposed to end users?

Just thought I'd check as I got round to giving it a run through SharpFuzz and if finds things lke

System.Collections.Generic.KeyNotFoundException: Directory entry 0 was not found.
   at OpenMcdf.DirectoryEntries.GetDictionaryEntry(UInt32 streamId)
   at OpenMcdf.RootContext..ctor(RootContextSite rootContextSite, Stream stream, Version version, IOContextFlags contextFlags)
   at OpenMcdf.Fuzz.Program.<>c.<Main>b__0_0(ReadOnlySpan`1 span) in S:\DevTest\FuzzOpenMcdf\Program.cs:line 16