multinet-app / multinet-api

Apache License 2.0
2 stars 2 forks source link

Investigate how responses are handled in testing #72

Open jjnesbitt opened 3 years ago

jjnesbitt commented 3 years ago

It appears that in a testing context (when using api_client or authenticated_api_client), data is directly returned from an api endpoint, without serialization. For example, if you directly return an arangodb cursor from an endpoint, inspecting r.data shows that the type is Cursor. This is in contrast to what we expect from the endpoint, which is serialized JSON. In my example, we can still call r.json() and it works as expected, but IMO this is still a slight issue, as it represents a difference in our test cases vs reality.

This doesn't usually crop up due to our use serializers in most places, but for the rare cases where we don't use serializers, this could cause issues. If nothing else we should be aware of this behavior, but we should also see what we can do to mitigate this.

jjnesbitt commented 3 years ago

Some relevant docs: https://www.django-rest-framework.org/api-guide/testing/#rendering-responses

waxlamp commented 3 years ago

When don't we use serializers?

naglepuff commented 3 years ago

At least one endpoint: GET /api/workspaces/{workspace}/aql/ doesn't use a serializer for the response. It just creates a response object and passes in the Cursor object returned as a result of executing the query with python-arango. @AlmightyYakob might know of another place where we don't use serializers. For this particular case, it might be possible to use a ListSerializer with a child JSONSerializer. However, in this case we don't necessarily know what the result will look like. If an AQL query evaluates to a list of string values, I'm not sure how JSONSerializer would handle that.

waxlamp commented 3 years ago

I don't think we should use serializers simply because we're facing the problem outlined in this issue (without further understanding of the problem), but just to be nerdy: a single string is in fact a basic JSON value, and therefore a ListSerializer with a JSONSerializer should (on the face of it) be just fine.

@AlmightyYakob, are there other places where we lack a serializer? And in the case of the AQL endpoint, what is the magic that allows for constructing a Response with a python-arango Cursor?

jjnesbitt commented 3 years ago

Another place we don't use serializers is in the table get_rows endpoint, as well as the endpoints for retrieving network nodes and edges. The response is paginated however, so it's not returning a cursor directly. I'm not sure what allows for the cursor to be returned directly, but I'm guessing somewhere in django's request lifecycle, it converts the iterable to a list/json.

Initially, I was thinking it was best to just return the cursor directly, since it works in Django, and it's the most direct path. The main reason for me opening this issue is that since it works in the general django response, I'd like the same response to exist in testing. We could use a serializer in this response, and maybe we should, but if it's just an issue with testing, I'd like to try to solve it there, rather than change our endpoint behavior to accommodate testing. From the docs I linked, it looks like there may be a valid approach there. However, I wouldn't be opposed to using a serializer in that endpoint directly either, so long as it doesn't come with serious disadvantages (which I doubt it will).