aio-libs / aio-libs/aiohttp-admin

Bulk update/delete returns incomplete IDs for composite primary keys

Open
#1,192 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
219
Forks
45
Avg merge
4m
Merged PRs (30d)
14

Description

## 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"`.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.