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

Fix single file uploads in multi-file forms #514

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

pydanny
Copy link
Collaborator

@pydanny pydanny commented Oct 11, 2024


name: Pull Request
about: Propose changes to the codebase
title: '[PR] Fix single file uploads in multi-file forms'
labels: 'bug'
assignees: ''


Related Issue

#513 Uploading a single file on a multiple file field requires a try/except block

Proposed Changes

If the annotation for the response element says it is a GenericAlias (which roughly means 'iterable') but the response element isn't actually iterable, then wrap it in a list.

Types of changes
What types of changes does your code introduce? Put an x in all the boxes that apply:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist
Go over all the following points, and put an x in all the boxes that apply:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I am aware that this is an nbdev project, and I have edited, cleaned, and synced the source notebooks instead of editing .py or .md files directly.

Additional Information
Any additional information, configuration or data that might be necessary to reproduce the issue.

@pydanny pydanny self-assigned this Oct 11, 2024
Copy link

gitnotebooks bot commented Oct 11, 2024

Found 1 changed notebook. Review the changes at https://app.gitnotebooks.com/AnswerDotAI/fasthtml/pull/514

@pydanny pydanny force-pushed the fix-1-file-uploads-multiple-files branch from f2a1fc4 to b997e18 Compare October 11, 2024 10:45
@pydanny pydanny marked this pull request as ready for review October 11, 2024 10:46
@pydanny pydanny requested a review from jph00 October 11, 2024 10:50
@pydanny
Copy link
Collaborator Author

pydanny commented Oct 11, 2024

Hey @jph00, here's a patch for multi-file uploads that comes with a test.

Copy link
Contributor

@jph00 jph00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Danny! Just one suggestion to look at.

fasthtml/core.py Outdated
@@ -191,6 +191,10 @@ async def _find_p(req, arg:str, p:Parameter):
if (res in (empty,None)) and p.default is empty: raise HTTPException(400, f"Missing required field: {arg}")
# If we have a default, return that if we have no value
if res in (empty,None): res = p.default
# if anno is a Generic Alias but res isn't an iterable then wrap in list
if isinstance(anno, GenericAlias):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are parameterised generics that aren't lists? We should probably me more specific about what we're looking for.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jph00 The challenge is that for this error anno is a GenericAlias, which arguably is a bug in its own right. Or maybe we're just not particular enough so this is a missing feature? Looking into that now.

In the meantime, I've changed the try/except to a more precise check. Hopefully that goes away, replaced with a more accurate anno specification.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diving into Fastcore's signature_ex function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm... I think signature_ex is a tangent. Digging into the behavior around params in _wrap_req.

@pydanny pydanny force-pushed the fix-1-file-uploads-multiple-files branch from b997e18 to e254e80 Compare October 14, 2024 11:06
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

Successfully merging this pull request may close these issues.

2 participants