plone / plone.base

Plone Interface contracts, plus basic features and utilities
https://pypi.org/project/plone.base
2 stars 0 forks source link

Content ID check should be more strict (it allows to break objects) #35

Closed laulaz closed 1 year ago

laulaz commented 1 year ago

It is actually possible to create a content with the same id as some field of its container.

Example (can easily be tested on demo) :

  1. Activate lead image behavior on folders
  2. Create a folder
  3. Create a content with title "image" in this folder

This leads to errors everywhere :

I think we could / should add some code to face this issue ... we added this simple check in our content type class for a project :

def checkValidId(self, id, allow_dup=0):
    if hasattr(self, id) and id not in self.contentIds():
        raise BadRequest()

It is probably easier / safer / better to put it in plone.base (see _check_for_collision) than to put it in Zope ? See OFS.ObjectManager to see where other checks are made.

We can provide a Pull Request if you agree on this.

mauritsvanrees commented 1 year ago

Makes sense to add this in _check_for_collision.

Just asking for clarity: why does your code have an extra check id not in self.contentIds()? Does this code also get called when editing an existing item, and does this then prevent a BadRequest? Makes sense in that case.

Not sure how this works in practice with your fix, but what I would hope:

But if it fails without retrying, I suppose this is still better than what happens now.

laulaz commented 1 year ago

@mauritsvanrees indeed, id not in self.contentIds() was not needed. We had that part because of another issue.

I confirm that the PR makes the ID check work as you hoped :

One issue remains, but I don't think we should / could easily address it : if the "image" content already exists before you activate the LeadImage behavior, it will break after activating the behavior.