Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Support non-spec MOS data better #88

Merged
merged 10 commits into from
Jan 16, 2024
Merged

fix: Support non-spec MOS data better #88

merged 10 commits into from
Jan 16, 2024

Conversation

nytamin
Copy link
Contributor

@nytamin nytamin commented Dec 27, 2023

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>
  1. If <time> is missing: Returns an object where time is populated with a MosTime with value Date.now()
  2. If <time> is empty: Throws with Error: MosTime: Invalid input: \"[object Object]\"
  3. If <time> is badly formatted: Throws with Error: MosTime: Invalid timestamp: \"BAD DATA\"
  4. If <opTime> is missing: Returns an object where time is populated with a MosTime with value Date.now()
  5. If <opTime> is empty: Throws with Error: MosTime: Invalid input: \"[object Object]\"
  6. 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:

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

If in non-strict mode:

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

Testing Instructions

Basic regression testing should be enough.

Other Information

Status

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

Now, mosTime will throw on all invalid input formats
Introducing createRequired and createOptional, used to better handle missing data (filling in fallback data in non-strict mode)
@codecov-commenter
Copy link

codecov-commenter commented Dec 27, 2023

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 ⚠️
...ages/helper/src/mosModel/profile2/xmlConversion.ts 38.09% 13 Missing ⚠️
packages/helper/src/mosModel/lib.ts 10.00% 8 Missing and 1 partial ⚠️
packages/connector/src/MosDevice.ts 91.30% 8 Missing ⚠️
packages/helper/src/utils/Errors.ts 14.28% 5 Missing and 1 partial ⚠️
packages/helper/src/mosModel/parseMosTypes.ts 85.18% 4 Missing ⚠️
packages/model/src/mosTypes/mosString128.ts 33.33% 4 Missing ⚠️
packages/model/src/mosTypes/mosTime.ts 80.95% 4 Missing ⚠️
...ages/helper/src/mosModel/profile1/xmlConversion.ts 40.00% 3 Missing ⚠️
packages/model/src/mosTypes/mosDuration.ts 25.00% 3 Missing ⚠️
... and 4 more
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

If a reply is unparseable in strict mode, a MosReplyError will now be thrown, which can expose a non-strict message (if possible)
@nytamin nytamin marked this pull request as ready for review January 10, 2024 19:13
@jstarpl jstarpl self-assigned this Jan 11, 2024
@jstarpl
Copy link
Member

jstarpl commented Jan 11, 2024

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 jstarpl changed the title Fix: Support non spec data better fix: Support non-spec MOS data better Jan 16, 2024
@jstarpl
Copy link
Member

jstarpl commented Jan 16, 2024

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

@jstarpl jstarpl merged commit 0368fe8 into master Jan 16, 2024
16 checks passed
@jstarpl jstarpl deleted the fix/non-spec-data branch January 16, 2024 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants