Fix accessing input file metadata in MetadataSvc

[m-fila]

BEGINRELEASENOTES

• Added tests reading metadata from input file
• Fixed accessing input file metadata in MetadataSvc

ENDRELEASENOTES

In the MetadataSvc added in #215 the matadata was split into two frames:

• the first one in theIMetadataSvc collecting metadata put during current data processing,
• the second one in theMetaDataSvc (shadowing the variable from interface) holding metadata from an input file. This frame was not accessible with get and put.

In this PR I propose to have one metadata frame containing both metadata from input and anything set in current processing. The frame will be not saved if it doesn't exist:

• there was no metadata in input,
• and no metadata was added with put.

Reading parameters from not existing frame gives empty optional

[tmadlener]

One small thing I just realized as well, we have the MetadataSvc but we have MetaDataHandles. IIUC both of them will stay around right? Should we try to homogenize the naming of the two things?

Another question: Does the MetadataSvc still work with podio < 1.0? I.e. before the Frame::getParameter started to return std::optional?

[m-fila]

I feel like these are questions for @jmcarcell as he was the original author of the service

From my understanding:

• the MetaDataHandle stays not to break already existing algorithms using them and for compatibility with PodioDataSvc.
• I'd prefer to use MetadataWhatever rather than MetaDataWhatever as "metadata" is a legit word, but probably it'd be easier to do the opposite and rename toMetaDataSvc
• I didn't check but I think it should work with podio<1, since return can be implicitly converted from value to optional as long as optional return type is declared. So return frame->getParameter<T>(name); should work for frame->getParameter<T> returning either T or optional<T>

[tmadlener]

I'd prefer to use MetadataWhatever rather than MetaDataWhatever as "metadata" is a legit word, but probably it'd be easier to do the opposite and rename toMetaDataSvc

I also like the Metadata better than MetaData, but I also agree with your assessment. Let's see whether the MetaDataHandle is supposed to stay before we discuss this further.

[m-fila]

tl;dr I really don't like having and exposing public unique_ptr member as part of the interface

Having a look at this, it's possible to fix the issue of the two frames by removing the redeclaration in

https://github.com/key4hep/k4FWCore/blob/941da762dda23886e13b4a53a6341c80c1eca5ce/k4FWCore/components/MetadataSvc.h#L39 , then there will only be one frame, all the tests work and empty optional is returned when there isn't a value.

I'd argue that the frame should go from IMetaDataSvc rather than from MetaDataSvc. The IMetaDataSvc is supposed to be an interface and shouldn't contain implementation. Having implementation of set and put is already pushing it, but since they are supposed to be templates then this is the easiest way

The rest of the changes are interface changes to use getFrame and setFrame instead of accessing directly m_frame (which would be possible after deleting the redeclaration and having only one). For simplicity I prefer accessing m_frame directly,

I agree that having the setter and getter on this service isn't pretty. But I see them as lesser evil:

• The setter was already there due to coupling with IOSvc.
• The protected getter is to give access for set and put and is a workaround for having templated methods in an interface that otherwise have everything virtual
• The public const getter is due to the coupling with Writer. I think that giving access to a const raw pointer is safer than exposing a std::unique_ptr. The getter gives raw pointer becasue this is an idiomatic way for not giving an ownership - there is no need to transfer ownership to the reader as it accepts a const Frame&

I see there is a strong coupling between IOSvc and MetadataSvc and I'm not sure if overall it wouldn't be better to have a single concrete class that implements bothIIOSvc and IMetadataSvc rather than having two separate implementations that depend on each other (IOSvc even creates the MetadataSvc)

There is also a check for whether the frame is valid or not and then give you an std::nullopt but this is already implemented in getParameter

I'm not sure to which getParameter you are referring to. The service creates a frame on the first put or if there is a frame from the input data given to it by IOSvc. So there is a state in which there is no frame in the service

• podio::Frame::getParameter can't be even used if the frame wasn't created
• k4FWCore::getParameter checks service validity not whether the service holds a valid frame.

[tmadlener]

The public const getter is due to the coupling with Writer. I think that giving access to a const raw pointer is safer than exposing a std::unique_ptr. The getter gives raw pointer becasue this is an idiomatic way for not giving an ownership - there is no need to transfer ownership to the reader as it accepts a const Frame&

I suppose we can't make that return references, because it's possible that getFrame is called, when we don't have a valid m_frame yet, and we need the fact that we can do the nullptr check? I agree that the raw pointer already signifies that the caller doesn't get ownership.

[m-fila]

I suppose we can't make that return references, because it's possible that getFrame is called, when we don't have a valid m_frame yet, and we need the fact that we can do the nullptr check? I agree that the raw pointer already signifies that the caller doesn't get ownership.

Yes, getFrame should return either a valid frame or signalize invalid state, so a reference is not good for that. Optional also doesn't work since we need reference semantic and there is no std optional reference