Skip to content

Add PUT request support at /api/jobs/{job_id}/files to FastAPIJobFiles #20353

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

Draft
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

kysrpex
Copy link
Contributor

@kysrpex kysrpex commented May 26, 2025

⚠️ This PR requires #20235, it will remain as a draft until the latter is merged. Have a look at 47f9617 to see a diff showing what truly changed. This is one of the tweaks that are needed for the development of an integration of Galaxy with ARC (Advanced Resource Connector) as a Pulsar job runner.

This path already supports GET, HEAD and POST requests; add support for PUT requests. There is a significant difference in behavior between POST and PUT requests:

  • POST requests take path and job_key both as query parameters or as body parameters belonging to a multipart request. PUT requests take them only as query parameters (just like GET and HEAD).
  • POST requests submit a file as one of the fields of the multipart request, whereas the submitted file is the whole body of the request for PUT requests.
  • POST requests can append to the tool_stdout and tool_stderr, PUT requests can only create new files or overwrite whole files.
  • POST requests support resumable uploads but PUT requests do not.
  • POST requests take the form parameters __file_path (path of a file uploaded via the nginx upload module) and __file but PUT requests do not.

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

kysrpex added 5 commits May 15, 2025 15:42
`FastAPIJobFiles` is the new, FastAPI version of `JobFilesAPIController`. The endpoints that have been migrated should exhibit exactly the same behavior as the old ones from `FastAPIJobFiles`. Something to keep in mind is that while FastAPI has some extra built-in features that the legacy WSGI system did not have, such as answering HEAD requests, those do not work because of the way legacy WSGI endpoints are injected into the FastAPI app (using `app.mount("/", wsgi_handler)`), meaning that for example, HEAD requests are passed to the `wsgi_handler` sub-application.

Endpoints dedicated to TUS uploads work in tandem with the WSGI middleware `TusMiddleware` from the `tuswsgi` package. As explained above, WSGI middlewares and endpoints are injected into the FastAPI app after FastAPI routes as a single sub-application `wsgi_handler` using `app.mount("/", wsgi_handler)`, meaning that requests are passed to the `wsgi_handler` sub-application (and thus to `TusMiddleware`) only if there was no FastAPI endpoint defined to handle them. Therefore, they cannot be migrated to FastAPI unless `TusMiddleware` is also migrated to ASGI.
Work around a bug in FastAPI (fastapi/fastapi#13175) that assigns the same operation id to both request methods GET and HEAD of the endpoint `/api/jobs/{job_id}/files` when using the `@router.api_route()` decorator with `methods=["GET", "HEAD"]` as keyword argument.
…T requests to `/api/jobs/{job_id}/files`

Pulsar formats the `path` and `job_key` parameters as query parameters when submitting POST requests to `/api/jobs/{job_id}/files`. However, many Galaxy tests format them as form parameters. The only way to keep the endpoint working as it should (as it worked before the migration to FastAPI) is to accept both query and form parameters.
… requests to `/api/jobs/{job_id}/files`

FastAPI will not use the parameter aliases of form parameters in the OpenAPI docs, but the name of their Python variables. Therefore, the API docs show `path_form` and `job_key_form`. Rename them so that the API docs show the correct parameter names.
…iles`

This path already supports GET, HEAD and POST requests; add support for PUT requests. There is a significant difference in behavior between POST and PUT requests:
- POST requests take `path` and `job_key` both as query parameters or as body parameters belonging to a multipart request. PUT requests take them only as query parameters (just like GET and HEAD).
- POST requests submit a file as one of the fields of the multipart request, whereas the submitted file is the whole body of the request for PUT requests.
- POST requests can append to the `tool_stdout` and `tool_stderr`, PUT requests can only create new files or overwrite whole files.
- POST requests support resumable uploads but PUT requests do not.
- POST requests take the form parameters `__file_path` (path of a file uploaded via the nginx upload module) and `__file` but PUT requests do not.
"content": {"application/json": None, "application/octet-stream": {"example": None}},
},
400: {
"description": (

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
.
"File not found, path does not refer to a file, or input dataset(s) for job have been purged."
)
},
},

Check failure

Code scanning / CodeQL

Polynomial regular expression used on uncontrolled data High

This
regular expression
that depends on a
user-provided value
may run slow on strings starting with 'dataset_' and with many repetitions of 'dataset_a'.
job = self.__authorize_job_access(trans, job_id, path=path, job_key=job_key)
self.__check_job_can_write_to_path(trans, job, path)

destination_file_exists = os.path.exists(path)

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
.
job = self.__authorize_job_access(trans, job_id, path=path, job_key=job_key)
self.__check_job_can_write_to_path(trans, job, path)

destination_file_exists = os.path.exists(path)

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
.
self.__check_job_can_write_to_path(trans, job, path)

destination_file_exists = os.path.exists(path)

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
.
destination_file_exists = os.path.exists(path)

# FastAPI can only read the file contents from the request body in an async context. To write the file without
# using an async endpoint, the async code that reads the file from the body and writes it to disk will have to

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
.
This path depends on a
user-provided value
.
# `destination_file.write(chunk)`, it has to run on its own event loop within the thread spawned to answer the
# request to the sync endpoint.
async def write():
with open(path, "wb") as destination_file:

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
.

target_dir = os.path.dirname(path)
util.safe_makedirs(target_dir)
event_loop = asyncio.new_event_loop()

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
.
This path depends on a
user-provided value
.

target_dir = os.path.dirname(path)
util.safe_makedirs(target_dir)
event_loop = asyncio.new_event_loop()

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
.
util.safe_makedirs(target_dir)
event_loop = asyncio.new_event_loop()
try:
asyncio.set_event_loop(event_loop)

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
.
@kysrpex
Copy link
Contributor Author

kysrpex commented May 27, 2025

@maikenp FYI

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

Successfully merging this pull request may close these issues.

1 participant