Issue with updating and bulk updating `BaseUser` via admin panel

[jrycw]

This issue arose while attempting to update or bulk update the Piccolo BaseUser via the admin panel, while following the repository headless-forum-fastapi.

Currently, the implementation of piccolo_api/crud/endpoints.py/patch_single works correctly when an admin user wants to change the password. However, it encounters issues when attempting to update or bulk update a Piccolo BaseUser.

After discussing with @sinisaos here, we devised a working solution.

@apply_validators
@db_exception_handler
async def patch_single(
    self, request: Request, row_id: PK_TYPES, data: t.Dict[str, t.Any]
) -> Response:
    """
    Patch a single row.
    """
    cleaned_data = self._clean_data(data)

    try:
        model = self.pydantic_model_optional(**cleaned_data)
    except pydantic.ValidationError as exception:
        return Response(str(exception), status_code=400)

    cls = self.table

    try:
        values = {getattr(cls, key): getattr(model, key) for key in data.keys()}
    except AttributeError:
        unrecognised_keys = set(data.keys()) - set(model.model_dump().keys())
        return Response(
            f"Unrecognised keys - {unrecognised_keys}.",
            status_code=400,
        )

    if self._hook_map:
        values = await execute_patch_hooks(
            hooks=self._hook_map,
            hook_type=HookType.pre_patch,
            row_id=row_id,
            values=values,
            request=request,
        )
    # ==========================================
    # method 1 from @jrycw 
    if issubclass(cls, BaseUser):
        if password := values.pop("password", None):
            cls._validate_password(password)
            values["password"] = cls.hash_password(password)

    # method 2 from @sinisaos 
    if issubclass(cls, BaseUser):
        try:
            # if password is changed
            if values["password"]:
                cls._validate_password(values["password"])
                values["password"] = cls.hash_password(values["password"])
            else:
                # use previous password
                values.pop("password")
        except KeyError:
            pass
    # ==========================================
    try:
        await cls.update(values).where(cls._meta.primary_key == row_id).run()
        new_row = (
            await cls.select(exclude_secrets=self.exclude_secrets)
            .where(cls._meta.primary_key == row_id)
            .first()
            .run()
        )
        assert new_row
        return CustomJSONResponse(self.pydantic_model(**new_row).model_dump_json())
    except ValueError:
        return Response("Unable to save the resource.", status_code=500)

However, this solution has raised another concern: what happens when we bulk update a field that is subject to a unique constraint? It appears that while we can update the first row successfully, subsequent rows fail. We're unsure if this is the correct behavior, or if there should be logic in place to prevent bulk updates when using the admin panel.

[dantownsend]

Interesting. I haven't tried bulk updating the BaseUser table. I'll give it a go.

[dantownsend]

I just gave it a go, and I can see what you mean. A 500 error was being raised.

I've tried out your suggested fix, and it all seems to work great.

[dantownsend]

Do you mind creating a PR with the changes?

I'm happy with either method. Lets go with this as it's a bit shorter:

    if issubclass(cls, BaseUser):
        if password := values.pop("password", None):
            cls._validate_password(password)
            values["password"] = cls.hash_password(password)

I noticed that we don't handle the potential exception raised by cls._validate_password(password). For example, if the password is too short. It would be nice to return a 400 error.

[jrycw]

Do you mind creating a PR with the changes?

I'm happy with either method. Lets go with this as it's a bit shorter:

    if issubclass(cls, BaseUser):
        if password := values.pop("password", None):
            cls._validate_password(password)
            values["password"] = cls.hash_password(password)

I noticed that we don't handle the potential exception raised by cls._validate_password(password). For example, if the password is too short. It would be nice to return a 400 error.

@dantownsend Absolutely, I'm more than willing to submit a PR. However, if we attempt to handle the exception as follows:

        if issubclass(cls, BaseUser):
            if password := values.pop("password", None):
                try:
                    cls._validate_password(password)
                except ValueError as e:
                    return Response(f"{e}", status_code=400)
                values["password"] = cls.hash_password(password)

it seems that test_patch_user_fails won't pass, as we're handling the exception and returning a response instead of raising it.

    def test_patch_user_fails(self):
        ...
        with self.assertRaises(ValueError):
            response = client.patch(f"/{user['id']}/", json=json)
>           self.assertEqual(response.content, b"The password is too short.")
E           AssertionError: ValueError not raised

[jrycw]

I just gave it a go, and I can see what you mean. A 500 error was being raised.

I've tried out your suggested fix, and it all seems to work great.

The fix should ensure that patch_single behaves correctly and addresses potential issues with updating and bulk updating (for fields not subject to unique constraints) using the admin panel.

However, when bulk updating fields subject to unique constraints (across any tables, not just BaseUser) via the admin panel, the first row is successfully updated, but subsequent rows fail. This behavior shouldn't be attributed to patch_single, as it functions correctly. One potential solution is to filter out fields subject to unique constraints in advance within the admin panel. However, it seems this issue is related to piccolo_admin and may require further investigation.

[sinisaos]

However, when bulk updating fields subject to unique constraints (across any tables, not just BaseUser) via the admin panel, the first row is successfully updated, but subsequent rows fail. This behavior shouldn't be attributed to patch_single, as it functions correctly. One potential solution is to filter out fields subject to unique constraints in advance within the admin panel. However, it seems this issue is related to piccolo_admin and may require further investigation.

This can be solved by adding a unique argument to an column extra in pydantic schema like this

# in piccolo/utils/pydantic.py
extra: JsonDict = {
    "help_text": column._meta.help_text,
    "choices": column._meta.get_choices_dict(),
    "secret": column._meta.secret,
    "nullable": column._meta.null,
    "unique": column._meta.unique, # new argument
}

After that we can simply not show unique column in Bulkupdate dropdown like this

// in BulkUpdateModal.vue
<template v-for="(property, columnName) in schema.properties">
    <option
        :key="columnName"
        :value="columnName"
        v-if="
            columnName != schema.extra.primary_key_name &&
            property.extra.unique == false // check if column is unique
        "
    >
        {{ property.title }}
    </option>
</template>

This will disable the attempt to update in bulk unique columns. Any thoughts?

[jrycw]

However, when bulk updating fields subject to unique constraints (across any tables, not just BaseUser) via the admin panel, the first row is successfully updated, but subsequent rows fail. This behavior shouldn't be attributed to patch_single, as it functions correctly. One potential solution is to filter out fields subject to unique constraints in advance within the admin panel. However, it seems this issue is related to piccolo_admin and may require further investigation.

This can be solved by adding a unique argument to an column extra in pydantic schema like this

# in piccolo/utils/pydantic.py
extra: JsonDict = {
    "help_text": column._meta.help_text,
    "choices": column._meta.get_choices_dict(),
    "secret": column._meta.secret,
    "nullable": column._meta.null,
    "unique": column._meta.unique, # new argument
}

After that we can simply not show unique column in Bulkupdate dropdown like this

// in BulkUpdateModal.vue
<template v-for="(property, columnName) in schema.properties">
    <option
        :key="columnName"
        :value="columnName"
        v-if="
            columnName != schema.extra.primary_key_name &&
            property.extra.unique == false // check if column is unique
        "
    >
        {{ property.title }}
    </option>
</template>

This will disable the attempt to update in bulk unique columns. Any thoughts?

@sinisaos I consider this approach to be beneficial because by filtering the fields subjected to unique constraints in advance while bulk updating, we can offer the best user experience for those using the admin panel. If other users are dissatisfied with this approach, they still have the option to build the interface they desire using piccolo_api directly.

[sinisaos]

@jrycw I also think it's an easy way (hiding the unique column field) to prevent admin errors. Bulk updating unique columns doesn't make sense because it breaks unique constraints (we have that case now because we can only update the first row, but already the next row raise error because unique constraints are violated), but we'll have to wait and see what Daniel says about all this.

[dantownsend]

I agree that filtering out unique columns from the bulk update dropdown makes a lot of sense. The solution that @sinisaos put forward looks good.

@dantownsend Absolutely, I'm more than willing to submit a PR. However, if we attempt to handle the exception as follows:

    if issubclass(cls, BaseUser):
        if password := values.pop("password", None):
            try:
                cls._validate_password(password)
            except ValueError as e:
                return Response(f"{e}", status_code=400)
            values["password"] = cls.hash_password(password)

it seems that test_patch_user_fails won't pass, as we're handling the exception and returning a response instead of raising it.

def test_patch_user_fails(self):
    ...
    with self.assertRaises(ValueError):
        response = client.patch(f"/{user['id']}/", json=json)

      self.assertEqual(response.content, b"The password is too short.")

E AssertionError: ValueError not raised

Your changes look good. Can we refactor the test to remove the with self.assertRaises(ValueError):?

[jrycw]

If I understand correctly, instead of raising a ValueError, we're considering refactoring the test to expect a 400 response if "the password is too short".

The test_patch_user_fails would then look like this:

def test_patch_user_fails(self):
    client = TestClient(PiccoloCRUD(table=BaseUser, read_only=False))

    json = {
        "username": "John",
        "password": "John123",
        "email": "john@test.com",
        "active": False,
        "admin": False,
        "superuser": False,
    }

    response = client.post("/", json=json)
    self.assertEqual(response.status_code, 201)

    user = BaseUser.select().first().run_sync()

    json = {
        "email": "john@test.com",
        "password": "1",
        "active": True,
        "admin": True,
        "superuser": False,
    }

    response = client.patch(f"/{user['id']}/", json=json)
    self.assertEqual(response.status_code, 400)
    self.assertEqual(response.content, b"The password is too short.")

@dantownsend If the changes to the code and test suit your expectations, I'll proceed with submitting a PR.

[dantownsend]

@jrycw Yes, that looks good to me - thanks!

[sinisaos]

@dantownsend If you want, I can do a unique column PR for Piccolo and Piccolo Admin in the evening?

[dantownsend]

@sinisaos That would be good, thanks!

[sinisaos]

@jrycw Can you please close this issue as all bugs from this issue are now resolved. Thanks.

[jrycw]

@sinisaos Sure, thank you once again for your assistance!