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

Converted JSON schemas use "EntityOrCommon" #1161

Open
2 tasks
khieta opened this issue Aug 30, 2024 · 3 comments
Open
2 tasks

Converted JSON schemas use "EntityOrCommon" #1161

khieta opened this issue Aug 30, 2024 · 3 comments
Labels
papercut Small annoyances in the Cedar SDK. Lower priority fixes than bugs. Smaller than a fature request

Comments

@khieta
Copy link
Contributor

khieta commented Aug 30, 2024

Describe the improvement you'd like to request

Due to recent changes introduced on main (which are currently unreleased, but will be released as part of 4.0), converting a Cedar schema to JSON will now introduce "EntityOrCommon" types. These seems like an internal detail that is not useful to expose to users. Is there a way to hide this detail when returning JSON data from SchemaFragment::to_json_string?

As an example, here's a snippet from the translation of the TinyTodo schema:

{
  "": {
    "commonTypes": {
      "Task": {
        "type": "Record",
        "attributes": {
          "id": {
            "type": "EntityOrCommon",
            "name": "Long"
          },
          "name": {
            "type": "EntityOrCommon",
            "name": "String"
          },
          "state": {
            "type": "EntityOrCommon",
            "name": "String"
          }
        }
      },
      "Tasks": {
        "type": "Set",
        "element": {
          "type": "EntityOrCommon",
          "name": "Task"
        }
      }
    },
    "entityTypes": {
      "Team": {
        "memberOfTypes": [
          "Team",
          "Application"
        ]
      },
      "User": {
        "memberOfTypes": [
          "Team",
          "Application"
        ],
        "shape": {
          "type": "Record",
          "attributes": {
            "joblevel": {
              "type": "EntityOrCommon",
              "name": "Long"
            },
            "location": {
              "type": "EntityOrCommon",
              "name": "String"
            }
          }
        }
      },
...

The more natural way for a user to write this would be:

{
  "": {
    "commonTypes": {
      "Task": {
        "type": "Record",
        "attributes": {
          "id": {
            "type": "Long"
          },
          "name": {
            "type": "String"
          },
          "state": {
            "type": "String"
          }
        }
      },
      "Tasks": {
        "type": "Set",
        "element": {
          "type": "Task"
        }
      }
    },
    "entityTypes": {
      "Team": {
        "memberOfTypes": [
          "Team",
          "Application"
        ]
      },
      "User": {
        "memberOfTypes": [
          "Team",
          "Application"
        ],
        "shape": {
          "type": "Record",
          "attributes": {
            "joblevel": {
              "type": "Long"
            },
            "location": {
              "type": "String"
            }
          }
        }
      },
...

Describe alternatives you've considered

No response

Additional context

Related: the Display trait for the Cedar schema format is also not as pretty as it could be. See #682.

Is this something that you'd be interested in working on?

  • 👋 I may be able to implement this internal improvement
  • ⚠️ This feature might incur a breaking change
@khieta khieta added pending-triage The cedar maintainers haven't looked at this yet. Automicaly added to all new issues. internal-improvement Refactoring, minor performance improvement, or other changes that Cedar users may never notice labels Aug 30, 2024
@khieta khieta mentioned this issue Aug 30, 2024
3 tasks
@cdisselkoen
Copy link
Contributor

cdisselkoen commented Aug 30, 2024

some context: in some ways, EntityOrCommon is more correct than the previous status quo. The Cedar syntax Long will actually refer to an entity type Long if it exists, in preference to the builtin type Long, as specified by RFC 24. (This is why RFC 24 introduced the __cedar::Long notation in the first place.) This remains true after #1150, which only introduced restrictions on the names of common types, not entity types. So, translating Cedar syntax Long to JSON EntityOrCommon is an accurate reflection of the semantics of the Cedar syntax.

To resolve this issue, we would have to actually resolve the typenames in the Cedar syntax (e.g., to know that Long does indeed refer to the builtin type Long) in order to write a more accurate JSON-syntax typename.

@shaobo-he-aws shaobo-he-aws added backlog and removed pending-triage The cedar maintainers haven't looked at this yet. Automicaly added to all new issues. labels Sep 10, 2024
@john-h-kastner-aws john-h-kastner-aws added papercut Small annoyances in the Cedar SDK. Lower priority fixes than bugs. Smaller than a fature request and removed internal-improvement Refactoring, minor performance improvement, or other changes that Cedar users may never notice backlog labels Oct 1, 2024
@kindjar
Copy link

kindjar commented Nov 13, 2024

I attempted to add a common type with type String = __cedar::String; but cedar validate returns an error:

error parsing schema: this uses a reserved schema keyword: `String`
 2 │     type String = __cedar::String;
  help: Keywords such as `entity`, `extension`, `set` and `record` cannot be used as common type names

On the other hand, I am able to create an Entity with the name String—that just earns me a warning:

  ⚠ The name `String` shadows a builtin Cedar name. You'll have to refer to the builtin as `__cedar::String`.

My own personal issue with using EntityOrCommon for built-in primitive types is that Amazon Verified Permissions does not work with JSON-formatted cedar schema like this. It doesn't seem to understand EntityOrCommon at all, so I can't use cedar translate-schema to convert my nice schema into JSON form to hand over to Amazon Verified Permissions without post-processing to remove all the EntityOrCommon references. I guess that's technically an AWS problem, not a Cedar problem, but... it's extra friction in the ecosystem.

@john-h-kastner-aws
Copy link
Contributor

If you want to use your schema with AVP, it should work if you convert the schema with the latest 3.x CLI release. This release won't insert EntityOrCommon, so it should work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
papercut Small annoyances in the Cedar SDK. Lower priority fixes than bugs. Smaller than a fature request
Projects
None yet
Development

No branches or pull requests

5 participants