bug: `types` in grammar.ts should require at least one `define...`

[dblclik]

Currently the grammar for types (ref) has the define group as optional but this should require at least one. Without this, the playground goes into a strange state and I don't believe there is a case for having an orphaned type that has no relations available.

Proposed resolution (if seen as an issue): The line currently reads

types           -> (_newline):* (type relations (define_or | define_and | define_but_not | define_base):*):*

but should instead read

types           -> (_newline):* (type relations (define_or | define_and | define_but_not | define_base):+):*

[dblclik]

I have a branch ready, but when trying to push I received an error about lacking correct access rights -- I'm guessing this is due to me trying to push a new branch up rather than having had one already created for me. If someone is able to either allow me to open a new branch or creates a branch from this issue I'll get the PR opened 🙌

[rhamzeh]

Thanks for raising the issue @dblclik!

I will split the answer into two parts: i- PR contributions ii- This issue (empty types)

i- For Pull Request/Code Contributions, apologies that we don't make this clearer, we'll improve our contribution guide to include that.

We generally follow the standard GitHub contribution workflow Contributing to Projects. What that entails is:

• Forking the project to create your own personal copy
• Cloning your own copy
• Pushing any modifications there
• Submitting a Pull Request into the original repository

ii- For the scope of this issue, it is actually valid to have an empty type, and the OpenFGA API currently accepts it.

Types with no relations can be helpful for future-proofing in some cases. (e.g. using a user type with no relations). It can be helpful to prefix all your users with a type instead of using floating user_ids in case in the future you'd like to establish relations between those users.

However, in the DSL having a floating type user alone might easily be a typo, so we are thinking of allowing you to specifically say that yes, you are aware that that type should exist. Something like:

type user
  relations
    none

What do you think of that?

[dblclik]

Thank you very much, I will absolutely use the fork/clone/PR approach for contributing 🙌

I do think that it is worthwhile to have some indication that it is an intentionally empty type versus something done in error. If we go with none, would we also need to make changes to how the server receives that JSON, or would you all want to have the JSON look like an empty type and have the DSL to API transformer skip anything that is none and hop to the next type? (I think the second scenario is much more preferable, but wanted to confirm)

[rhamzeh]

would we also need to make changes to how the server receives that JSON

The OpenFGA API should accept an empty type in the JSON syntax already. So no changes need to be made there. The user type would need to exist in the model on the API if you plan on adding tuples such as:

- user: user:anne
  relation: writer
  object: document:roadmap 

The DSL should probably error in this case:

type user
type document
  relations
     define writer as self

type user
  relations
    none

type document
  relations
    define writer as self

It would send the second case to the OpenFGA API as:

{
  "type_definitions": [
    {
      "type": "user",
      "relations": {}
    },
    {
      "type": "document",
      "relations": {
        "writer": {
          "this": {}
        }
      }
    }
  ]
}

What do you think about that?

[matthewpereira]

Note: I've added an issue (more of a placeholder really!) here to help us remember to rectify the fact that this forking workflow isn't explicitly spelled out in our contributor docs.

[rhamzeh]

Hey @dblclik - Just to double back on this, we have decided to allow no relations to be defined. That would be helpful in the case of "user" where not everyone might have relations to define on that type.

So models like:

type user

and

{
  "type_definitions": [
    {
      "type": "user",
      "relations": {}
    }
}

are now allowed.

This has been incorporated into the transformer - let us know what you think!