Skip to content

Conversation

marcelklehr
Copy link

See nextcloud/app_api#683 for the corresponding AppAPI PR.

Changes proposed in this pull request:

  • Add a trigger_handler param to set_handlers() that will be called when the /trigger endpoint is requested by AppAPI.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for trigger handlers in the FastAPI integration by introducing a new trigger_handler parameter to the set_handlers() function. This enables handling of task processing trigger events from AppAPI.

  • Adds trigger_handler parameter to set_handlers() function
  • Implements automatic FastAPI endpoint registration for trigger callbacks
  • Provides documentation for the new trigger handler functionality

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@marcelklehr marcelklehr force-pushed the feat/taskprocessing-trigger-handler branch from af3abaa to 254936c Compare October 20, 2025 11:10
@marcelklehr marcelklehr force-pushed the feat/taskprocessing-trigger-handler branch from 254936c to ba1c542 Compare October 20, 2025 11:17
@marcelklehr marcelklehr force-pushed the feat/taskprocessing-trigger-handler branch from ba1c542 to e5ce40a Compare October 20, 2025 11:31
@oleksandr-nc
Copy link
Member

What do you think about passing providerId in the URL path? (e.g. "trigger/{provider_id}")

@marcelklehr
Copy link
Author

What do you think about passing providerId in the URL path? (e.g. "trigger/{provider_id}")

Mh, no opnion, either is good for me, I think. What would be the benefit of having it in the URL path?

@oleksandr-nc
Copy link
Member

oleksandr-nc commented Oct 20, 2025

Linters only complains for two things:

  1. Too long line(125 symbols): :param trigger_handler: callback which will be called for task processing trigger events with the name of the provider.
  2. Snake-case style of variable: providerId

We can fix second option with url path or with adding #pylint ignore or with something like this:

async def trigger_callback(**provider_id: Annotated[str, Body(alias="providerId")]**):

Not sure that this will work for FastAPI for query params - I know that this works for FormMetadata and for JSON inputs well.

I am fine with any type of fix, even with adding pylint ignore.

Other Pylint error that is not related to this PR I already fixed in the main branch

@marcelklehr marcelklehr force-pushed the feat/taskprocessing-trigger-handler branch from e5ce40a to 40c89a9 Compare October 20, 2025 14:04
@marcelklehr marcelklehr force-pushed the feat/taskprocessing-trigger-handler branch from 95b668d to c77caaa Compare October 21, 2025 06:59
Copy link
Member

@oleksandr-nc oleksandr-nc left a comment

Choose a reason for hiding this comment

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

🚀

Copy link

codecov bot commented Oct 21, 2025

Codecov Report

❌ Patch coverage is 0% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.93%. Comparing base (48c9113) to head (b2c157b).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
nc_py_api/ex_app/integration_fastapi.py 0.00% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #382      +/-   ##
==========================================
- Coverage   95.11%   94.93%   -0.18%     
==========================================
  Files          45       45              
  Lines        5361     5371      +10     
==========================================
  Hits         5099     5099              
- Misses        262      272      +10     
Files with missing lines Coverage Δ
nc_py_api/ex_app/integration_fastapi.py 23.63% <0.00%> (-1.53%) ⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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