schema conversion method(fixed) - #323
Conversation
2227727 to
1c377f3
Compare
| } | ||
| public String toCedarFormat() throws InternalException { | ||
| if (type != JsonOrCedar.Json || schemaJson.isEmpty()) { | ||
| throw new InternalException("Schema is not in JSON format"); |
There was a problem hiding this comment.
@mark-creamer-amazon do we want it to throw an Exception in this case or just return the Schema? For example, if the schema is already in Cedar format and the user calls this method, we can return the schema which is already in the Cedar format.
There was a problem hiding this comment.
I was imagining that we would want to return the schema content we have as a String instead of returning an exception!
You could imagine perhaps that if you had a list of Schema objects of mixed JSON or Cedar types, and you enumerated with toCedarFormat across all, you'd want the Cedar format string regardless of the type
There was a problem hiding this comment.
I tried implementing/writing to return the Schema as a string.
| if (type != JsonOrCedar.Cedar || schemaText.isEmpty()) { | ||
| throw new InternalException("Schema is not in cedar format"); | ||
| } | ||
| return cedarToJsonJni(schemaText.get()); |
There was a problem hiding this comment.
I believe we should return a JsonNode instead of String here because that's how we represent JSON schema in Schema.
Can do something like:
return OBJECT_MAPPER.readTree(cedarToJsonJni(schemaText.get()));
and let it throw JSONProcessingException (which shouldn't occur ideally because we get this JSON converted string from Cedar).
| Schema schema = Schema.parse(JsonOrCedar.Json, schemaJson); | ||
| String cedarSchema = schema.toCedarFormat(); | ||
| assertNotNull(schema); | ||
| assertTrue(cedarSchema.contains("entity User"), "Expected Cedar to contain 'entity User'"); |
There was a problem hiding this comment.
Let's assert the full Cedar schema value here instead of just "entity User"
| Schema schema = Schema.parse(JsonOrCedar.Cedar, cedarSchema); | ||
| String jsonSchema = schema.toJsonFormat(); | ||
| assertNotNull(schema); | ||
| assertTrue(jsonSchema.contains("User"), "Expected Json to contain 'User'"); |
There was a problem hiding this comment.
Add assertion on the entire jsonSchema value
| public void toCedarFormatThrowsIfNotJson() { | ||
| String cedarSchema = """ | ||
| entity Foo; | ||
| """; | ||
|
|
||
| assertThrows(InternalException.class, () -> { | ||
| Schema schema = Schema.parse(JsonOrCedar.Cedar, cedarSchema); | ||
| schema.toCedarFormat(); // should throw | ||
| }); | ||
| } | ||
|
|
||
| @Test | ||
| public void toJsonFormatThrowsIfNotCedar() { | ||
| String jsonSchema = """ | ||
| { | ||
| "schema": { | ||
| "entityTypes": { | ||
| "User": {} | ||
| } | ||
| } | ||
| } | ||
| """; | ||
|
|
||
| assertThrows(InternalException.class, () -> { | ||
| Schema schema = Schema.parse(JsonOrCedar.Json, jsonSchema); | ||
| schema.toJsonFormat(); // should throw | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
We can modify these tests based on what we decide in this comment:
@mark-creamer-amazon do we want it to throw an Exception in this case or just return the Schema? For example, if the schema is already in Cedar format and the user calls this method, we can return the schema which is already in the Cedar format.
| pub fn cedarToJsonJni<'a>(mut env: JNIEnv<'a>, _: JClass, cedar_schema: JString<'a>) -> jvalue { | ||
| match get_json_schema_internal(&mut env, cedar_schema) { | ||
| Ok(text) => match env.new_string(&text) { | ||
| Ok(jstr) => JValueGen::Object(JObject::from(jstr)).as_jni(), |
There was a problem hiding this comment.
Same as the comment above
| { | ||
| "schema": { | ||
| "entityTypes": { | ||
| "User": { | ||
| "memberOfTypes": ["Group"] | ||
| }, | ||
| "Group": {}, | ||
| "File": {} | ||
| }, | ||
| "actions": { | ||
| "read": { | ||
| "appliesTo": { | ||
| "principalTypes": ["User"], | ||
| "resourceTypes": ["File"] | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| "#; |
| let cedar = result.unwrap(); | ||
| assert!( | ||
| cedar.contains("entity User"), | ||
| "Expected output to contain 'entity User'" |
There was a problem hiding this comment.
This will have to be updated once the previous feedback is implemented (will have to convert JValue into string for assertion).
Also, assert on the full value instead of just entity User
|
|
||
| let json = result.unwrap(); | ||
| assert!( | ||
| json.contains("\"entityTypes\""), |
There was a problem hiding this comment.
Assert on full value
| }); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Add tests for Invalid values for both
1c377f3 to
38bea1f
Compare
3 participants
Issue #, if available:
Schema conversion methods for
Description of changes:
Wrote a new conversion method for Cedar -> JSON and JSON -> Cedar.
Along with the corresponding test cases for each method.