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

gracefully handle bulk uploading files to content types without IPrimaryFieldInfo (e.g. *.txt via TUS) #138

Open
djay opened this issue Dec 13, 2023 · 5 comments
Labels
01 type: bug something does not work

Comments

@djay
Copy link
Member

djay commented Dec 13, 2023

Problem

Uploading a new file via contents doesn't handle the case where there is no primary field adapter, like .txt files

Others have encounted this problem
https://community.plone.org/t/objects-can-no-longer-adapt-to-iprimaryfieldinfo/2597/3
https://community.plone.org/t/ttw-dexterity-ct-could-not-adapt-plone-dexterity-content-container-to-interfaces-iprimaryfieldinfo/1562

Steps to reproduce

  1. contents > upload (in volto)
  2. upload .txt file
  3. Get exception

Expected behaviour

  • Probably best idea is to fall back to File if iprimaryfieldinfo doesn't adapt. ie just always allow upload
  • Alternatives
    • ensure IPrimaryFieldInfo works for Page content types
    • give error and ask them to configure the mine type registery
    • ensure all types have a way convert from a file of any kind?
    • very least get rid of the .txt files being uploaded as Page instead of File? When does that make sense?

Actual behaviour

If you upload a .txt file via x you will get the following exception

Traceback (innermost last):
  Module ZPublisher.WSGIPublisher, line 167, in transaction_pubevents
  Module ZPublisher.WSGIPublisher, line 376, in publish_module
  Module ZPublisher.WSGIPublisher, line 271, in publish
  Module ZPublisher.mapply, line 85, in mapply
  Module Products.PDBDebugMode.wsgi_runcall, line 60, in pdb_runcall
  Module plone.restapi.services.content.tus, line 65, in __call__
  Module plone.restapi.services, line 19, in render
  Module plone.restapi.services.content.tus, line 224, in reply
  Module plone.restapi.services.content.tus, line 250, in create_or_modify_content
  Module zope.component.hooks, line 135, in adapter_hook
  Module plone.dexterity.primary, line 23, in __init__
TypeError: ('Could not adapt', <FolderishDocument at document.2023-06-16.3422935871>, <InterfaceClass plone.rfc822.interfaces.IPrimaryFieldInfo>)

This is because text/plain and .txt is linked to Page content types. and Page content types no longer seem to have a primary field that accepts text

due to this code

https://github.com/plone/plone.restapi/blob/d5b83a67b5a894b7985c67e6c2ae0c9eb1034fd6/src/plone/restapi/services/content/tus.py#L249-L255

if not fieldname:
            info = IPrimaryFieldInfo(obj, None)
            if info is not None:
                fieldname = info.fieldname
            elif base_hasattr(obj, "getPrimaryField"):
                field = obj.getPrimaryField()
                fieldname = field.getName()

Alternative would be to give a helpful error that the content type registry needs to adjusted to pick a better content type.

@djay djay added the 01 type: bug something does not work label Dec 13, 2023
@djay djay changed the title gracefully handle bulk uploading files to content types without IPrimaryFieldInfo (e.g. *.txt) gracefully handle bulk uploading files to content types without IPrimaryFieldInfo (e.g. *.txt via TUS) Dec 13, 2023
@plone plone deleted a comment from bajajtushar094 Dec 17, 2023
@djay
Copy link
Member Author

djay commented Dec 17, 2023

First thing is to work out the best solution based on how iprimaryfieldinfo is used in other parts of plone to handle similar problem

@davisagli
Copy link
Member

davisagli commented Jan 13, 2024

@djay Are you trying to create a new content item or add a file to an existing one?

Based on the traceback, it looks like it's following the logic for when mode is set to something other than "create", so it's trying to set the file on the current context. It makes sense that this throws an error if the current context doesn't have a field that's designated as primary.

It sounds like maybe this is a client-side problem where it's passing a different mode than you expect?

@djay
Copy link
Member Author

djay commented Jan 15, 2024

@davisagli creating a new file. I've rewritten it with steps to reproduce to try and make it clearer.
Not sure why you think the path is not for creating. IPrimaryFieldInfo is used when a field is not specified which I think is the case when editing.

My understanding is that Plone has always tried to match the file extension and mimetype to a content type and then used the primaryfield adapter to work out what file field to shove the file data into. I can't remember what the classic version of TUS does if this fails but I suspect it falls back to File also.

I'd rather not a client side solution since there is no restapi to access the mimetype registry and the IprimaryFieldInfo and what would you do if it fails client side? Ask the user? The portal should be configured to do sensible things when uploading anykind of file I think. Working on a user action to convert types is more useful time spent I think.

@davisagli
Copy link
Member

Not sure why you think the path is not for creating.

Indeed, I was jumping to conclusions there. I saw that the object which could not be adapted is a FolderishDocument rather than a File. That could happen if self.context is a Document and mode is something other than "create", but it could also happen if mode is "create" and the content_type_registry returns Document (which sounds like it is the case here).

I'd rather not a client side solution since there is no restapi to access the mimetype registry and the IprimaryFieldInfo and what would you do if it fails client side? Ask the user? The portal should be configured to do sensible things when uploading anykind of file I think.

Yeah I agree with you on that, I was just trying to pinpoint what's going wrong, and was on the wrong track.

This is because text/plain and .txt is linked to Page content types. and Page content types no longer seem to have a primary field that accepts text

Okay yeah, that helps explain it. Before volto, Document had a text field that was primary. With volto and blocks-based pages, it doesn't. plone.volto should probably adjust the configuration of the content_type_registry so that text/plain is mapped to File rather than Document. I'll transfer this issue to plone.volto

@davisagli davisagli transferred this issue from plone/plone.restapi Jan 15, 2024
@davisagli
Copy link
Member

@djay Er, now that I transferred the issue, I'm realizing that it's also a reasonable request for plone.restapi's TUS service to do something better in this case. Maybe fall back to creating a File if the content type found via content_type_registry doesn't have a primary field? But, that would also slow things down by needing to create one item and roll that back before creating another one, so I can see the argument for leaving it as is with an error (but the error message could be improved)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
01 type: bug something does not work
Projects
None yet
Development

No branches or pull requests

2 participants