Skip to content
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

Special case for when OneToOneField is also primary key. #5192

Merged
merged 2 commits into from Jun 16, 2017

Conversation

matteius
Copy link
Contributor

@matteius matteius commented May 30, 2017

Description

Closes: #5135

I've been working this issue occasionally since the last day of the PyCon sprints. I setup a test case to reproduce the problem from the issue and then iterated in a debugger to determine where I thought the best spot to make a code change was. I determined that build_standard_field feels like the right place for a couple reasons. One was that I first tried to do the change in get_field_names except that expects all the results to be retuned Django fields and I am trying to override it to be the right serializer field. I followed the code further down and determined that it was falling back to the generic models.Field: ModelField in serializer_field_mapping.

I found two convenient attributes on the field class which is one_to_one and primary_key so if both are True then we know we have to handle this special case. If you comment out the three code lines in the serialziers.py it will fail my newly added tests for this case.

Please let me know if you think this change should be amended somehow, but my confidence level is high enough now to put out a PR for fixing this issue as I didn't think of anything else that needs to be considered.

@tomchristie
Copy link
Member

tomchristie commented Jun 16, 2017

Looks great! Nice work 😄

@tomchristie tomchristie merged commit 598e587 into encode:master Jun 16, 2017
1 check passed
@tomchristie tomchristie added this to the 3.6.4 Release milestone Jun 16, 2017
@marshalc
Copy link

marshalc commented Jan 29, 2019

I think I may have found another related edge case here. I'm being faced with a 'view_name' argument is required AssertionError whilst trying to use DRF 3.9.1 (also with 3.9.0).

The stack trace looks like...

Traceback (most recent call last):
  File "/Users/carl/.virtualenvs/diakonia-internal/lib/python3.7/site-packages/django/core/handlers/exception.py", line 34, in inner
    response = get_response(request)
  File "/Users/carl/.virtualenvs/diakonia-internal/lib/python3.7/site-packages/django/core/handlers/base.py", line 126, in _get_response
    response = self.process_exception_by_middleware(e, request)
  File "/Users/carl/.virtualenvs/diakonia-internal/lib/python3.7/site-packages/django/core/handlers/base.py", line 124, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/usr/local/Cellar/python/3.7.2/Frameworks/Python.framework/Versions/3.7/lib/python3.7/contextlib.py", line 74, in inner
    return func(*args, **kwds)
  File "/Users/carl/.virtualenvs/diakonia-internal/lib/python3.7/site-packages/django/views/decorators/csrf.py", line 54, in wrapped_view
    return view_func(*args, **kwargs)
  File "/Users/carl/.virtualenvs/diakonia-internal/lib/python3.7/site-packages/rest_framework/viewsets.py", line 116, in view
    return self.dispatch(request, *args, **kwargs)
  File "/Users/carl/.virtualenvs/diakonia-internal/lib/python3.7/site-packages/rest_framework/views.py", line 495, in dispatch
    response = self.handle_exception(exc)
  File "/Users/carl/.virtualenvs/diakonia-internal/lib/python3.7/site-packages/rest_framework/views.py", line 455, in handle_exception
    self.raise_uncaught_exception(exc)
  File "/Users/carl/.virtualenvs/diakonia-internal/lib/python3.7/site-packages/rest_framework/views.py", line 492, in dispatch
    response = handler(request, *args, **kwargs)
  File "/Users/carl/.virtualenvs/diakonia-internal/lib/python3.7/site-packages/rest_framework/mixins.py", line 45, in list
    return self.get_paginated_response(serializer.data)
  File "/Users/carl/.virtualenvs/diakonia-internal/lib/python3.7/site-packages/rest_framework/serializers.py", line 768, in data
    ret = super(ListSerializer, self).data
  File "/Users/carl/.virtualenvs/diakonia-internal/lib/python3.7/site-packages/rest_framework/serializers.py", line 262, in data
    self._data = self.to_representation(self.instance)
  File "/Users/carl/.virtualenvs/diakonia-internal/lib/python3.7/site-packages/rest_framework/serializers.py", line 686, in to_representation
    self.child.to_representation(item) for item in iterable
  File "/Users/carl/.virtualenvs/diakonia-internal/lib/python3.7/site-packages/rest_framework/serializers.py", line 686, in <listcomp>
    self.child.to_representation(item) for item in iterable
  File "/Users/carl/.virtualenvs/diakonia-internal/lib/python3.7/site-packages/rest_framework/serializers.py", line 513, in to_representation
    fields = self._readable_fields
  File "/Users/carl/.virtualenvs/diakonia-internal/lib/python3.7/site-packages/django/utils/functional.py", line 37, in __get__
    res = instance.__dict__[self.name] = self.func(instance)
  File "/Users/carl/.virtualenvs/diakonia-internal/lib/python3.7/site-packages/rest_framework/serializers.py", line 376, in _readable_fields
    field for field in self.fields.values()
  File "/Users/carl/.virtualenvs/diakonia-internal/lib/python3.7/site-packages/rest_framework/serializers.py", line 363, in fields
    for key, value in self.get_fields().items():
  File "/Users/carl/.virtualenvs/diakonia-internal/lib/python3.7/site-packages/rest_framework/serializers.py", line 1059, in get_fields
    fields[field_name] = field_class(**field_kwargs)
  File "/Users/carl/.virtualenvs/diakonia-internal/lib/python3.7/site-packages/rest_framework/relations.py", line 290, in __init__
    assert self.view_name is not None, 'The `view_name` argument is required.'
AssertionError: The `view_name` argument is required.

The only thing I'm changing from having a working return, is that I'm overriding the get_default_field_names() in the HyperlinkedModelSerializer because I need to be able to see the Primary Key in the result data, and not have it hidden (as is the default case) by the URL. Thus I've created this custom Serializer

class AtlasHyperLinkedModelSerialiser(serializers.HyperlinkedModelSerializer):
    # Override the default behavior of the HyperlinkedModelSerializer to replace the PK with a URL, and instead ADD the URL along with the PK
    def get_default_field_names(self, declared_fields, model_info):
        """
        Return the default list of field names that will be used if the
        `Meta.fields` option is not specified.
        """
        return (
            [self.url_field_name] +
            [model_info.pk.name] +
            list(declared_fields) +
            list(model_info.fields) +
            list(model_info.forward_relations)
        )

If I remove the [model_info.pk.name] + from the call then this is identical to the original and works fine.

The edge case may be a result of my working with a legacy Database that is read-only and whose schema was defined by third-parties. The only models to have problems with the modified serialiser above are the ones with a OneToOneField as their primary key, e.g.:

class PersonPatient(models.Model):
    person_id = models.OneToOneField(Person, models.DO_NOTHING, primary_key=True, db_column='PERSON_ID')

or

class Location(models.Model):
    location_cd = models.OneToOneField(CodeValue, models.DO_NOTHING, db_column='location_cd', primary_key=True)

...which would seem to rule out it being an issue related to naming conventions for FKs.

I'm now a little lost and lacking the time to dig deeper into DRF's workings here, so I'm hoping that something obvious will jump out to this audience as they've looked at this issue already. I should add that I'm chasing this solution as having to add all the fields specifically is something of an immense ask on a DB with 6000+ tables, some with near 100 columns - using __all__ is a coding lifesaver!

Suggestions please?

I've got tables with composite primary keys in them, and they work fine for listings, but break on direct references, but that's another separate problem I'm debugging...

@marshalc
Copy link

marshalc commented Jan 29, 2019

Additionally, having tried to manually list all the fields on one of the smaller tables, I get the same error:

Traceback (most recent call last):
  File "/Users/carl/.virtualenvs/diakonia-internal/lib/python3.7/site-packages/django/core/handlers/exception.py", line 34, in inner
    response = get_response(request)
  File "/Users/carl/.virtualenvs/diakonia-internal/lib/python3.7/site-packages/django/core/handlers/base.py", line 126, in _get_response
    response = self.process_exception_by_middleware(e, request)
  File "/Users/carl/.virtualenvs/diakonia-internal/lib/python3.7/site-packages/django/core/handlers/base.py", line 124, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/usr/local/Cellar/python/3.7.2/Frameworks/Python.framework/Versions/3.7/lib/python3.7/contextlib.py", line 74, in inner
    return func(*args, **kwds)
  File "/Users/carl/.virtualenvs/diakonia-internal/lib/python3.7/site-packages/django/views/decorators/csrf.py", line 54, in wrapped_view
    return view_func(*args, **kwargs)
  File "/Users/carl/.virtualenvs/diakonia-internal/lib/python3.7/site-packages/rest_framework/viewsets.py", line 116, in view
    return self.dispatch(request, *args, **kwargs)
  File "/Users/carl/.virtualenvs/diakonia-internal/lib/python3.7/site-packages/rest_framework/views.py", line 495, in dispatch
    response = self.handle_exception(exc)
  File "/Users/carl/.virtualenvs/diakonia-internal/lib/python3.7/site-packages/rest_framework/views.py", line 455, in handle_exception
    self.raise_uncaught_exception(exc)
  File "/Users/carl/.virtualenvs/diakonia-internal/lib/python3.7/site-packages/rest_framework/views.py", line 492, in dispatch
    response = handler(request, *args, **kwargs)
  File "/Users/carl/.virtualenvs/diakonia-internal/lib/python3.7/site-packages/rest_framework/mixins.py", line 45, in list
    return self.get_paginated_response(serializer.data)
  File "/Users/carl/.virtualenvs/diakonia-internal/lib/python3.7/site-packages/rest_framework/serializers.py", line 768, in data
    ret = super(ListSerializer, self).data
  File "/Users/carl/.virtualenvs/diakonia-internal/lib/python3.7/site-packages/rest_framework/serializers.py", line 262, in data
    self._data = self.to_representation(self.instance)
  File "/Users/carl/.virtualenvs/diakonia-internal/lib/python3.7/site-packages/rest_framework/serializers.py", line 686, in to_representation
    self.child.to_representation(item) for item in iterable
  File "/Users/carl/.virtualenvs/diakonia-internal/lib/python3.7/site-packages/rest_framework/serializers.py", line 686, in <listcomp>
    self.child.to_representation(item) for item in iterable
  File "/Users/carl/.virtualenvs/diakonia-internal/lib/python3.7/site-packages/rest_framework/serializers.py", line 513, in to_representation
    fields = self._readable_fields
  File "/Users/carl/.virtualenvs/diakonia-internal/lib/python3.7/site-packages/django/utils/functional.py", line 37, in __get__
    res = instance.__dict__[self.name] = self.func(instance)
  File "/Users/carl/.virtualenvs/diakonia-internal/lib/python3.7/site-packages/rest_framework/serializers.py", line 376, in _readable_fields
    field for field in self.fields.values()
  File "/Users/carl/.virtualenvs/diakonia-internal/lib/python3.7/site-packages/rest_framework/serializers.py", line 363, in fields
    for key, value in self.get_fields().items():
  File "/Users/carl/.virtualenvs/diakonia-internal/lib/python3.7/site-packages/rest_framework/serializers.py", line 1059, in get_fields
    fields[field_name] = field_class(**field_kwargs)
  File "/Users/carl/.virtualenvs/diakonia-internal/lib/python3.7/site-packages/rest_framework/relations.py", line 290, in __init__
    assert self.view_name is not None, 'The `view_name` argument is required.'
AssertionError: The `view_name` argument is required.

and that's with a model of...

class Location(models.Model):
    location_cd = models.OneToOneField(CodeValue, models.DO_NOTHING, db_column='location_cd', primary_key=True)

    apache_reltn_flag = models.FloatField(editable=False, )
    census_ind = models.FloatField(editable=False, blank=True, null=True)
    chart_format_id = models.ForeignKey(ChartFormat, models.DO_NOTHING, blank=True, null=True, db_column='CHART_FORMAT_ID')
    contributor_source_cd = models.FloatField(editable=False, blank=True, null=True)
    contributor_system_cd = models.FloatField(editable=False, )
    data_status_cd = models.FloatField(editable=False, )
    data_status_dt_tm = models.DateField(editable=False, blank=True, null=True)
    data_status_prsnl_id = models.ForeignKey(Prsnl, models.DO_NOTHING, editable=False, db_column='DATA_STATUS_PRSNL_ID')
    discipline_type_cd = models.FloatField(editable=False, )
    exp_lvl_cd = models.FloatField(editable=False, blank=True, null=True)
    ref_lab_acct_nbr = models.CharField(editable=False, max_length=20, blank=True, null=True)
    facility_accn_prefix_cd = models.FloatField(editable=False, )
    icu_ind = models.FloatField(editable=False, blank=True, null=True)
    location_type_cd = models.ForeignKey(CodeValue, models.DO_NOTHING, db_column='LOCATION_TYPE_CD')
    organization_id = models.ForeignKey(Organization, models.DO_NOTHING, db_column='ORGANIZATION_ID')
    patcare_node_ind = models.FloatField(editable=False, blank=True, null=True)
    registration_ind = models.FloatField(editable=False, blank=True, null=True)
    reserve_ind = models.FloatField(editable=False, )
    resource_ind = models.FloatField(editable=False, blank=True, null=True)
    transfer_dt_tm_ind = models.FloatField(editable=False, )
    transmit_outbound_order_ind = models.FloatField(editable=False, blank=True, null=True)

    # Obsolete fields (according to the DocInfo)
    # view_type_cd = models.ForeignKey(CodeValue, models.DO_NOTHING, db_column='VIEW_TYPE_CD')
    view_type_cd = models.FloatField(editable=False, blank=True, null=True)  # Break the FK link, but see the data

    # Common date boundaries for the record
    beg_effective_dt_tm = models.DateField(editable=False, )
    end_effective_dt_tm = models.DateField(editable=False, )

    def __str__(self):
        return "Location #{0}".format(self.location_cd.display)

    class Meta:
        managed = False
        db_table = 'LOCATION'

and a serialiser of:

class LocationSerialiser(serializers.HyperlinkedModelSerializer):
    class Meta:
        model = Location
        # fields = '__all__'
        fields = ('location_cd', 'apache_reltn_flag', 'census_ind', 'chart_format_id', 'contributor_source_cd',
                  'contributor_system_cd', 'data_status_cd', 'data_status_dt_tm', 'data_status_prsnl_id',
                  'discipline_type_cd', 'exp_lvl_cd', 'ref_lab_acct_nbr', 'facility_accn_prefix_cd', 'icu_ind',
                  'location_type_cd', 'organization_id', 'patcare_node_ind', 'registration_ind', 'reserve_ind',
                  'resource_ind', 'transfer_dt_tm_ind', 'transmit_outbound_order_ind', 'view_type_cd',
                  'beg_effective_dt_tm', 'end_effective_dt_tm')

This makes me suspect the problem is a bit deeper on the OneToOneField as PK and not just related to adding in the PK to the default_fields.

@xordoquy
Copy link
Collaborator

xordoquy commented Jan 29, 2019

The discussion group is the best place to take this discussion and other usage questions. Thanks!

@marshalc
Copy link

marshalc commented Jan 30, 2019

Noted, thank you, and posted...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OneToOneField should be PrimaryKeyRelatedField, not ModelField, raises exception
4 participants