Possible bug: When the content delivery API is prohibited from accessing a model because its associated user role doesnât have the right permissions, it still returns a 200 status code along with empty response arrays as data, like:
To the client, this is indistinguishable from models that are actually empty. I think it would be clearer if either the HTTP code errored out as 401 unauth, or if the GraphQL response included the errors member (per graphql error docs). Either way, the Dato SDK/clients should also realize the request was not successful.
We had a case where someone accidentally changed a user role and blanked all of our already-deployed static pages. Next.jsâs incremental static regeneration pinged the Content Delivery API and got back a successful (albeit empty) array and so deleted all our content, mistakenly believing they were updated and emptied on purpose (when reality it was just a permissions issue, and shouldâve caused a failed update and no regeneration instead).
In summary, I believe a zero-length array due to model emptiness should be different from a permissions issue due to the associated user role not having access.
Since the introduction of workflows and transator roles we have refactored our permission system, but I was able to test locally and I can confirm that CDA returned an empty array even before these two new features.
So at least we can exclude a bug
This happens to be the default behavior of our graphql server:
By default, GraphQL-Ruby silently replaces unauthorized objects with nil, as if they didnât exist.
I think we can improve our documentation because this part is missing
@fabrizio , thanks for looking into it! Would it be possible to have your server return an error instead? From the GraphQL-Ruby docs, maybe throwing an error by overriding the default behavior:
class MySchema < GraphQL::Schema
# Override this hook to handle cases when `authorized?` returns false for an object:
def self.unauthorized_object(error)
# Add a top-level error to the response instead of returning nil:
raise GraphQL::ExecutionError, "An object of type #{error.type.graphql_name} was hidden due to permissions"
end
end
If it was just added to the documentation as a note, it still creates kind of an unexpected âgotchaâ moment because a valid, 0-length array signifies something different (a new or empty model) vs an actual error (insufficient permissions).
Itâs hard for downstream functions/promise resolvers to deal with this situation when the dato client makes it LOOK like a success even though it was an error. Yes, we can check for array.length everywhere, but that both makes for messy code thatâs harder to debug (especially inside nested loops) and precludes the use of actual empty arrays (such as checking for new models).
Hey @roger, Iâll add my 2 cents to the conversation.
The thing is, itâs not black and white Depending on the permissions set on the API token, you might be able to see just a couple records, the complete collection, or no records at all. The fact that a certain token cannot see any record is not necessarily an error per se. Given the same permissions, 5 minutes later you might see something that passes the checks.
So Iâd argue that it could be counterintuitive/a problem for an application if we returned an error for some query, and then 5 minutes later no error, just because some new records were created in the meantime. In other words, I see your point, but the âunexpectednessâ of the API interface really depends on the use case I think.
âŚnot to mention the fact that we cannot change such a basic behavior without upgrading the version of the GraphQL API
Hmm, not super familiar with GraphQL errors, but in that case⌠would it be OK to have it return BOTH the array (empty or not) AND more details in the errors member? Like:
{
"data": {
"allExhibitions": [],
"allBasicPages": []
}
"errors": [
{
"message": "Some entries for model [X] were hidden due to permissions. Partial results returned.",
"location", "path", etc...
}
]
}
Or would that also be a breaking change in the API? (Not sure if there are ânon-fatalâ graphQL errors that clients could look for without breaking preexisting queries)
Depending on the permissions set on the API token, you might be able to see just a couple records, the complete collection, or no records at all. The fact that a certain token cannot see any record is not necessarily an error per se. Given the same permissions, 5 minutes later you might see something that passes the checks.