aio-libs / aio-libs/aiohttp-admin

Bulk update/delete returns incomplete IDs for composite primary keys

Đang mở
#1,192 1 bình luận 0 reaction 0 người được giao Xem trên GitHub
Ngôn ngữ chính
Python
Star
219
Fork
45
Merge trung bình
4 phút
Pull request đã merge (30 ngày)
14

Mô tả

## Summary

`updateMany` / `deleteMany` return only the **first** column of a composite primary key (e.g. `"1"`) instead of the pipe-joined ID used everywhere else (e.g. `"1|2"`).

This is a regression/gap in the compound-key support from #769, not a new feature request. Single-record endpoints already implement `"col1|col2"` correctly. Bulk endpoints do not.

## Expected

react-admin requires `updateMany` / `deleteMany` to return the same record IDs used by `getList` / `getOne` / `getMany`:

```json
{"data": ["1|2", "1|3"]}
```

That format is the contract from #769 and from `_convert_record()`:

```python
"id": "|".join(str(record[pk]) for pk in self.primary_key)
```

## Actual (reproduced on current `master`)

Against a table with PK `(id, other)` and rows `(1, 2)`, `(1, 3)`, `(4, 5)`:

| Endpoint | Status | IDs |
|---|---|---|
| `GET …/list` | 200 | `["1\|2", "1\|3", "4\|5"]` |
| `GET …/one?id=1\|2` | 200 | `"1\|2"` |
| `GET …/many?ids=["1\|2","1\|3"]` | 200 | `"1\|2"`, `"1\|3"` |
| **`PUT …/update_many?ids=["1\|2","1\|3"]`** | 200 | **`["1", "1"]`** |
| **`DELETE …?ids=["4\|5"]`** | 200 | **`["4"]`** |

The bulk **writes succeed** (incoming `"1|2"` is parsed correctly and the rows are updated/deleted). Only the **response IDs are wrong**. Two different rows both come back as `"1"`, so react-admin’s cache / selection / unselect after a bulk action will desync.

## Why this is a bug, not intentional

1. #769 defined `"first|second"` as the ID format for compound keys and was closed as implemented.
2. List/get-one/get-many already return that format; bulk is inconsistent with the same resource.
3. `_convert_ids()` is documented as converting IDs to the “correct output format”, but it does `str(i)` instead of the same `"|".join(...)` as `_convert_record()`.
4. `SAResource.update_many` / `delete_many` are typed as returning `list[tuple[Any, ...]]` (full PK tuples) but call `Result.scalars()`, which **only yields the first RETURNING column**.

## Root cause

**1. SQLAlchemy backend drops extra PK columns** (`aiohttp_admin/backends/sqlalchemy.py`):

```python
stmt = stmt.values(data).returning(*(self._table.c[pk] for pk in self.primary_key))
return list(await conn.scalars(stmt)) # first column only
```

Same pattern in `delete_many`.

**2. ID encoding does not pipe-join** (`aiohttp_admin/backends/abc.py`):

```python
def _convert_ids(self, ids: Sequence[_ID]) -> tuple[str, ...]:
return tuple(str(i) for i in ids) # "1" or "(1, 2)", never "1|2"
```

Even if (1) returned full tuples, `str((1, 2))` would be `"(1, 2)"`, not `"1|2"`.

## Test gap (no existing coverage)

| Test | What it covers | Composite PK bulk? |
|---|---|---|
| `tests/test_backends_sqlalchemy.py::test_id_nonpk` | Schema only: `primary_key == ("id", "other")` | No HTTP |
| `tests/test_backends_sqlalchemy.py::test_nonid_pk_api` | list/one/many/create/update for a **single** non-`id` PK | No |
| `tests/test_views.py::test_update_many` / `test_delete_many` | `Dummy2Model` integer PK, expects `{"data": ["1", "2"]}` | No |
| `admin-js/tests/relationships.test.js` | **Composite foreign keys**, not composite PKs | No |

That is why this slipped through: compound PK support was implemented and tested for get/create/update-one, never for bulk.

## Suggested fix

1. Return full PK tuples from `RETURNING` (`[tuple(row) for row in await conn.execute(stmt)]`) instead of `scalars()`.
2. Encode with the same helper as records: `"|".join(str(part) for part in i)`.
3. Add an API test for composite PK `update_many` / `delete_many` asserting `{"data": ["1|2", "1|3"]}`.

Single-column PKs stay `"1"` / `"2"` because `(1,)` joins to `"1"`.

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Đánh giá

Issue này chưa được đánh giá.

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.