Disable /learn API method by default#843
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #843 +/- ##
==========================================
+ Coverage 99.63% 99.64% +0.01%
==========================================
Files 99 99
Lines 7350 7361 +11
==========================================
+ Hits 7323 7335 +12
+ Misses 27 26 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
The existing visibility settings only affect the REST API. I think this could also be scoped the same way; the CLI probably shouldn't care about this setting. |
|
There was a problem hiding this comment.
Pull Request Overview
This PR disables the learn functionality by default unless explicitly enabled via the project configuration.
- Adds a configuration parameter "allow_learn" to control the learn functionality.
- Updates tests for REST API, project configuration, and CLI to account for the learn functionality change.
- Adjusts error handling in REST endpoints and the project learning method.
Reviewed Changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/test_rest.py | Adds a test case to ensure the REST learn functionality returns 403 when disabled |
| tests/test_project.py | Updates the expected project count reflecting the new configuration change |
| tests/test_config.py | Updates the expected count of project IDs to match the new project setup |
| tests/test_cli.py | Adjusts commit add counts and completion results for the modified projects |
| annif/rest.py | Introduces a new error response for disabled learning and handles OperationFailedException |
| annif/project.py | Adds a conditional to check the allow_learn flag before invoking learning on the backend |
Files not reviewed (2)
- annif/openapi/annif.yaml: Language not supported
- tests/projects.cfg: Language not supported
| corpus = _documents_to_corpus(body, project.subjects) | ||
| project.learn(corpus) | ||
| except OperationFailedException as err: | ||
| if str(err) == "Learning not enabled for project": |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
osma
left a comment
There was a problem hiding this comment.
I suggested defining and using a new exception type for this case. Otherwise LGTM and can be merged!
|
The learn functionality is rarely used, so it is best to be disabled by default for API requests.
This pull request introduces a new feature that restricts learning operations on projects unless explicitly enabled with
allow_learnproject config parameter.Adds a new
403response in the OpenAPI specification for trying to access the/v1/<project>/learnwhen the project is not configured to allow this.