So there were a few edge-cases around the way we type, encode and decode observation position metadata.
We were using fromPartial for encoding metadata, which assigns defaults for missing props, which meant that decoding would pass type checks, but defaults would be assigned to the missing props, in this case longitude and latitude, which is a "null island" bug.
This PR makes a few changes:
manualLocation is now optional in protobuf - the distinction between when this is undefined (when there is no location for the observation) and when it is boolean (when there is a location) is important.
gpsAvailable, networkAvailable, passiveAvailable props of metadata.positionProvider are now optional on the protobuf, to distinguish between cases when this data is not available on the client (iOS) and when it is.
locationServicesEnabled of metadata.positionProvider is now required on the JSONSchema - if we do store position provider metadata, then we have this data - this corresponds to the Expo types.
timestamp and coords are now required fields of position and lastSavedPosition in the JSON schema. These metadata options don't make sense without this data.
If a protobuf is missing timestamp or coords (possible, because of the way protobuf make everything optional), then we strip the position prop from metadata (rather than the alternative of assigning defaults or throwing)
This PR also removes usage of fromPartial, and stops generating it since it's no longer needed. Using this is "dangerous" because it assigns default values deeply to a message, which can lead to unexpected results - data that is type safe, but might have required fields set to default values.
Impact on frontend:
This is breaking, because it changes types to make some fields required. However fields that are newly set to required are also required / always set on the Expo Location LocationObject and LocationProviderStatus. Therefore in frontend code this should not break things, because these values are being set from these objects from Expo.
These changes are not really specific to Expo - it would not make sense to add this metadata without these required fields being available, even if we move away from Expo for reading location.
Why is this important?
In the future, this (position) metadata is potentially important for "proofs" of observations. Without these changes it is possible that default values could be unexpectedly set, e.g. values could be false when they were actually unknown (undefined).
boy protobuf and typescript are tricky beasts...
So there were a few edge-cases around the way we type, encode and decode observation position metadata.
We were using
fromPartialfor encoding metadata, which assigns defaults for missing props, which meant that decoding would pass type checks, but defaults would be assigned to the missing props, in this caselongitudeandlatitude, which is a "null island" bug.This PR makes a few changes:
manualLocationis now optional in protobuf - the distinction between when this isundefined(when there is no location for the observation) and when it is boolean (when there is a location) is important.gpsAvailable,networkAvailable,passiveAvailableprops ofmetadata.positionProviderare now optional on the protobuf, to distinguish between cases when this data is not available on the client (iOS) and when it is.locationServicesEnabledofmetadata.positionProvideris now required on the JSONSchema - if we do store position provider metadata, then we have this data - this corresponds to the Expo types.timestampandcoordsare now required fields ofpositionandlastSavedPositionin the JSON schema. These metadata options don't make sense without this data.timestamporcoords(possible, because of the way protobuf make everything optional), then we strip the position prop from metadata (rather than the alternative of assigning defaults or throwing)This PR also removes usage of
fromPartial, and stops generating it since it's no longer needed. Using this is "dangerous" because it assigns default values deeply to a message, which can lead to unexpected results - data that is type safe, but might have required fields set to default values.Impact on frontend:
This is breaking, because it changes types to make some fields required. However fields that are newly set to required are also required / always set on the Expo Location
LocationObjectandLocationProviderStatus. Therefore in frontend code this should not break things, because these values are being set from these objects from Expo.These changes are not really specific to Expo - it would not make sense to add this metadata without these required fields being available, even if we move away from Expo for reading location.
Why is this important?
In the future, this (position) metadata is potentially important for "proofs" of observations. Without these changes it is possible that default values could be unexpectedly set, e.g. values could be
falsewhen they were actually unknown (undefined).