Read accessors throw or return the wrong type depending on call order #333

Note on provenance: This issue was found by Claude Opus 5 while reviewing #325. The reproductions and output shown below were produced by building and running the code, but I have only briefly reviewed the analysis myself and I am not an expert in this area. Please review it critically.

Impact

The object cache is keyed on path alone, with no record of what type was stored there. Once something has been cached for a path, asking for that path again as a different type does not do what you would expect, and what goes wrong depends on which factory you happen to call.

For a user this shows up as order dependence in ordinary read code. Read a table's rows and then ask for a typed column and you get an uncaught std::runtime_error out of a documented read accessor. Do the same two calls in the other order and everything works. Nothing in the API hints that the order matters.

The related failure is quieter. The two non template create overloads hand back whatever is cached without checking it against the type you asked for, so you can request one type and receive another, with no error and no log.

What happens

Both reproduced against bae5dda4.

Uncaught throw, on a reopened file containing an EventsTable:

readRows(0,1)  then  readTimestampColumn()
  -> THREW std::runtime_error:
     "Attempting to add a different instance of a RegisteredType with the same path."

readTimestampColumn()  then  readRows(0,1)
  -> fine

Wrong type returned, with a Container already cached at a path:

create("core::TimeSeries",     "/example/path")  ->  Container   (same pointer)
create("unknown::UnknownType", "/example/path")  ->  Container   (same pointer)

Not nullptr, not the requested type, and no diagnostic. The second case is the more surprising one: a request for a type that does not exist at all still returns a live object.

Cause

Three entry points, three different policies for the same situation.

auto existing = getExistingRecordingObject(path, io);
if (existing) {
  auto typed = std::dynamic_pointer_cast<T>(existing);
  if (typed) return typed;
}
auto result = std::shared_ptr<T>(new T(path, io));
result->registerRecordingObject();     // throws when a different instance is already at this path

throw std::runtime_error("Attempting to add a different instance of a RegisteredType with the same path.");

The base revision's comment at this site described the intended contract, "If no object exists for the path, or if the existing object is of a different type, create a new instance". That comment was removed and the different type branch was never implemented.

Suggested direction

Three steps, smallest first. Any one of them removes the throw in practice, but the third is what makes the behaviour consistent.

1. Stop polluting the cache. Have readColumn and loadConfiguredColumnsFromFile resolve each column's actual type from the file, which the two argument create already does via getFullTypeName(path) at RegisteredType.cpp:141, instead of hardcoding VectorData.
2. Make a genuine mismatch legible. In the template create<T>, return nullptr on a failed cast rather than constructing a second instance. Callers already null check, and the cache key stays path only.
3. Apply the same policy to the two non template overloads, which today do not check at all.

Folding the requested type into the lookup itself would give all three a single policy, at the cost of a larger change to the cache key.

Notes

VectorDataTyped::create at src/nwb/hdmf/table/VectorData.cpp:274-287 and DataTyped::create each hand roll a workaround for exactly this situation, deliberately returning an unregistered instance. Every subclass that uses the plain REGISTER_SUBCLASS factory, including TimestampVectorData, DurationVectorData, VectorIndex, ElementIdentifiers and TimeIntervals, has no such guard.

No test covers the failing order. #325 adds nine io->getRecordingObjects()->clear() calls to tests/testRegisteredType.cpp, where there were none before, several commented "Clear the cache to ensure we create a new instance". Those clears are what keep the existing assertions holding.

Originally raised as a review thread on #325 (src/nwb/RegisteredType.cpp:94). The thread was resolved, but no commit after it touched RegisteredType.{hpp,cpp} or RecordingObjects.{hpp,cpp}, and both symptoms still reproduce on the PR head.