WIP: workaround "'PKOnlyObject' object has no attribute" error for toOne related #492
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I will have to read into those code bits a bit more to understand whether this fix is the right way to go. In anyway this would be much easier once a test is added. It would be great if you could follow up on that. Once done I can have a closer look. |
|
working on it. |
…uteError exception.
n2ygk
commented
Oct 13, 2018
| @@ -319,6 +327,32 @@ def test_retrieve_related_None(self): | |||
| self.assertEqual(resp.status_code, 200) | |||
| self.assertEqual(resp.json(), {'data': None}) | |||
|
|
|||
| # the following test reproduces/confirm fix for this bug: | |||
There was a problem hiding this comment.
@sliverc here's the test case that reproduces/fixes the bug. I made two new models and put sample data in drf_example as well so you can play around with this in dev as well as reproduce with test code.
There was a problem hiding this comment.
OK good. I will have a look but I am not sure when I will get to it.
There was a problem hiding this comment.
Thank-you. Some of these blames may help understand it:
(env) django-rest-framework-json-api$ git blame -L136,220 rest_framework_json_api/serializers.py
d62b455f (Jonathan Senecal 2015-09-28 22:47:55 -0400 136) class ModelSerializer(IncludedResourcesValidationMixin, SparseFieldsetsMixin, ModelSerializer):
. . .
22c4587c (Tim Selman 2018-09-04 09:55:17 +0200 194) def _get_field_representation(self, field, instance):
22c4587c (Tim Selman 2018-09-04 09:55:17 +0200 195) request = self.context.get('request')
22c4587c (Tim Selman 2018-09-04 09:55:17 +0200 196) is_included = field.source in get_included_resources(request)
22c4587c (Tim Selman 2018-09-04 09:55:17 +0200 197) if not is_included and \
22c4587c (Tim Selman 2018-09-04 09:55:17 +0200 198) isinstance(field, ModelSerializer) and \
22c4587c (Tim Selman 2018-09-04 09:55:17 +0200 199) hasattr(instance, field.source + '_id'):
22c4587c (Tim Selman 2018-09-04 09:55:17 +0200 200) attribute = getattr(instance, field.source + '_id')
22c4587c (Tim Selman 2018-09-04 09:55:17 +0200 201)
22c4587c (Tim Selman 2018-09-04 09:55:17 +0200 202) if attribute is None:
22c4587c (Tim Selman 2018-09-04 09:55:17 +0200 203) return None
22c4587c (Tim Selman 2018-09-04 09:55:17 +0200 204)
22c4587c (Tim Selman 2018-09-04 09:55:17 +0200 205) resource_type = get_resource_type_from_serializer(field)
22c4587c (Tim Selman 2018-09-04 09:55:17 +0200 206) if resource_type:
22c4587c (Tim Selman 2018-09-04 09:55:17 +0200 207) return OrderedDict([('type', resource_type), ('id', attribute)])
22c4587c (Tim Selman 2018-09-04 09:55:17 +0200 208)
22c4587c (Tim Selman 2018-09-04 09:55:17 +0200 209) attribute = field.get_attribute(instance)
22c4587c (Tim Selman 2018-09-04 09:55:17 +0200 210)
22c4587c (Tim Selman 2018-09-04 09:55:17 +0200 211) # We skip `to_representation` for `None` values so that fields do
22c4587c (Tim Selman 2018-09-04 09:55:17 +0200 212) # not have to explicitly deal with that case.
22c4587c (Tim Selman 2018-09-04 09:55:17 +0200 213) #
22c4587c (Tim Selman 2018-09-04 09:55:17 +0200 214) # For related fields with `use_pk_only_optimization` we need to
22c4587c (Tim Selman 2018-09-04 09:55:17 +0200 215) # resolve the pk value.
22c4587c (Tim Selman 2018-09-04 09:55:17 +0200 216) check_for_none = attribute.pk if isinstance(attribute, PKOnlyObject) else attribute
22c4587c (Tim Selman 2018-09-04 09:55:17 +0200 217) if check_for_none is None:
22c4587c (Tim Selman 2018-09-04 09:55:17 +0200 218) return None
22c4587c (Tim Selman 2018-09-04 09:55:17 +0200 219) else:
22c4587c (Tim Selman 2018-09-04 09:55:17 +0200 220) return field.to_representation(attribute)
(env) django-rest-framework-json-api$ git blame -L169,199 rest_framework_json_api/relations.py
cb7f830a (Anton Shutik 2018-07-19 16:06:57 +0300 169) class ResourceRelatedField(HyperlinkedMixin, PrimaryKeyRelatedField):
cb7f830a (Anton Shutik 2018-07-19 16:06:57 +0300 170) _skip_polymorphic_optimization = True
cb7f830a (Anton Shutik 2018-07-19 16:06:57 +0300 171) self_link_view_name = None
cb7f830a (Anton Shutik 2018-07-19 16:06:57 +0300 172) related_link_view_name = None
cb7f830a (Anton Shutik 2018-07-19 16:06:57 +0300 173) related_link_lookup_field = 'pk'
cb7f830a (Anton Shutik 2018-07-19 16:06:57 +0300 174)
cb7f830a (Anton Shutik 2018-07-19 16:06:57 +0300 175) default_error_messages = {
cb7f830a (Anton Shutik 2018-07-19 16:06:57 +0300 176) 'required': _('This field is required.'),
cb7f830a (Anton Shutik 2018-07-19 16:06:57 +0300 177) 'does_not_exist': _('Invalid pk "{pk_value}" - object does not exist.'),
cb7f830a (Anton Shutik 2018-07-19 16:06:57 +0300 178) 'incorrect_type': _(
cb7f830a (Anton Shutik 2018-07-19 16:06:57 +0300 179) 'Incorrect type. Expected resource identifier object, received {data_type}.'
cb7f830a (Anton Shutik 2018-07-19 16:06:57 +0300 180) ),
cb7f830a (Anton Shutik 2018-07-19 16:06:57 +0300 181) 'incorrect_relation_type': _(
cb7f830a (Anton Shutik 2018-07-19 16:06:57 +0300 182) 'Incorrect relation type. Expected {relation_type}, received {received_type}.'
cb7f830a (Anton Shutik 2018-07-19 16:06:57 +0300 183) ),
cb7f830a (Anton Shutik 2018-07-19 16:06:57 +0300 184) 'missing_type': _('Invalid resource identifier object: missing \'type\' attribute'),
cb7f830a (Anton Shutik 2018-07-19 16:06:57 +0300 185) 'missing_id': _('Invalid resource identifier object: missing \'id\' attribute'),
cb7f830a (Anton Shutik 2018-07-19 16:06:57 +0300 186) 'no_match': _('Invalid hyperlink - No URL match.'),
cb7f830a (Anton Shutik 2018-07-19 16:06:57 +0300 187) }
cb7f830a (Anton Shutik 2018-07-19 16:06:57 +0300 188)
cb7f830a (Anton Shutik 2018-07-19 16:06:57 +0300 189) def __init__(self, **kwargs):
cb7f830a (Anton Shutik 2018-07-19 16:06:57 +0300 190) # check for a model class that was passed in for the relation type
cb7f830a (Anton Shutik 2018-07-19 16:06:57 +0300 191) model = kwargs.pop('model', None)
cb7f830a (Anton Shutik 2018-07-19 16:06:57 +0300 192) if model:
cb7f830a (Anton Shutik 2018-07-19 16:06:57 +0300 193) self.model = model
cb7f830a (Anton Shutik 2018-07-19 16:06:57 +0300 194)
cb7f830a (Anton Shutik 2018-07-19 16:06:57 +0300 195) super(ResourceRelatedField, self).__init__(**kwargs)
cb7f830a (Anton Shutik 2018-07-19 16:06:57 +0300 196)
cb7f830a (Anton Shutik 2018-07-19 16:06:57 +0300 197) def use_pk_only_optimization(self):
cb7f830a (Anton Shutik 2018-07-19 16:06:57 +0300 198) # We need the real object to determine its type...
cb7f830a (Anton Shutik 2018-07-19 16:06:57 +0300 199) return self.get_resource_type_from_included_serializer() is not None
(env) django-rest-framework-json-api$ git blame -L308,327 rest_framework_json_api/relations.py
61406625 (leo-naeka 2017-06-05 22:23:17 +0200 308) class PolymorphicResourceRelatedField(ResourceRelatedField):
61406625 (leo-naeka 2017-06-05 22:23:17 +0200 309) """
61406625 (leo-naeka 2017-06-05 22:23:17 +0200 310) Inform DRF that the relation must be considered polymorphic.
61406625 (leo-naeka 2017-06-05 22:23:17 +0200 311) Takes a `polymorphic_serializer` as the first positional argument to
61406625 (leo-naeka 2017-06-05 22:23:17 +0200 312) retrieve then validate the accepted types set.
61406625 (leo-naeka 2017-06-05 22:23:17 +0200 313) """
61406625 (leo-naeka 2017-06-05 22:23:17 +0200 314)
61406625 (leo-naeka 2017-06-05 22:23:17 +0200 315) _skip_polymorphic_optimization = False
61406625 (leo-naeka 2017-06-05 22:23:17 +0200 316) default_error_messages = dict(ResourceRelatedField.default_error_messages, **{
61406625 (leo-naeka 2017-06-05 22:23:17 +0200 317) 'incorrect_relation_type': _('Incorrect relation type. Expected one of [{relation_type}], '
61406625 (leo-naeka 2017-06-05 22:23:17 +0200 318) 'received {received_type}.'),
61406625 (leo-naeka 2017-06-05 22:23:17 +0200 319) })
61406625 (leo-naeka 2017-06-05 22:23:17 +0200 320)
61406625 (leo-naeka 2017-06-05 22:23:17 +0200 321) def __init__(self, polymorphic_serializer, *args, **kwargs):
61406625 (leo-naeka 2017-06-05 22:23:17 +0200 322) self.polymorphic_serializer = polymorphic_serializer
61406625 (leo-naeka 2017-06-05 22:23:17 +0200 323) super(PolymorphicResourceRelatedField, self).__init__(*args, **kwargs)
61406625 (leo-naeka 2017-06-05 22:23:17 +0200 324)
b4dffb82 (santiavenda 2018-01-08 18:02:47 -0800 325) def use_pk_only_optimization(self):
b4dffb82 (santiavenda 2018-01-08 18:02:47 -0800 326) return False
b4dffb82 (santiavenda 2018-01-08 18:02:47 -0800 327)
I suppose just having ResourceRelatedField.use_pk_only_optimization() return False as was done for PolymorphicResourceRelatedField would do the trick, but I don't understand exactly when the pk_only optimization should apply and when not.
Maybe it should be False in ResourceRelatedField and add relations.SkipDataMixin.use_ok_only_optimization() to return True.
There was a problem hiding this comment.
I tried the above and test_performance.PerformanceTestCase fails.
|
@sliverc Sorry to distract with the other issue about permissions. Have you had a chance to look this over? |
|
@Anton-Shutik since you "touched it last" (RelatedMixin), perhaps you can also chime in on this PR. |
|
@timcbaoth also since you were near the |
… constraint would allow null (django-json-api#499)
py.test is the old way of running pytest. Also remove DJANGO_SETTINGS_MODULE env when running tests This is already defined in pytest.ini and py.test will take it from as it should also take parameters we might add in pytest.ini in the future.
This way CI doesn't suddently break when a dependency has updated but we still keep up-to-date. pyupio will open a PR with the updated dependencies.
…NAMES (django-json-api#507) Changelog entry was correct so not adding a new one to add only noise.
…uteError exception.
…t-framework-json-api into related_disable_pk_only
|
Ugh. Once again, trying to rebase has totally screwed up my commit history. I'll resubmit as a new PR. |
|
"never rebase anything you’ve pushed somewhere" - https://git-scm.com/book/en/v2/Git-Branching-Rebasing |
Closed
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #489
Description of the Change
This is a workaround that makes the toOne related field view work for my app, but I am quite sure it is not the right solution:
Based on
coveragethere appear to be cases wherefield.use_pk_only_optimization()is not even defined much less returningTrue.Added new models, serializers, etc. to isolate and demonstrate the bug.
Checklist
CHANGELOG.mdAUTHORS