fix: Support non-spec MOS data better

nytamin commented 8 months ago

About the Contributor

This PR is posted on behalf of SuperFly.tv

Type of Contribution

This is a: Code improvement. An attempt at making mos-connection better at handling off-spec messages.

This PR is another try at fixing the issues behind #84

Current Behavior

If we receive a message that is not according to the specification, like for example if calling await mosDevice.requestMachineInfo() and receiving the reply <listMachInfo>:

<listMachInfo>
    <manufacturer>RadioVision, Ltd.</manufacturer>
    <model>TCS6000</model>
    <hwRev>0</hwRev>
    <swRev>2.1.0.37</swRev>
    <DOM>0</DOM>
    <SN>927748927</SN>
    <ID>airchache.newscenter.com</ID>
    <time>2009-04-11T17:20:42</time>
    <opTime>2009-03-01T23:55:10</opTime>
    <mosRev>2.8.2</mosRev>
    <supportedProfiles deviceType="NCS">
        <mosProfile number="0">YES</mosProfile>
    </supportedProfiles>
    </listMachInfo>

If <time> is missing: Returns an object where time is populated with a MosTime with value Date.now()
If <time> is empty: Throws with Error: MosTime: Invalid input: \"[object Object]\"
If <time> is badly formatted: Throws with Error: MosTime: Invalid timestamp: \"BAD DATA\"
If <opTime> is missing: Returns an object where time is populated with a MosTime with value Date.now()
If <opTime> is empty: Throws with Error: MosTime: Invalid input: \"[object Object]\"
If <opTime> is badly formatted: Throws with Error: MosTime: Invalid timestamp: \"BAD DATA\"

Why this is bad:

(Note: According to the MOS protocol, <time> is required and <opTime> is optional.)

How the library handles bad data is inconsistent: 1 and 2 (or 4 and 5) are arguably similar in xml-land, but treated very differently.
When using the library, it is hard to figure out if the value in <time> is real or fictional (like in 1 or 4).

New Behavior

If calling await mosDevice.requestMachineInfo():

If in strict mode:

If <time> is missing: Throws with Error: MosTime: Invalid input: "undefined"
If <time> is empty: Throws with Error: MosTime: Invalid input: "undefined"
If <time> is badly formatted: Throws with Error: MosTime: Invalid timestamp: "BAD DATA"
If <opTime> is missing: Returns an object where opTime is undefined.
If <opTime> is empty: Returns an object where opTime is undefined.
If <opTime> is badly formatted: Throws with Error: MosTime: Invalid timestamp: "BAD DATA"

If in non-strict mode:

If <time> is missing: Returns an object where the time field is populated with a MosTime with value 0
If <time> is empty: Returns an object where the time field is populated with a MosTime with value 0
If <time> is badly formatted: Throws with Error: MosTime: Invalid timestamp: "BAD DATA"
If <opTime> is missing: Returns an object where opTime is undefined
If <opTime> is empty: Returns an object where opTime is undefined
If <opTime> is badly formatted: Throws with Error: MosTime: Invalid timestamp: "BAD DATA"

Testing Instructions

Basic regression testing should be enough.

Other Information

Status

[x] PR is ready to be reviewed.
[ ] The functionality has been tested by the author.
[x] Relevant unit tests has been added / updated.
[x] Relevant documentation (code comments, system documentation) has been added / updated.

codecov-commenter commented 8 months ago

Codecov Report

Attention: 77 lines in your changes are missing coverage. Please review.

Comparison is base (8591c99) 78.17% compared to head (aca05c3) 77.21%. Report is 1 commits behind head on master.

Files	Patch %	Lines
...ages/helper/src/mosModel/profile0/xmlConversion.ts	10.00%	15 Missing and 3 partials :warning:
...ages/helper/src/mosModel/profile2/xmlConversion.ts	38.09%	13 Missing :warning:
packages/helper/src/mosModel/lib.ts	10.00%	8 Missing and 1 partial :warning:
packages/connector/src/MosDevice.ts	91.30%	8 Missing :warning:
packages/helper/src/utils/Errors.ts	14.28%	5 Missing and 1 partial :warning:
packages/helper/src/mosModel/parseMosTypes.ts	85.18%	4 Missing :warning:
packages/model/src/mosTypes/mosString128.ts	33.33%	4 Missing :warning:
packages/model/src/mosTypes/mosTime.ts	80.95%	4 Missing :warning:
...ages/helper/src/mosModel/profile1/xmlConversion.ts	40.00%	3 Missing :warning:
packages/model/src/mosTypes/mosDuration.ts	25.00%	3 Missing :warning:
... and 4 more

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #88 +/- ## ========================================== - Coverage 78.17% 77.21% -0.96% ========================================== Files 63 65 +2 Lines 3422 3634 +212 Branches 778 848 +70 ========================================== + Hits 2675 2806 +131 - Misses 668 744 +76 - Partials 79 84 +5 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jstarpl commented 8 months ago

Hello!

Thank you for contributing to the Sofie Project!

If you haven’t already, please give our contribution guidelines a read. We will reach out to you to schedule an initial discussion regarding this (more details to follow).

jstarpl commented 8 months ago

We don't think we need a discussion on this. More leeway for not-entirely MOS-compliant systems seems a good idea.

nrkno / sofie-mos-connection