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

Any reason for having HyperlinkedModelSerializer instead of ModelSerializer? #228

Closed
Serhiy-Shekhovtsov opened this Issue Nov 16, 2017 · 3 comments

Comments

Projects
None yet
3 participants
@Serhiy-Shekhovtsov
Copy link
Contributor

Serhiy-Shekhovtsov commented Nov 16, 2017

Is there any reason for using HyperlinkedModelSerializer for serializing the data before sending it to client side?

Having this serializer have following drawbacks:

  • Returned data doesn't have the id. Here is an example of data for /api/candidates.json method:
    {"url":"http://interface:8001/api/candidates/1.json","centroid":{ ... },"created":" ... ","probability_concerning":0.3,"case":"http://interface:8001/api/cases/1.json"}
  • Returned URL is not usable.
    • As you can see, it's absolute and since the port doesn't match the Vue.js server's port - AJAX requests to that URL are blocked by the browser.
    • Also the url contains .json extension. For example, services we have for marking/dismissing candidates look like this: candidates/1/mark, candidates/1/dismiss

So, what are the benefits?

Maybe I am missing something here, since I am totally new to django, but it looks like replacing HyperlinkedModelSerializer with ModelSerializer helps to solve the issue of missing id and it also doesn't return unusable urls.

Possible Solution

AFAIK, we can still use HyperlinkedModelSerializer but patch it by overriding a method for creating serializer and passing None as request. Or we can simply use to ModelSerializer.

@isms

This comment has been minimized.

Copy link
Contributor

isms commented Nov 17, 2017

Hyperlinked fields let the frontend get canonical URLs from the backend rather than doing domain + id + endpoint stuff in frontend JS which will immediately break if the endpoints change or if a field other than id is used for lookups in the future. Instead the idea is that you can fetch a related resource by fetching object.url. You can make it work either way but including links seems to be considered truer to the REST concept.

To address the two specifics:

  1. "Also the url contains .json extension" - is there an example of an issue? Or is this a setting that can simply be changed on the serializer or view? Seems like views are still free to return custom endpoints like /mark and /dismiss that don't go by this scheme as the only thing HyperlinkedModelSerializer tries to add links for is the specific instance it is serializing.

  2. Looks like we may be able to get relative URLs simply by having the [Thing]ViewSet views in backend/api/views.py all extend a class which extends the viewsets.ModelViewSet's get_serializer_context and overriding the parent context making sure request=None (ref: encode/django-rest-framework#3532 (comment))

Is there are strong reason to prefer reverting to non-hyperlinked versions?

Serhiy-Shekhovtsov added a commit to Serhiy-Shekhovtsov/concept-to-clinic that referenced this issue Nov 18, 2017

Serhiy-Shekhovtsov added a commit to Serhiy-Shekhovtsov/concept-to-clinic that referenced this issue Nov 18, 2017

Serhiy-Shekhovtsov added a commit to Serhiy-Shekhovtsov/concept-to-clinic that referenced this issue Nov 18, 2017

@Serhiy-Shekhovtsov

This comment has been minimized.

Copy link
Contributor

Serhiy-Shekhovtsov commented Nov 18, 2017

@isms, your suggestion to inherit all from base and override the get_serializer_context is nice, didn't think about it. And after I have implemented it the issue with .json is also gone.

Btw, here are my findings about .json issue: When request is not set to None for the serializer, then generated URL will be absolute and also copy the extension of the query. If you load /api/candidates.json each candidate will have URL xxx/api/candidates/1.json.

Serhiy-Shekhovtsov added a commit to Serhiy-Shekhovtsov/concept-to-clinic that referenced this issue Nov 18, 2017

Serhiy-Shekhovtsov added a commit to Serhiy-Shekhovtsov/concept-to-clinic that referenced this issue Nov 18, 2017

@isms

This comment has been minimized.

Copy link
Contributor

isms commented Nov 18, 2017

Closed by #231 - thanks @Serhiy-Shekhovtsov.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment