FirebaseExtended / protobuf-rules-gen

This is an experimental protoc plugin that generates Firebase Rules for Cloud Firestore based on Google's Protocol Buffer format. This allows you to easily validate your data in a platform independent manner.
Apache License 2.0
197 stars 13 forks source link

couple questions #11

Closed ribrdb closed 6 years ago

ribrdb commented 6 years ago
  1. The example shows how to use the protocol compiler to generate language implementations of the types in addition to the rules. Is this an intended use case? Because none of the generated files seem usable with the firestore apis. The typescript looks like it would work if you use numberic_values, but it seems odd that the default is incompatible with how protobufs generate enums.

  2. Is there a reason it's called numberic instead of numeric?

  3. The generated rules code for optional fields seems rather inefficient. Will this really scale to having lots of optional fields? Protobuffers discourage required fields for maintainability, and it seems like every field needs to be optional to use the generated types with update(). Couldn't you say['starredWebsites','phone','email','name']. hasAll(resource.keys()) instead of all this:

    (resource.keys().hasAll([]) && resource.size() == 0) ||
          (resource.keys().hasAll(['name']) && resource.size() == 1) ||
          (resource.keys().hasAll(['email']) && resource.size() == 1) ||
          (resource.keys().hasAll(['phone']) && resource.size() == 1) ||
          (resource.keys().hasAll(['starredWebsites']) && resource.size() == 1) ||
          (resource.keys().hasAll(['email','name']) && resource.size() == 2) ||
          (resource.keys().hasAll(['phone','name']) && resource.size() == 2) ||
          (resource.keys().hasAll(['starredWebsites','name']) && resource.size() == 2) ||
          (resource.keys().hasAll(['phone','email']) && resource.size() == 2) ||
          (resource.keys().hasAll(['starredWebsites','email']) && resource.size() == 2) ||
          (resource.keys().hasAll(['starredWebsites','phone']) && resource.size() == 2) ||
          (resource.keys().hasAll(['phone','email','name']) && resource.size() == 3) ||
          (resource.keys().hasAll(['starredWebsites','email','name']) && resource.size() == 3) ||
          (resource.keys().hasAll(['starredWebsites','phone','name']) && resource.size() == 3) ||
          (resource.keys().hasAll(['starredWebsites','phone','email']) && resource.size() == 3) ||
          (resource.keys().hasAll(['starredWebsites','phone','email','name']) && resource.size() == 4))
rockwotj commented 6 years ago
  1. The ideal use case I would say would be to be able to use with models, but it really seems this is only possible with typescript/javascript. I'll change the default values when I get a chance.

  2. Just me making typos, I'll fix it.

  3. Yes that is a better solution :) There is about to be a new function called .hasAny that is actually going to make this much easier/readable. And yes to get this to work with update() you'll want to only use optional fields.

rockwotj commented 6 years ago

I've opened another issue to track using .hasAny instead of the huge mess of properties that currently exists.

rockwotj commented 6 years ago

I opened a PR to default to numbers instead of strings (which fixes that typo). Issue #12 tracks your third point.

rockwotj commented 6 years ago

Let me know if that helps, thanks for your feedback!

ribrdb commented 6 years ago

Cool, thanks. I've been thinking about writing a proto-generator to generate models for go & javascript. But I got hung up trying to decide what features to implement. Just making some basic classes would be easy but some other ideas I had include:

rockwotj commented 6 years ago

First two sound great. However, arrays are still indexed by Firestore, and I'm hoping the map hack won't be needed in the near future.

ribrdb commented 6 years ago

Oh really? I guess I misinterpreted this in the docs:

Although Cloud Firestore can store arrays, it does not support querying array members

Are you saying the entire array is stored in the index as one value? Is there a reason there's no support for unindexed properties like datastore has?

rockwotj commented 6 years ago

Yes individual array elements are not yet indexed, but the full array is.

And the reason is simplicity and because Firestore allows you to index large values, where in Datastore you could only have 1500 bytes in a single index entry. Firestore will support disabling the built-in indexes soon and will have per field and collection controls so you can turn them on and off. Until then, we decided to turn on all indexes as a default.