Skip to content

Fix BookListSerializer update example logic to support creation#9990

Open
JPisOP007 wants to merge 1 commit into
encode:mainfrom
JPisOP007:fix/docs-book-list-serializer-update-example
Open

Fix BookListSerializer update example logic to support creation#9990
JPisOP007 wants to merge 1 commit into
encode:mainfrom
JPisOP007:fix/docs-book-list-serializer-update-example

Conversation

@JPisOP007

Copy link
Copy Markdown

Description

This PR fixes a bug in the documentation example for BookListSerializer.update() under the ListSerializer section of the serializers API guide.

Problem

The current documentation example crashes with a KeyError if list items are submitted without an id field. This occurs when trying to perform creations alongside updates via a ListSerializer.

Solution

  1. Added list comprehensions to separate the list items:
    • Filtered data_mapping to only include validated data items containing an 'id' key (updates/deletes).
    • Filtered out new_items that do not contain an 'id' key (creations).
  2. Iterated over new_items and called .create(data) on the child serializer (self.child.create(data)).
  3. Updated the BookSerializer definition in the example to use id = serializers.IntegerField(required=False). This ensures validation succeeds when creating new items that do not yet have an ID.

refs #9469

@ulugbekbackend

Copy link
Copy Markdown

Logic looks correct overall — nice catch on the KeyError that happens when creating list items without an id.
A couple of small suggestions:

Avoid iterating validated_data twice. Right now data_mapping and new_items are built with two separate comprehensions, each looping over the full list. This could be combined into a single loop:

data_mapping = {}
new_items = []
for item in validated_data:
    if 'id' in item:
        data_mapping[item['id']] = item
    else:
        new_items.append(item)

This keeps the same result but only goes through validated_data once.

Is there a docs test covering this example? DRF sometimes has tests that verify code examples in the docs actually run correctly. If one exists for this section, it might need updating too — worth checking.

Otherwise this is a solid fix for a real bug in the documentation. Thanks for working on it!

@ulugbekbackend ulugbekbackend left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Overall logic is solid — good fix for the KeyError on creation without an id. Left a couple of inline notes.

book_mapping = {book.id: book for book in instance}
data_mapping = {item['id']: item for item in validated_data}
data_mapping = {item['id']: item for item in validated_data if 'id' in item}
new_items = [item for item in validated_data if 'id' not in item]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nit: this could be a single loop instead of two separate comprehensions, to avoid iterating validated_data twice — e.g.:

data_mapping = {}
new_items = []
for item in validated_data:
    if 'id' in item:
        data_mapping[item['id']] = item
    else:
        new_items.append(item)

# so use a writable field here, rather than the default which would be read-only.
id = serializers.IntegerField()
# The id is optional so that new items (without an id) can be created.
id = serializers.IntegerField(required=False)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Makes sense given the new create path.

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.

2 participants