-
-
Notifications
You must be signed in to change notification settings - Fork 73
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
It's possible to set an invalid short name when adding content #1487
Comments
Some time ago we faced an issue adding a content with I think we should have one single place in CMFPlone where the id validation is done, and then use that validator here in p.restapi too. |
@erral There is already a way to get a valid id that takes into account those checks, which is an INameChooser adapter on the container. The restapi endpoint for adding content already uses it when the API client did not explicitly specify an id: https://github.com/plone/plone.restapi/blob/master/src/plone/restapi/services/content/utils.py#L93 But, that is getting skipped when the client specified an id: https://github.com/plone/plone.restapi/blob/master/src/plone/restapi/services/content/add.py#L93 It's an easy fix to use the name chooser there too. The question is, will the change in behavior cause any problems? |
@davisagli if I recall correctly we added the id option mainly for migrations, so that a migration algorithm is able to set an ID. You are correct that we should validate this id no matter what. Adding a check for a valid ID on the passed id param is not a breaking change IMO. We are just preventing the system from ending up with an invalid id/path. |
I agree with Timo, I think we should do the same as Classic UI does. If we don't allow spaces in id in Classic we should neither allow that in p.restapi |
+1 for Timo's pov. |
When I see David's PR, I have a doubt. If the id in itself is perfectly valid, say
I have never found the name of this option very intuitive, I always have to read the documentation twice to understand if I should use True or False in my use case. The default should probably be to not fail. But I wonder if it is actually common to pass the id here, or if usually you would only pass a title, and Plone figures out a nice id for you. |
@mauritsvanrees I agree to improve here, but first @davisagli fix solves the immediate problem. We may want to create a new issue for safe_id |
@mauritsvanrees You have a good point that it could be useful to be able to use this endpoint in a mode that throws an error rather than normalizing the id, if a specific id was requested but is not available.
I can think of a few different use cases for passing an id:
In all these cases, I can imagine sometimes it is more helpful to succeed with a modified id and sometimes it is more helpful to get a clear error response, so that the human or code controlling the process can apply a special policy for falling back to something else. That supports the idea of adding an option for it. I'm even leaning a little bit toward returning an error response as the default mode, because then it's more obvious that the requested id isn't being used. I think I'll revise my implementation in #1488 to do that, and wait to add an option for normalizing the id until someone needs it. |
My fix for this was too aggressive, and prevents setting ids that are supposed to be allowed (for example, including capital letters or periods) |
If an
id
is specified while creating content via the FolderPost endpoint, it is used without validating that it meets the criteria for a valid id. For example, it's possible to add an item with an id containing spaces.What I would expect: either the id should be normalized (this is what already happens if it is set while editing the item later), or trying to add an item with an invalid id should return a 400 response with a helpful error message.
Note: This might be considered a backwards incompatible change, since someone calling the API may be assuming that the item has the id that was specified, and not a normalized form of it. But, it could also be considered a bugfix, because it never should have been possible to add items with an invalid id.
Thoughts on how to handle this, @tisto?
The text was updated successfully, but these errors were encountered: