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

[Code health] Introduce protos as structure for checking data in Firestore at compile-time #2049

Closed
gino-m opened this issue Nov 7, 2023 · 5 comments
Assignees
Labels
type: code health Improvements to readability or robustness of codebase

Comments

@gino-m
Copy link
Collaborator

gino-m commented Nov 7, 2023

See also google/ground-platform#1277

@os-micmec FYI tracking bug also here

Proposed micro-design (read):

  1. Use DocumentSnapshot.getData() to fetch data as a Map<String!, Any!>
  2. Tweak data in Map when needed. We should try to minimize special logic in this layer.
  3. Recursively wrap Map in Gson JsonElement and related classes.
  4. Use Gson to populate classes generated with json-kotlin-schema-codegen

We would do the reverse for writing:

  1. Convert object to JsonElement using Gson
  2. Convert JsonElement to Map
  3. Tweak Map as needed.
  4. Pass Map to update methods in Firestore libs.

We can preserve our current representation of nested arrays, using indexed maps instead.

Alternatives considered:

  • kpropmap libraries requires conversion to its own PropertyMap interface. It's also not clear how we'd implement polymorphism or custom (de)serializers.
  • kotlinx.serialization Properties requires @Serialization annotations on data classes (issue). Our data classes are generated, so we can't do that. Also not correct - you can set custom annotations
  • Gson and kotlinx.serialization Json require conversion to/from JSON strings, which can be slow. <-- not true - we can also recursively wrap our object graph to JsonObject and JsonElement as per this example
  • DocumentSnapshot.toObject doesn't handle polymorphism, and doesn't offer much flexibility in customizing mappings.

@sufyanAbbasi FYI

Comments which follow are from early research and are out of date.

@gino-m gino-m added type: code health Improvements to readability or robustness of codebase priority: p2 labels Nov 7, 2023
@gino-m gino-m self-assigned this Nov 7, 2023
@github-project-automation github-project-automation bot moved this to Todo in Ground Nov 7, 2023
@gino-m
Copy link
Collaborator Author

gino-m commented Dec 12, 2023

@gino-m gino-m added this to Ground Dec 19, 2023
@gino-m gino-m added this to the EUDR MVP milestone Dec 19, 2023
@gino-m
Copy link
Collaborator Author

gino-m commented Jan 3, 2024

There's currently no easy way to do this. We could use something like quicktype to generate model code from JSON Schema, but that would require data to be converted to JSON and then parse (wasteful), and it's not as customizable so classes like GeoPoint won't be accessible. Firestore doesn't support this by design (filed this FR), so we'd need to roll our own. Dart isn't supported on Cloud Functions (see FR), so even if we can share model and logic between Angular and Android, we'd still need another impl there.

@gino-m gino-m removed their assignment Jan 3, 2024
@gino-m gino-m removed this from the 2) EUDR MVP milestone Jan 3, 2024
@gino-m gino-m added this to the 3) Launch milestone Jan 10, 2024
@gino-m
Copy link
Collaborator Author

gino-m commented Jan 11, 2024

@gino-m
Copy link
Collaborator Author

gino-m commented Jan 15, 2024

A few more notes:

@gino-m gino-m changed the title Define shared model across Android, web, and Cloud Functions [Code health] Shared compile-time checks of Firestore structure across Android, web, and Cloud Functions Jan 24, 2024
@gino-m gino-m changed the title [Code health] Shared compile-time checks of Firestore structure across Android, web, and Cloud Functions [Code health] Enforce structure of data in Firestore at compile-time Jan 24, 2024
@jcqli jcqli moved this to Todo in Ground Feb 16, 2024
@gino-m
Copy link
Collaborator Author

gino-m commented Feb 27, 2024

@jcqli Moving out of this iteration for now since we punted on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: code health Improvements to readability or robustness of codebase
Projects
Status: Done
Status: Todo
Development

No branches or pull requests

2 participants