Skip to content

Commit f5e02bb

Browse files
authored
fix: add was_attribute_set() guard to relationship loop in update() (#685)
fix: add was_attribute_set() guard to relationship loop in update() Partial updates using model instances (e.g. Model(id=..., name='new')) were silently clearing relationships because SQLAlchemy auto-initializes unset relationship attributes to None/[], which then passed the 'is not MISSING' check and got written through to the DB. This adds the same was_attribute_set() guard that already protects the column loop to the relationship loop, so only explicitly set relationships are copied during update(). Closes #684
1 parent ca80f58 commit f5e02bb

3 files changed

Lines changed: 163 additions & 0 deletions

File tree

advanced_alchemy/repository/_async.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2276,6 +2276,12 @@ async def update(
22762276
# Skip relationships with incompatible lazy loading strategies
22772277
continue
22782278

2279+
# Only copy relationships that were explicitly set on the input instance
2280+
# This prevents overwriting existing relationships with uninitialized
2281+
# None/[] values from SQLAlchemy's auto-initialization
2282+
if not was_attribute_set(data, mapper, relationship.key):
2283+
continue
2284+
22792285
if (new_value := getattr(data, relationship.key, MISSING)) is not MISSING:
22802286
# Skip relationships that cannot be handled by generic merge operations
22812287
if isinstance(new_value, list):

advanced_alchemy/repository/_sync.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2271,6 +2271,12 @@ def update(
22712271
# Skip relationships with incompatible lazy loading strategies
22722272
continue
22732273

2274+
# Only copy relationships that were explicitly set on the input instance
2275+
# This prevents overwriting existing relationships with uninitialized
2276+
# None/[] values from SQLAlchemy's auto-initialization
2277+
if not was_attribute_set(data, mapper, relationship.key):
2278+
continue
2279+
22742280
if (new_value := getattr(data, relationship.key, MISSING)) is not MISSING:
22752281
# Skip relationships that cannot be handled by generic merge operations
22762282
if isinstance(new_value, list):

tests/integration/test_repository.py

Lines changed: 151 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1387,3 +1387,154 @@ async def test_composite_pk_upsert_many_all_new(
13871387
for user_id, role_id in [(100, 100), (101, 101)]:
13881388
created = await maybe_async(user_role_repo.get((user_id, role_id)))
13891389
assert created is not None
1390+
1391+
1392+
async def test_repo_update_partial_does_not_clear_relationships_github_684(
1393+
seeded_test_session_async: "tuple[AsyncSession, dict[str, type]]",
1394+
) -> None:
1395+
"""Test that partial update with a model instance does not clear relationships (GitHub Issue #684).
1396+
1397+
When updating with a model instance where only scalar fields are set
1398+
(e.g., Author(id=..., name="New Name")), the unset relationship fields
1399+
should NOT overwrite existing relationships with None/[].
1400+
1401+
Regression test for: https://github.com/litestar-org/advanced-alchemy/issues/684
1402+
"""
1403+
session, models = seeded_test_session_async
1404+
author_model = models["author"]
1405+
book_model = models["book"]
1406+
author_repo = create_repository(session, author_model)
1407+
book_repo = create_repository(session, book_model)
1408+
1409+
# Create an author with a book using the relationship (so ORM tracks it)
1410+
author = author_model(name="Alice", dob=datetime.date(1980, 1, 1))
1411+
book = book_model(title="Great Book")
1412+
author.books = [book]
1413+
author = await maybe_async(author_repo.add(author))
1414+
author_id = author.id
1415+
await session.flush()
1416+
1417+
# Expire the session so the next get() does a fresh load with selectin
1418+
session.expire_all()
1419+
1420+
# Verify the relationship is set
1421+
fetched_author = await maybe_async(author_repo.get(author_id))
1422+
assert fetched_author.books is not None
1423+
assert len(fetched_author.books) == 1
1424+
book_id = fetched_author.books[0].id
1425+
1426+
# Expire again to clear the identity map before the update
1427+
session.expire_all()
1428+
1429+
# Partial update: only change name, do NOT touch the books relationship
1430+
partial_update = author_model(id=author_id, name="Bob")
1431+
updated_author = await maybe_async(author_repo.update(partial_update))
1432+
1433+
# Verify: name was updated
1434+
assert updated_author.name == "Bob"
1435+
1436+
# Expire and re-fetch to verify DB state
1437+
session.expire_all()
1438+
refetched = await maybe_async(author_repo.get(author_id))
1439+
assert refetched.books is not None, "BUG: books relationship was silently cleared during partial update"
1440+
assert len(refetched.books) == 1, "BUG: books relationship was silently cleared during partial update"
1441+
assert refetched.books[0].id == book_id
1442+
1443+
# Also verify the book still exists and is associated
1444+
refetched_book = await maybe_async(book_repo.get(book_id))
1445+
assert refetched_book.author_id == author_id, "BUG: book's author_id was cleared"
1446+
1447+
1448+
async def test_repo_update_partial_does_not_crash_non_nullable_fk_github_684(
1449+
seeded_test_session_async: "tuple[AsyncSession, dict[str, type]]",
1450+
) -> None:
1451+
"""Test that partial update on parent doesn't cause IntegrityError for non-nullable FK children (GitHub #684).
1452+
1453+
When a child has a non-nullable FK to a parent, partially updating the parent
1454+
(without touching the relationship) must not attempt to set the relationship to None,
1455+
which would violate the NOT NULL constraint.
1456+
1457+
Regression test for: https://github.com/litestar-org/advanced-alchemy/issues/684
1458+
"""
1459+
session, models = seeded_test_session_async
1460+
author_model = models["author"]
1461+
book_model = models["book"]
1462+
author_repo = create_repository(session, author_model)
1463+
book_repo = create_repository(session, book_model)
1464+
1465+
# Create an author with a book (book.author_id is non-nullable)
1466+
author = author_model(name="Charlie", dob=datetime.date(1990, 5, 5))
1467+
book = book_model(title="Non-Nullable FK Book")
1468+
author.books = [book]
1469+
author = await maybe_async(author_repo.add(author))
1470+
author_id = author.id
1471+
await session.flush()
1472+
session.expire_all()
1473+
1474+
# Fetch the book to get its ID
1475+
fetched_author = await maybe_async(author_repo.get(author_id))
1476+
assert len(fetched_author.books) == 1
1477+
book_id = fetched_author.books[0].id
1478+
session.expire_all()
1479+
1480+
# Partial update of the book: only change title, don't touch author relationship
1481+
# The book's `author` relationship (with non-nullable FK) should not be cleared
1482+
partial_book = book_model(id=book_id, title="Updated Title", author_id=author_id)
1483+
# This should NOT raise IntegrityError
1484+
updated_book = await maybe_async(book_repo.update(partial_book))
1485+
1486+
assert updated_book.title == "Updated Title"
1487+
assert updated_book.author_id == author_id
1488+
1489+
1490+
async def test_repo_update_explicit_relationship_still_works_github_684(
1491+
seeded_test_session_async: "tuple[AsyncSession, dict[str, type]]",
1492+
) -> None:
1493+
"""Test that explicitly setting a relationship during update still works correctly (GitHub #684).
1494+
1495+
The fix for #684 should only skip relationships that were NOT explicitly set.
1496+
When a relationship IS explicitly set, it should still be updated normally.
1497+
1498+
Regression test for: https://github.com/litestar-org/advanced-alchemy/issues/684
1499+
"""
1500+
session, models = seeded_test_session_async
1501+
author_model = models["author"]
1502+
book_model = models["book"]
1503+
author_repo = create_repository(session, author_model)
1504+
book_repo = create_repository(session, book_model)
1505+
1506+
# Create two authors
1507+
author1 = await maybe_async(author_repo.add(author_model(name="Author1", dob=datetime.date(1980, 1, 1))))
1508+
author2 = await maybe_async(author_repo.add(author_model(name="Author2", dob=datetime.date(1985, 2, 2))))
1509+
await session.flush()
1510+
1511+
# Save IDs before any expire_all() calls to avoid MissingGreenlet
1512+
author1_id = author1.id
1513+
author2_id = author2.id
1514+
1515+
# Create a book linked to author1 via the relationship
1516+
book = book_model(title="Transferable Book")
1517+
book.author = author1
1518+
book = await maybe_async(book_repo.add(book))
1519+
book_id = book.id
1520+
await session.flush()
1521+
session.expire_all()
1522+
1523+
# Verify initial state
1524+
fetched_book = await maybe_async(book_repo.get(book_id))
1525+
assert fetched_book.author_id == author1_id
1526+
session.expire_all()
1527+
1528+
# Explicitly update the book's author_id FK column to point to author2
1529+
# This verifies that explicitly set FK columns (which back relationships)
1530+
# are still properly applied during update
1531+
update_book = book_model(id=book_id, title="Transferred Book", author_id=author2_id)
1532+
updated_book = await maybe_async(book_repo.update(update_book))
1533+
1534+
assert updated_book.title == "Transferred Book"
1535+
assert updated_book.author_id == author2_id
1536+
1537+
# Verify in DB
1538+
session.expire_all()
1539+
refetched = await maybe_async(book_repo.get(book_id))
1540+
assert refetched.author_id == author2_id

0 commit comments

Comments
 (0)