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

typed references and subcollections #25

Open
ribrdb opened this issue Aug 30, 2018 · 8 comments
Open

typed references and subcollections #25

ribrdb opened this issue Aug 30, 2018 · 8 comments

Comments

@ribrdb
Copy link
Contributor

ribrdb commented Aug 30, 2018

I've started working on a protobuf code generator to create typescript models. One feature I need is to be able to specify the type of a reference or subcollection. I'm thinking something like this:

message Foo {...}
message Bar {
  Foo fooref = 1 [(protofire.options).reference = true];
  map<string, Foo> foocol = 2 [(protofire.options).subcollection = true];
}

This would generate something like:

interface Bar {
  fooref: Reference<Foo>
  foocol: Collection<Foo>

Would this be something you'd be interested in supporting, or does it make more sense for us to have our own rules generator?

@rockwotj
Copy link
Contributor

rockwotj commented Sep 2, 2018

Sorry can you explain what you mean by the difference between subcollection and reference?

Why is subcollection a map type?

There is always an validation options on a field if you need something application specific.

See: https://github.com/FirebaseExtended/protobuf-rules-gen/blob/master/testdata/test6.proto and https://github.com/FirebaseExtended/protobuf-rules-gen/blob/master/testdata/test6.rules

@ribrdb
Copy link
Contributor Author

ribrdb commented Sep 3, 2018

Sorry, let me try to explain this better.
Reference fields store a Firestore reference, but also hold type information for the code generator.
As far as the rules generator is concerned, this is the same as a string field with the reference option.
Sub-collections don't actually represent a field, they should be ignored by the rules generator. They instruct the code generator to create methods for accessing a sub collection. For example if b is a reference to document Bar/abc:

// Get a typed reference to collection Bar/abc/foocol
const bfoos: Collection<Foo> = b.foocol();
// Access a reference property from a snapshot
const snap = await b.get();
const foo?: Reference<Foo> = s.fooref;

And this is the rules I'd want generated:

function isBarMessage(resource) {
  return resource.keys().hasAll([]) &&
          ((!resource.keys().hasAny(['fooref'])) || (resource.fooref is reference)));
}

I don't think the validation option helps. I believe it lets me add to the generate rules, but I need to modify or omit part of what's generated.

@ribrdb
Copy link
Contributor Author

ribrdb commented Sep 3, 2018

Also I may use a repeated field instead of a map. A map is closer to how Collections work, but since it's going to be handled specially by the code generator I don't think it matters.

@rockwotj
Copy link
Contributor

rockwotj commented Sep 3, 2018

Oh I see, so you want is to be able to ignore fields from rules?

@rockwotj
Copy link
Contributor

rockwotj commented Sep 3, 2018

Does the option extra_properties help here? Or no because you need the proto interface to have the property?

@ribrdb
Copy link
Contributor Author

ribrdb commented Sep 4, 2018

I don't think extra_properties helps, but an option to ignore a field would.

@ribrdb
Copy link
Contributor Author

ribrdb commented Nov 29, 2018

I came up with a workaround here, although it seems pretty hacky. I wrote another code generator that wraps //firebase_rules_generator:firebase_rules_options and //firebase_rules_generator:generator.
It modifies the CodeGeneratorRequest to convert from (protofire.field) options to (google.firebase.rules.firebase_rules_field) options, then runs the RulesGenerator.

I had a couple of ideas for a cleaner way to implement it:

  1. Add some virtual methods to RulesGenerator, that I can override in a subclass. Right now I would need something like ShouldIgnoreField() and IsReferenceField()
  2. Directly support the protofire options in RulesGenerator. Right now it's pretty simple:
syntax = "proto2";
package protofire;

import "google/protobuf/descriptor.proto";

message ProtofireFieldOptions {
  // protobuf-rules-gen should ignore fields with this option
  optional bool collection = 1;
  // This is pretty much the same as (google.firebase.rules.firebase_rules_field).reference_type
  // except that it can apply to a string or a message field.
  optional bool ref = 2;
}

extend google.protobuf.FieldOptions {
  optional ProtofireFieldOptions field = 1044;
}

What do you think? Would you be ok with any of these solutions? Or do you have any other ideas?

@rockwotj
Copy link
Contributor

I think for ref we can extend the current reference type (that is a message) to work for message types as well.

As for collection I think if we add an option to ignore things that's fine - I don't think I want to support these exact options as they are.

I'm also totally fine to accept some PRs to refactor the rules generator to have some virtual methods and be pluggable. I sadly don't have time to work on that at the moment right now 😞

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

No branches or pull requests

2 participants