From 928569eba2ae0f98dfd67f6282b296d245e6d358 Mon Sep 17 00:00:00 2001 From: Humayun Ahmad Date: Tue, 23 Jul 2024 02:55:39 +0500 Subject: [PATCH 1/4] handle zero as valid pk/id in get_resource_id util method - update tests to include cases where ID is zero --- AUTHORS | 1 + CHANGELOG.md | 1 + rest_framework_json_api/utils.py | 10 ++++++---- tests/test_utils.py | 7 +++++++ 4 files changed, 15 insertions(+), 4 deletions(-) diff --git a/AUTHORS b/AUTHORS index 7f4a8237..2f7e4d7f 100644 --- a/AUTHORS +++ b/AUTHORS @@ -52,3 +52,4 @@ Tim Selman Tom Glowka Ulrich Schuster Yaniv Peer +Humayun Ahmad \ No newline at end of file diff --git a/CHANGELOG.md b/CHANGELOG.md index 8c3c7720..b5759b17 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ any parts of the framework not mentioned in the documentation should generally b * Allow overwriting of url field again (regression since 7.0.0) * Ensured that no fields are rendered when sparse fields is set to an empty value. (regression since 7.0.0) +* Handled zero as a valid ID in `get_resource_id` function. ## [7.0.1] - 2024-06-06 diff --git a/rest_framework_json_api/utils.py b/rest_framework_json_api/utils.py index e12080ac..25f78872 100644 --- a/rest_framework_json_api/utils.py +++ b/rest_framework_json_api/utils.py @@ -307,12 +307,14 @@ def get_resource_type_from_serializer(serializer): def get_resource_id(resource_instance, resource): """Returns the resource identifier for a given instance (`id` takes priority over `pk`).""" if resource and "id" in resource: - return resource["id"] and encoding.force_str(resource["id"]) or None + return ( + encoding.force_str(resource["id"]) if resource["id"] is not None else None + ) if resource_instance: return ( - hasattr(resource_instance, "pk") - and encoding.force_str(resource_instance.pk) - or None + encoding.force_str(resource_instance.pk) + if hasattr(resource_instance, "pk") and resource_instance.pk is not None + else None ) return None diff --git a/tests/test_utils.py b/tests/test_utils.py index 4e103ae2..a3beb12e 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -403,6 +403,13 @@ class SerializerWithoutResourceName(serializers.Serializer): (None, {"id": 11}, "11"), (object(), {"pk": 11}, None), (BasicModel(id=6), {"id": 11}, "11"), + (BasicModel(id=0), None, "0"), + (None, {"id": 0}, "0"), + ( + BasicModel(id=0), + {"id": 0}, + "0", + ), ], ) def test_get_resource_id(resource_instance, resource, expected): From 617968191de78e010e9abd667e0a985d3b3f8a67 Mon Sep 17 00:00:00 2001 From: Oliver Sauder Date: Tue, 23 Jul 2024 10:46:06 +0400 Subject: [PATCH 2/4] Avoid double lookup when getting resource id --- rest_framework_json_api/utils.py | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/rest_framework_json_api/utils.py b/rest_framework_json_api/utils.py index 25f78872..805f5f09 100644 --- a/rest_framework_json_api/utils.py +++ b/rest_framework_json_api/utils.py @@ -307,15 +307,11 @@ def get_resource_type_from_serializer(serializer): def get_resource_id(resource_instance, resource): """Returns the resource identifier for a given instance (`id` takes priority over `pk`).""" if resource and "id" in resource: - return ( - encoding.force_str(resource["id"]) if resource["id"] is not None else None - ) + _id = resource["id"] + return encoding.force_str(_id) if _id is not None else None if resource_instance: - return ( - encoding.force_str(resource_instance.pk) - if hasattr(resource_instance, "pk") and resource_instance.pk is not None - else None - ) + pk = getattr(resource_instance, "pk", None) + return encoding.force_str(pk) if pk is not None else None return None From 39fb42c10c313ba6174d505d417943cbfed9c23d Mon Sep 17 00:00:00 2001 From: Oliver Sauder Date: Tue, 23 Jul 2024 10:46:26 +0400 Subject: [PATCH 3/4] Clarified changelog that it is a regression --- CHANGELOG.md | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b5759b17..4fb30d1b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,13 +8,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 Note that in line with [Django REST framework policy](https://www.django-rest-framework.org/topics/release-notes/), any parts of the framework not mentioned in the documentation should generally be considered private API, and may be subject to change. +## [Unreleased] + +### Fixed + +* Handled zero as a valid ID for resource (regression since 6.1.0) + ## [7.0.2] - 2024-06-28 ### Fixed * Allow overwriting of url field again (regression since 7.0.0) * Ensured that no fields are rendered when sparse fields is set to an empty value. (regression since 7.0.0) -* Handled zero as a valid ID in `get_resource_id` function. ## [7.0.1] - 2024-06-06 From 9040f8d184d8b32f4c7fadc0447bad0458d5d727 Mon Sep 17 00:00:00 2001 From: Oliver Sauder Date: Tue, 23 Jul 2024 10:47:39 +0400 Subject: [PATCH 4/4] Sorted AUTHORS file --- AUTHORS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/AUTHORS b/AUTHORS index 2f7e4d7f..d421d5e6 100644 --- a/AUTHORS +++ b/AUTHORS @@ -15,6 +15,7 @@ David Guillot, for Contexte David Vogt Felix Viernickel Greg Aker +Humayun Ahmad Jamie Bliss Jason Housley Jeppe Fihl-Pearson @@ -52,4 +53,3 @@ Tim Selman Tom Glowka Ulrich Schuster Yaniv Peer -Humayun Ahmad \ No newline at end of file