Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Attributes can be deleted from frozen models #7784

Closed
1 task done
alexmojaki opened this issue Oct 9, 2023 · 3 comments · Fixed by #7800
Closed
1 task done

Attributes can be deleted from frozen models #7784

alexmojaki opened this issue Oct 9, 2023 · 3 comments · Fixed by #7800
Assignees
Labels
bug V2 Bug related to Pydantic V2

Comments

@alexmojaki
Copy link
Contributor

Initial Checks

  • I confirm that I'm using Pydantic V2

Description

While BaseModel.__setattr__ checks self.model_config.get('frozen', None), __delattr__ doesn't. I haven't actually run into this in practice, it was just something I noticed while poking around, but it seems like a simple omission that users wouldn't expect.

I also note that https://docs.pydantic.dev/latest/concepts/models/#faux-immutability says:

Immutability in Python is never strict. If developers are determined/stupid they can always modify a so-called "immutable" object.

but this seems like an easy mistake to make without being determined/stupid. This also means that hashing can break in a confusing way as demonstrated in the example code, which isn't the case in the example in the docs where there's a mutable non-hashable field.

Example Code

from pydantic import BaseModel


class M(BaseModel, frozen=True):
    a: int


m = M(a=1)
s = {m}
print(m in s)  # True

del m.a  # should probably raise an error
print(m in s)  # False
s = {m}
print(m in s)  # True

Python, Pydantic & OS Version

pydantic version: 2.4.2
        pydantic-core version: 2.10.1
          pydantic-core build: profile=release pgo=true
                 install path: /home/alex/work/pydantic/pydantic
               python version: 3.11.5 (main, Sep  9 2023, 21:35:25) [GCC 7.5.0]
                     platform: Linux-5.15.0-86-generic-x86_64-with-glibc2.35
             related packages: typing_extensions-4.7.1 email-validator-2.0.0.post2 pyright-1.1.330.post0 mypy-1.1.1 pydantic-extra-types-2.1.0 pydantic-settings-2.0.3
@alexmojaki alexmojaki added bug V2 Bug related to Pydantic V2 pending Awaiting a response / confirmation labels Oct 9, 2023
@sydney-runkle sydney-runkle removed the pending Awaiting a response / confirmation label Oct 10, 2023
@sydney-runkle sydney-runkle self-assigned this Oct 10, 2023
@sydney-runkle
Copy link
Member

Hi @alexmojaki,

Thanks for filing this issue. I agree - this is problematic behavior and something we should fix 🐛 . I'd be happy to open a PR to address this, but you're also more than welcome to open one (I can review if so). 😄

@alexmojaki
Copy link
Contributor Author

I took a look, and I'm wondering if the error raised should be:

  1. Similar to the errors raised in __setattr__, but:
    1. With different types (e.g. frozen_instance_del instead of frozen_instance) or the same types?
    2. With input being None or some special sentinel representing deletion?
  2. Or just some other exception type?

@sydney-runkle
Copy link
Member

@alexmojaki,

Great questions. I think it'd be great to model the raised error after those raised in __setattr__ in the case of a frozen model. I think frozen_instance should be fine for the type. Regarding the input, I'd lean towards just None.

Thanks 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug V2 Bug related to Pydantic V2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants