fix: observation metadata types (#248)

* WIP: 7b5de70 add failing test

* fix: observation metadata types and encoding defaults

* fix: don't generate partial and json methods (unused)

* remove comment

Co-authored-by: Evan Hahn <[email protected]>

* fix undefined check

Co-authored-by: Evan Hahn <[email protected]>

---------

Co-authored-by: Evan Hahn <[email protected]>

buf.gen.yaml

@@ -7,6 +7,8 @@ plugins:
     opt:
       - esModuleInterop=true
       - importSuffix=.js
+      - outputJsonMethods=false
+      - outputPartialMethods=false
       - env=node
       - snakeToCamel=false
       - useDate=string

proto/observation/v1.proto

@@ -38,7 +38,7 @@ message Observation_1 {
 
   // METADATA
   message Metadata {
-    bool manualLocation = 1;
+    optional bool manualLocation = 1;
 
     message Position {
       google.protobuf.Timestamp timestamp = 1;
@@ -57,10 +57,10 @@ message Observation_1 {
     }
 
     message PositionProvider {
-      bool gpsAvailable = 1;
-      bool passiveAvailable = 2;
+      optional bool gpsAvailable = 1;
+      optional bool passiveAvailable = 2;
       bool locationServicesEnabled = 3;
-      bool networkAvailable = 4;
+      optional bool networkAvailable = 4;
     }
 
     optional Position position = 3;

schema/observation/v1.json

@@ -7,6 +7,7 @@
     "position": {
       "description": "Position details",
       "type": "object",
+      "required": ["timestamp", "coords"],
       "properties": {
         "timestamp": {
           "description": "Timestamp of when the current position was obtained",
@@ -162,6 +163,7 @@
         "positionProvider": {
           "description": "Details of the location providers that were available on the device when the observation was recorded",
           "type": "object",
+          "required": ["locationServicesEnabled"],
           "properties": {
             "gpsAvailable": {
               "description": "Whether the user has enabled GPS for device location (this is not the same as whether location is turned on or off, this is a device setting whether to use just wifi and bluetooth or use GPS for location)",

src/lib/decode-conversions.ts

@@ -22,10 +22,15 @@ import {
 } from '../types.js'
 import { ExhaustivenessError, VersionIdObject, getVersionId } from './utils.js'
 import type { Observation, Track } from '../index.js'
-import type { Observation_1_Attachment } from '../proto/observation/v1.js'
+import type {
+  Observation_1_Attachment,
+  Observation_1_Metadata,
+  Observation_1_Metadata_Position,
+} from '../proto/observation/v1.js'
 import type { Track_1_Position } from '../proto/track/v1.js'
 import { ProjectSettings_1_ConfigMetadata } from '../proto/projectSettings/v1.js'
 import { ProjectSettings } from '../schema/projectSettings.js'
+import type { Position } from '../schema/observation.js'
 
 /** Function type for converting a protobuf type of any version for a particular
  * schema name, and returning the most recent JSONSchema type */
@@ -76,7 +81,7 @@ export const convertObservation: ConvertFunction<'observation'> = (
   message,
   versionObj
 ) => {
-  const { common, schemaVersion, ...rest } = message
+  const { common, metadata, schemaVersion, ...rest } = message
   const jsonSchemaCommon = convertCommon(common, versionObj)
   let presetRef
 
@@ -95,7 +100,7 @@ export const convertObservation: ConvertFunction<'observation'> = (
     ...rest,
     attachments: message.attachments.map(convertAttachment),
     tags: convertTags(message.tags),
-    metadata: message.metadata,
+    metadata: metadata ? removeInvalidPositionMetadata(metadata) : {},
     presetRef,
   }
   return obs
@@ -453,3 +458,32 @@ function convertTrackPosition(
     timestamp: position.timestamp,
   }
 }
+
+/**
+ * Because of the way protobuf works, it's possible that a protobuf message is
+ * missing required fields. In this case `timestamp` and the `latitude` and
+ * `longitude` fields on `coords`. We shouldn't have any observations with these
+ * fields missing, but if we do, rather than throwing (and not indexing the
+ * observation at all), we remove the position metadata, since it is not useful
+ * without this metadata.
+ */
+function removeInvalidPositionMetadata(
+  metadata: Observation_1_Metadata
+): Observation['metadata'] {
+  const { position, lastSavedPosition, ...rest } = metadata
+  return {
+    ...rest,
+    position: position && removeInvalidPosition(position),
+    lastSavedPosition:
+      lastSavedPosition && removeInvalidPosition(lastSavedPosition),
+  }
+}
+
+function removeInvalidPosition(
+  position: Observation_1_Metadata_Position
+): Position | undefined {
+  if (position.coords === undefined || position.timestamp === undefined) {
+    return undefined
+  }
+  return position as Position
+}

src/lib/encode-conversions.ts

@@ -95,9 +95,6 @@ export const convertObservation: ConvertFunction<'observation'> = (
   mapeoDoc
 ) => {
   const attachments = mapeoDoc.attachments.map(convertAttachment)
-  const metadata: Observation_1_Metadata | undefined = mapeoDoc.metadata && {
-    ...Observation_1_Metadata.fromPartial(mapeoDoc.metadata),
-  }
   let presetRef
   if (mapeoDoc.presetRef) {
     presetRef = {
@@ -111,7 +108,6 @@ export const convertObservation: ConvertFunction<'observation'> = (
     ...mapeoDoc,
     attachments,
     tags: convertTags(mapeoDoc.tags),
-    metadata,
     presetRef,
   }
 }

test/fixtures/bad-docs.js

@@ -39,7 +39,6 @@ export const badDocs = [
           index: 123,
         }),
       ],
-      refs: [],
       attachments: [],
       tags: {},
       metadata: {},

test/index.test.js

@@ -19,6 +19,7 @@ import {
   goodDocsCompleted,
   badDocs,
 } from './fixtures/index.js'
+import { cachedValues } from './fixtures/cached.js'
 
 test('Bad docs throw when encoding', () => {
   for (const { text, doc } of badDocs) {
@@ -225,6 +226,75 @@ test(`test encoding of wrongly formatted header`, async () => {
   })
 })
 
+/** @type {import('../dist/index.js').Observation} */
+const minimalObservation = {
+  docId: cachedValues.docId,
+  versionId: cachedValues.versionId,
+  originalVersionId: cachedValues.originalVersionId,
+  schemaName: 'observation',
+  createdAt: cachedValues.createdAt,
+  updatedAt: cachedValues.updatedAt,
+  links: [],
+  lat: 24.0424,
+  lon: 21.0214,
+  attachments: [],
+  tags: {},
+  metadata: {},
+  deleted: false,
+}
+
+test(`encoding observation with missing position metadata`, async () => {
+  /** @type {import('../dist/index.js').Observation} */
+  const doc = {
+    ...minimalObservation,
+    metadata: {
+      position: /** @type {any} */ ({ coords: {} }),
+    },
+  }
+  const buf = encode(doc)
+  const decodedDoc = decode(buf, parseVersionId(doc.versionId))
+  assert.equal(decodedDoc.schemaName, 'observation')
+  // a previous bug meant that protobuf defaults of 0 were being set for lat/lon
+  assert.equal(
+    typeof decodedDoc.metadata?.position?.coords?.longitude,
+    'undefined'
+  )
+  assert.equal(
+    typeof decodedDoc.metadata?.position?.coords?.latitude,
+    'undefined'
+  )
+})
+
+test(`decoding observation with missing position provider props`, async () => {
+  /** @type {import('../dist/index.js').Observation} */
+  const doc = {
+    ...minimalObservation,
+    metadata: {
+      positionProvider: {
+        locationServicesEnabled: true,
+      },
+    },
+  }
+  const buf = encode(doc)
+  const decodedDoc = decode(buf, parseVersionId(doc.versionId))
+  assert.equal(decodedDoc.schemaName, 'observation')
+  assert.equal(
+    typeof decodedDoc.metadata?.positionProvider?.gpsAvailable,
+    'undefined',
+    'optional gpsAvailable prop should be undefined'
+  )
+  assert.equal(
+    typeof decodedDoc.metadata?.positionProvider?.networkAvailable,
+    'undefined',
+    'optional networkAvailable prop should be undefined'
+  )
+  assert.equal(
+    typeof decodedDoc.metadata?.positionProvider?.passiveAvailable,
+    'undefined',
+    'optional passiveAvailable prop should be undefined'
+  )
+})
+
 /**
  * Remove undefined properties (deeply) from an object, by round-tripping to
  * JSON. Also handles Buffers via JSON.parse reviver