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: observation metadata types #248

Merged
merged 5 commits into from
Sep 4, 2024
Merged

Conversation

gmaclennan
Copy link
Member

@gmaclennan gmaclennan commented Sep 4, 2024

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 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).

@gmaclennan gmaclennan self-assigned this Sep 4, 2024
src/lib/decode-conversions.ts Outdated Show resolved Hide resolved
src/lib/decode-conversions.ts Outdated Show resolved Hide resolved
buf.gen.yaml Show resolved Hide resolved
gmaclennan and others added 2 commits September 4, 2024 16:02
Co-authored-by: Evan Hahn <[email protected]>
Co-authored-by: Evan Hahn <[email protected]>
@gmaclennan gmaclennan merged commit 10999ac into main Sep 4, 2024
6 checks passed
@gmaclennan gmaclennan deleted the fix/observation-metadata branch September 4, 2024 15:14
gmaclennan added a commit that referenced this pull request Sep 9, 2024
* main:
  test: test validating a schema name that could "trick" JavaScript (#250)
  test: properly test more `validate` errors (#249)
  fix: observation metadata types (#248)
  chore: remove `any`s from decode helper function (#247)
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.

2 participants