fix(robot-server): Fix error parsing persisted loadModule commands

[SyntaxColoring]

Overview

Fixes RSS-471.

Test Plan

The integration tests added in #14508 will cover this.

Details

When you load a Magnetic Block, that's represented by a Protocol Engine loadModule command. Unlike other loadModule commands, it will not have a result.serialNumber. This is all good and correct so far.

That serialNumber field is defined like this:

serialNumber: Optional[str] = Field(
    ...,  # This is a literal ellipsis, not me summarizing.
    description="blah blah"
)

This means, when parsing this model from persistent storage, serialNumber must be present, either as a string or as an explicit Python None / JSON null. It cannot be omitted; it will not default to None/null.

The bug is that this does not match how these models are saved to persistent storage. There, the field is omitted. My fix is to add a default of None, so they match.

We're only seeing this now because, as of #14355, we're parsing models that were saved under a different set of Pydantic options. Formerly, we were doing .dict() (for analyses, runs, and commands). Now, we're doing .json(by_alias=True, exclude_none=True) (for analyses, runs, and commands). The "bad" part of that is exclude_none=True—see below.

Review requests

Is this the only place we've introduced this bug?

Theoretically, this bug can happen in any other model that's declaring a field like field: Optional[T] = Field(default=...).

I've run a global text search on the monorepo with this Comby rule. Basically, look for any Optional[X] = Field(Y), where Y contains ....

comby 'Optional[...] = :[~(pydantic\.)?]Field(:[params])' \
  '' \
  -rule 'where match :[params] { | ":[~.*\.\.\..*]" -> true}' \
  -match-only \
  .py

Although there are many hits, all except for this one seem to be false positives or models that aren't stored in the database.

But it's difficult to be confident that this search is exhaustive.

Is exclude_none=True a correct way to serialize these objects in the first place?

This seems like it will take a bit of attention to untangle, but here is a spike ticket for us to figure that out: RSS-473. If you have any thoughts, drop them there.

Risk assessment

I don't think this change can make anything worse, but the underlying hazard still remains.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9dc8f2d) 67.74% compared to head (db36a78) 67.76%.
Report is 1 commits behind head on chore_release-7.2.0.

Additional details and impacted files

@@                   Coverage Diff                   @@
##           chore_release-7.2.0   #14509      +/-   ##
=======================================================
+ Coverage                67.74%   67.76%   +0.02%     
=======================================================
  Files                     2517     2517              
  Lines                    72068    72119      +51     
  Branches                  9278     9278              
=======================================================
+ Hits                     48823    48873      +50     
- Misses                   21027    21028       +1     
  Partials                  2218     2218              

Flag	Coverage Δ
app	64.55% <ø> (ø)
g-code-testing	92.43% <ø> (ø)
hardware	57.54% <ø> (+0.24%)	⬆️
hardware-testing	∅ <ø> (∅)
protocol-designer	37.93% <ø> (ø)
shared-data	75.24% <ø> (-0.02%)	⬇️
step-generation	86.90% <ø> (ø)
system-server	96.04% <ø> (ø)
update-server	50.56% <ø> (ø)
usb-bridge	76.94% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
.../opentrons/protocol_engine/commands/load_module.py	100.00% <ø> (ø)

... and 8 files with indirect coverage changes

[sfoster1]

Yeah that looks like a good fix.

I suspect that the safest thing here is to always always always load input from storage, any storage, in a way that treats null and not present as exactly equivalent.