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

Failed to delete an item of OutOfOrderTableProxy #383

Open
ninoseki opened this issue Oct 2, 2024 · 1 comment
Open

Failed to delete an item of OutOfOrderTableProxy #383

ninoseki opened this issue Oct 2, 2024 · 1 comment

Comments

@ninoseki
Copy link

ninoseki commented Oct 2, 2024

Hello, first of all, thanks for creating a great library.

I noticed there is a glitch while dealing with OutOfOrderTableProxy. I cannot delete a key via pop.

import tomlkit

text = """
[tool.poetry]
name = "dummy"
version = "0.0.0"
description = ""
authors = []
readme = "README.md"

[tool.poetry-dynamic-versioning]
enable = true

[[tool.poetry.source]]
name = "foo"
url = ""

[build-system]
requires = ["poetry-core>=1.0.0"]
build-backend = "poetry.core.masonry.api"

[tool.mypy]
ignore_missing_imports = true
"""

doc = tomlkit.parse(text)
tool = doc["tool"]

print(type(tool)) # <class 'tomlkit.container.OutOfOrderTableProxy'>
print(tool.keys()) # KeysView({'poetry': {'source': [{'name': 'foo', 'url': ''}]}, 'poetry-dynamic-versioning': {'enable': True}, 'mypy': {'ignore_missing_imports': True}})

tool.pop("poetry") # tomlkit.exceptions.NonExistentKey: 'Key "poetry" does not exist.'

(Tested with v0.13.2)

Am I misusing something? Or is this a bug in OutOfOrderTableProxy's __delitem__ method?

@qris
Copy link

qris commented Jan 31, 2025

I think this may be the same bug:

import tomlkit
del tomlkit.loads("[tool.pdm.scripts]\n[tool.pdm]\n[dependency-groups]\n[tool.ruff]\n")["tool"]["pdm"]'

Outputs:

tomlkit.exceptions.NonExistentKey: 'Key "pdm" does not exist.'

Deleting other keys works fine.

I traced the problem to here, in OutOfOrderTableProxy.__init__:

    def __init__(self, container: Container, indices: tuple[int, ...]) -> None:
        self._container = container
        self._internal_container = Container(True)
        self._tables = []
        self._tables_map = {}

        for i in indices:
            _, item = self._container._body[i]

            if isinstance(item, Table):
                self._tables.append(item)
                table_idx = len(self._tables) - 1
                for k, v in item.value.body:
                    self._internal_container._raw_append(k, v)
                    self._tables_map.setdefault(k, []).append(table_idx)
                    if k is not None:
                        dict.__setitem__(self, k.key, v)

In this case, item.value.body contains two keys called pdm (I think maybe there should only be one?)

[(<Key pdm>, {'scripts': {}}), (<Key pdm>, {})]

So we go around the for k, v in item.value.body: loop twice, adding the same table_idx (zero) twice to self._tables_map[k].- Then in __del__ we try to delete the same key twice, but it fails the second time because it's already been deleted.

I made a small change to __init__ to avoid adding the same key twice:

                    key_to_index = self._tables_map.setdefault(k, [])
                    if table_idx not in key_to_index:
                        key_to_index.append(table_idx)
                    if k is not None:
                        dict.__setitem__(self, k.key, v)

And it appears to work, for this case and a few other simple sanity checks:

python3.12 -c 'import tomlkit
with open("pyproject.toml", "rb") as f: k=tomlkit.load(f); del k["tool"]["pdm"]; print(tomlkit.dumps(k))'
[dependency-groups]
[tool.ruff]

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

No branches or pull requests

2 participants