Filter operations on OpenAPI and GraphQL Schema #3018
Conversation
| case FETCH: | ||
| return canRead(clazz); | ||
| case DELETE: | ||
| return canDelete(clazz); |
There was a problem hiding this comment.
This would require UpdatePermission - not Delete.
There was a problem hiding this comment.
Changed to UpdatePermission
| @@ -16,7 +18,7 @@ | |||
| /** | |||
| * Assign custom Delete permission checks. | |||
| */ | |||
| @Target({TYPE, PACKAGE}) | |||
| @Target({TYPE, PACKAGE, FIELD, METHOD}) | |||
There was a problem hiding this comment.
There is no way to Delete a field or method in JSON-API. You can update relationships (by deleting elements) but that is an UpdatePermission. I recommend we continue to use UpdatePermission for relationship management.
There was a problem hiding this comment.
Removed these changes. This was actually done so that the DeletePermission could be applied to the id field to filter off the operations by id on OpenAPI which I think was requested for HJSON as I think those endpoints to query by id don't work. I have changed it to use the Exclude annotation instead.
| } | ||
|
|
||
| GraphQLEnumType enumResult = builder.build(); | ||
|
|
||
| enumConversions.put(enumClazz, enumResult); |
There was a problem hiding this comment.
Agree this method is a bit confusing. From what I gather it's not possible to set a graphql enum and indicate that only a subset of values are acceptable. Instead I needed to create a graphql enum per entity. eg. instead of just ElideRelationshipOp I needed to create one per entity, eg. ElideRelationshipOpDownloads, ElideRelationshipOpGroup, ElideRelationshipOpProduct and then indicate the acceptable values for each. In this case the enums needed to be deduplicated based on the graphql enum name and not the enum type. I have changed this to create a specific method for this use case instead of trying to modify the existing method to cater to this.
| return existing; | ||
| } | ||
|
|
||
| String entityName = entityDictionary.getJsonAliasFor(clazz); |
There was a problem hiding this comment.
I'm really confused what this code is doing. It looks like it is changing the value of relationship op to include the entity name. I'm not following the intent here, but we don't want to change the api contract. I also feel like we really need two distinct methods here - one to get the relationship ops for a root level entity and one to get the relationship ops for a relationship.
There was a problem hiding this comment.
I'm actually not that clear on the logic for determining the relationship ops from the permissions for graphql. Is there a difference in allowable values in the relationship ops for the root entity vs the relationship?
There was a problem hiding this comment.
Permissions are not as straightforward given that graphql mutations behave more like patch extension requests in JSON-API. Whether you can perform the operation or not depends not just on the operation being performed but on the data passed along with the operation.
Also some operations like UPSERT and REPLACE are really doing different things depending on the data.
Root entities:
- UPSERT - CreatePermission OR UpdatePermission (depending on whether the object already exists or not)
- DELETE - DeletePermission
- REMOVE - DeletePermission
- REPLACE - CreatePermission OR UpdatePermission OR DeletePermission
- FETCH - ReadPermission
- UPDATE - UpdatePermission
Relationships:
- FETCH - ReadPermission (on the field)
- REMOVE - UpdatePermission (on the field)
- DELETE - DeletePermission (on the type being deleted)
- UPSERT - CreatePermission (on the type being created) OR UpdatePermission(on the field)
- REPLACE - CreatePermission OR UpdatePermission (field and type being manipulated) OR DeletePermission
- UPDATE - UpdatePermission (on the field)
We can possibly simplify this though to the following:
Root entities:
- UPSERT - CreatePermission
- DELETE - DeletePermission
- REMOVE - DeletePermission
- REPLACE - Too complex to put a restriction on. Just always allow it.
- FETCH - ReadPermission
- UPDATE - UpdatePermission
Relationships:
- FETCH - ReadPermission (on the field)
- REMOVE - UpdatePermission (on the field)
- DELETE - DeletePermission (on the type being deleted)
- UPSERT - UpdatePermission (on the field)
- REPLACE - UpdatePermission (on the field)
- UPDATE - UpdatePermission (on the field)
Alternatively, we don't solve for GraphQL at all this way and punt. Maybe we need a more explicit way to filter out operations in the graphql schema.
There was a problem hiding this comment.
I think it makes sense to at least attempt to filter the operations as that is better than nothing. I retained some criteria of restricting REPLACE as I think the read only use case should be relatively common and exposing REPLACE is misleading.
|
|
| case FETCH: | ||
| return canRead(clazz); | ||
| case DELETE: | ||
| return canUpdate(clazz); |
There was a problem hiding this comment.
I think this is actually canDelete as it deletes the underlying objects as opposed to simply removing them from the relationship.
There was a problem hiding this comment.
Have changed it to canDelete.
| String queryPath = String.join(" ", validationError.getQueryPath()); | ||
| result.add(ValidationError.newValidationError() | ||
| .description( | ||
| "Invalid operation: " + queryPath + " is read only.") |
There was a problem hiding this comment.
Under what circumstances does this error get generated? How do we know it is an attempt to write something that is read only?
There was a problem hiding this comment.
This gets generated in the event an invalid value is used that couldn't map to the GraphQL enum.
I was trying to get the pre-existing test in NoCacheAggregationDataStoreIntegrationTest to pass but I guess I should change the expectation instead.
| @@ -248,28 +254,46 @@ public PathItem getRelationshipPath() { | |||
| RelationshipType relationshipType = dictionary.getRelationshipType(parentClass, name); | |||
|
|
|||
| if (relationshipType.isToMany()) { | |||
| path.get(new Operation().tags(getTags()).description("Returns the relationship identifiers for " + name) | |||
| .responses(new ApiResponses().addApiResponse("200", okPluralResponse))); | |||
| if (canCreate(type)) { | |||
There was a problem hiding this comment.
Similar to GraphQL, when you are POSTING to a relationship, we are not creating the type but just updating the relationship (which is Update permission). All of the relationship edits are effectively Updates.
There was a problem hiding this comment.
Have made the changes.
Resolves #2641
I wasn't sure what to use to determine whether or not to expose operations by id for OpenAPI so it's currently using Permission annotations on the id which is rather odd.
Description
Motivation and Context
More accurate documentation on what operations are supported on the backend.
How Has This Been Tested?
Added unit tests.
License
I confirm that this contribution is made under an Apache 2.0 license and that I have the authority necessary to make this contribution on behalf of its copyright owner.