SNEWS2 / SNEWS_Publishing_Tools

Publishing Tool for SNEWS
BSD 3-Clause "New" or "Revised" License
2 stars 2 forks source link

Default Tier Message Creation #85

Open KaraMelih opened 9 months ago

KaraMelih commented 9 months ago

The current implementation allows for creating all

SNEWSHeartbeatMessage, SNEWSTimingTierMessage, SNEWSSignificanceTierMessage, SNEWSCoincidenceTierMessage, SNEWSRetractionMessage

type messages. Under the snews_pt.messages class they are all defined as a child class of the SNEWSMessage and have their respective reqfields which is checked with has_required_fields() function upon creation.

However, these required fields are defined as follows;

class SNEWSCoincidenceTierMessage(SNEWSMessage):
    """Message for SNEWS 2.0 coincidence tier."""

    reqfields = [ 'neutrino_time' ]
    fields = SNEWSMessage.basefields + reqfields + [ 'machine_time', 'p_val', 'is_test' ]

    def __init__(self, neutrino_time=None, p_val=None, **kwargs):
        super().__init__(self.fields,
                         neutrino_time=self.clean_time_input(neutrino_time),
                         p_val=p_val,
                         **kwargs)

    def is_valid(self):
       ....

So that there are always defaults for the required fields. The validation ( self.is_valid() ) is checked only later when the message is submitted, and then it properly shows that the created message is not valid.

Do we want this behavior or should we also immediately check if the message has required fields and is valid?

There is a second danger with the SNEWSCoincidenceTierMessage where it creates a default message with the current time as the neutrino time. So by default, it has all the required fields (i.e. neutrino time) and it is valid, which might lead to problems.

EDIT: if the default value is None function has_required_fields() actually does complain. The issue arise from the clean_time_input() function which is invoked before the has_required_fields() and replaces None with -> datetime.now() so, it silently becomes a valid message.

KaraMelih commented 9 months ago

Similar problem also occurs in SNEWSTimingTierMessage class

class SNEWSTimingTierMessage(SNEWSMessage):
    """Message for SNEWS 2.0 timing tier."""

    reqfields = [ 'timing_series' ]
    fields = SNEWSMessage.basefields + reqfields + [ 'machine_time', 'p_val', 'is_test' ]

    def __init__(self, p_val=None, timing_series=None, **kwargs):
        super().__init__(self.fields,
                         p_val=p_val,
                         timing_series=[self.clean_time_input(t) for t in timing_series],
                         **kwargs)

by default the timing_series argument is None and __init__ function tries to iterate over None. If it is a list of none i.e. [None] then it would also create a message with the current time.

It might be better if the self.clean_time_input() function rejects, or at least complains about None's and tells the user "time was None appending the current datetime" otherwise it will silently change the value without raising any suspicion