Skip to content

Fixed #36482 -- Rendered PK with value_to_string() in admin. #19600

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

shangxiao
Copy link
Contributor

@shangxiao shangxiao commented Jun 27, 2025

Trac ticket number

ticket-36482

Branch description

This PR demonstrates how to render a PK "correctly", using field.value_to_string(obj), so that get_object() will correctly retrieve value when it uses to_python() to deserialise the PK back to a Python type.

You can test this with the following example:

class Foo(Model):
    bar = DateTimeRangeField(primary_key=True)

Note that there are places that still need updating, like here where the pk is rendered as a string directly passed to quote() inside the template where we cannot use field.value_to_string(obj):

{% url opts|admin_urlname:'history' original.pk|admin_urlquote as history_url %}

Checklist

  • This PR targets the main branch.
  • The commit message is written in past tense, mentions the ticket number, and ends with a period.
  • I have checked the "Has patch" ticket flag in the Trac system.
  • I have added or updated relevant tests.
  • I have added or updated relevant docs, including release notes if applicable.
  • I have attached screenshots in both light and dark modes for any UI changes.

@shangxiao shangxiao marked this pull request as ready for review June 28, 2025 07:47
Comment on lines +687 to +689
@property
def pk_str(self):
return self._meta.pk.value_to_string(self)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know adding this may be contentious but we could use it everywhere else value_to_string() is used - in serialisation & sessions.

Comment on lines 137 to 144
def to_python(self, value):
return json.loads(value)

def value_to_string(self, obj):
return self.value_from_object(obj)
return json.dumps(self.value_from_object(obj))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a separate issue(?) that I noticed while trying to find a suitable way to setup a unit test: JSONField wasn't doing any proper json conversion to/from string values. Perhaps nobody cares 🤷‍♂️

…e_to_string()...

 - JSONField had no to_python() so the base implementation was used.
 - value_to_string() was simply deferring to value_from_object() without
   json processing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant