[gino-m]

Collect One review series: P3 Protos: data, library, userdata (5/60)

Tracking issue: #2629 · Stacked on: #2630 (C01) · Reviewer: @rfontanarosa
FYI: @gino-m @shobhitagarwal1612

Context

Collect One is the Kotlin Multiplatform successor to Ground: a single shared codebase for the web console, the mobile app (Android, iOS, browser), and a Spanner-backed user-data service. Forms use ProtoForms, a lossless proto3 encoding of ODK XForms.

This PR is one slice of the frozen wip/baobab tree (collect-one-review-base), landing through 60 stacked PRs into c1 (see #2629). It only adds the files listed below, and it's stacked on C01 (#2630), so the diff shows only this slice.

The proto PRs (P1–P3) import each other, e.g. survey_def.proto imports forms/form_def.proto and data/audit_record.proto. Nothing compiles them until shared/core lands in K1, after all three merge, so please review the schemas as a contract. CI only checks buf format here.

What's in this PR

• data/ (groundplatform.v2.data):
  • entity_record.proto: versioned geospatial/tabular EntityRecord (map features / data table rows), with PlotLock, QualityRollup, and EntityConflict for offline branch/trunk conflicts (ODK Entities model).
  • submission_record.proto: immutable SubmissionRecord (wraps a RecordInstance, review status, and denormalized field entries and geometry for listing).
  • audit_record.proto: append-only AuditRecord, FieldDelta, and AuditInfo (who and when, reused by survey and library messages).
  • impact_event.proto / impact_aggregate.proto: "data was used" signals and the privacy-safe aggregates produced from them.
  • data_service.proto: DataService: get, list and cluster entities (bounding box), get and list submissions, batched MutateData, plot locks, media upload and download URLs, impact events and aggregates, audit records, places, and mutation logs (ListMutationLogs/GetAppliedMutations back the sync engine).
• library/ (groundplatform.v2.library): shared concept library (ConceptDef, CodeList, PrivacyClass), purpose packs, form templates, organization library settings, bundles, and LibraryService.
• userdata/ (groundplatform.v2.userdata): ClientSyncState (the client's persisted base records and sync tokens) and UserDataSeed (local and dev seed format).

Where it fits

DataService and the record types are the wire contract between the offline-first clients and the user-data service. The client sync engine (A5) records outbox mutations against them and rebases on ListMutationLogs. The server (S1–S2) persists them in Spanner.

Not in this PR

• Forms and RecordInstance → P1. Survey, ACLs and organizations → P2.
• Narrative docs for records, sync, and the Spanner schema → D3/D4.
• Purpose-pack content (.textproto data) → X1.

Suggested reading order

1. audit_record.proto, entity_record.proto, submission_record.proto
2. data_service.proto: entity/submission reads → MutateData → mutation logs
3. userdata/client_sync_state.proto
4. library/*, impact_*

Focus areas

• Sync contract: mutation idempotency keys, record versions, sync tokens, and mutation log paging. Does it support incremental, resumable sync on flaky connections, as Ground Android needs today?
• Conflict representation in EntityRecord (EntityConflict, plot locks).
• Privacy classes on library concepts and how impact aggregates avoid leaking individual records.

Safe to skim

• library_bundle.proto, library_settings.proto, user_data_seed.proto (thin containers).

Size

Source	Tests	Data	Files
2,057	0	0	14

Testing

bash scripts/buf-format.sh --check: PASS. CI: check (ktfmt, buf).

[gemini-code-assist]

Code Review

This pull request introduces several Protocol Buffer definitions for the Ground Platform v2, covering data services, entity and submission records, impact tracking, library services, and client synchronization state. The feedback focuses on improving API consistency, precision, and adherence to the Google Protocol Buffers Style Guide. Key recommendations include reusing the GeoBoundingBox message in PlaceRecord, changing perimeter_meters to a double for better precision, optimizing ARGB hex color fields to uint32 instead of int64, removing the redundant occurred_at_iso timestamp string, and prefixing enum values with their respective enum names to prevent naming conflicts.

[gemini-code-assist]

Instead of flattening the bounding box coordinates into four separate optional double fields (bbox_min_lng, bbox_min_lat, etc.), you should reuse the GeoBoundingBox message defined earlier in this file (line 215). This improves consistency, reduces duplication, and makes the API cleaner. Since fields 12, 13, and 14 are being removed, they should be marked as reserved to prevent future numbering conflicts.

Suggested change

	optional double bbox_min_lng = 11;
	optional double bbox_min_lat = 12;
	optional double bbox_max_lng = 13;
	optional double bbox_max_lat = 14;
	optional GeoBoundingBox bbox = 11;
	reserved 12, 13, 14;
	reserved "bbox_min_lat", "bbox_max_lng", "bbox_max_lat";

[gino-m]

Good question; checking with the author and will follow up here.

[gemini-code-assist]

Perimeter is a continuous physical measurement, similar to area (area_hectares is a double). Using int32 for perimeter_meters unnecessarily discards precision (e.g., fractional meters) and is inconsistent with the use of double for other spatial measurements. Consider changing it to double.

Suggested change

	int32 perimeter_meters = 23;
	double perimeter_meters = 23;

[gino-m]

Good question; checking with the author and will follow up here.

[gemini-code-assist]

An ARGB color hex value (e.g., 0xFFRRGGBB) fits entirely within 32 bits. Using int64 is inefficient as it allocates twice the necessary memory/payload size. Using uint32 is more appropriate and avoids negative value representation issues in languages with signed types.

Suggested change

	// Default ARGB color hex (`0xFFRRGGBB`) for map rendering.
	uint32 color_hex = 27;

[gino-m]

Good question; checking with the author and will follow up here.

[gemini-code-assist]

An ARGB color hex value (e.g., 0xFFRRGGBB) fits entirely within 32 bits. Using int64 is inefficient as it allocates twice the necessary memory/payload size. Using uint32 is more appropriate and avoids negative value representation issues in languages with signed types.

Suggested change

	int64 color_hex = 18;
	uint32 color_hex = 18;

[gino-m]

Good question; checking with the author and will follow up here.

[gemini-code-assist]

The field occurred_at_iso is redundant because occurred_at (of type google.protobuf.Timestamp) already represents the exact time. Storing both can lead to data inconsistency if they diverge, and increases payload size. It is highly recommended to let the client format the Timestamp to ISO-8601 or any other localized format as needed.

Suggested change

	// Optional ISO-8601 string representation of `occurred_at`.
	string occurred_at_iso = 12;
	reserved 12;
	reserved "occurred_at_iso";

[gino-m]

Good question; checking with the author and will follow up here.

[gemini-code-assist]

According to the Google Protocol Buffers Style Guide, enum values should be prefixed with the enum name (e.g., AUDIT_ACTION_) to avoid naming conflicts in languages that do not scope enums (like C++ or C). While AUDIT_ACTION_UNSPECIFIED follows this convention, the other values (CREATE, UPDATE, etc.) do not. Prefixing them ensures consistency and safety.

Suggested change

	enum AuditAction {
	AUDIT_ACTION_UNSPECIFIED = 0;
	CREATE = 1;
	UPDATE = 2;
	SOFT_DELETE = 3;
	RESTORE = 4;
	PUBLISH = 5;
	}
	enum AuditAction {
	AUDIT_ACTION_UNSPECIFIED = 0;
	AUDIT_ACTION_CREATE = 1;
	AUDIT_ACTION_UPDATE = 2;
	AUDIT_ACTION_SOFT_DELETE = 3;
	AUDIT_ACTION_RESTORE = 4;
	AUDIT_ACTION_PUBLISH = 5;
	}

References

1. Google Protocol Buffers Style Guide recommends prefixing enum values with the enum name to avoid naming conflicts in languages that do not scope enums. ^(link)

[gino-m]

Good question; checking with the author and will follow up here.

[gemini-code-assist]

According to the Google Protocol Buffers Style Guide, enum values should be prefixed with the enum name (e.g., AGGREGATION_) to avoid naming conflicts in languages that do not scope enums. Prefixing all values ensures consistency and prevents potential name clashes with other enums (e.g., NONE or SUM are very common names).

enum Aggregation {
  AGGREGATION_UNSPECIFIED = 0;
  AGGREGATION_COUNT_DISTINCT_FEATURES = 1;
  AGGREGATION_COUNT_BY_CODE = 2;
  AGGREGATION_SUM = 3;
  AGGREGATION_MEAN = 4;
  AGGREGATION_SHARE_BY_CODE = 5;
  AGGREGATION_NONE = 6;
}

References

1. Google Protocol Buffers Style Guide recommends prefixing enum values with the enum name to avoid naming conflicts in languages that do not scope enums. ^(link)

[gino-m]

Good question; checking with the author and will follow up here.

[gino-m]

Heads-up: encrypted form submissions are being removed from Collect One (no code used them, and the server needs to read submissions). The change for this slice is in #2636, which merges into this PR's branch. Please review it alongside this PR.