-
Notifications
You must be signed in to change notification settings - Fork 41
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
Add reconciliation API endpoint to REST API #734
base: main
Are you sure you want to change the base?
Conversation
Excellent work!
I opened an issue about this problem: reconciliation-api/specs#139 I will soon provide more detailed comments about the code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the detailed line-by-line comments.
In addition, tests/test_openapi.py is failing on GitHub Actions as well as for me locally. There seems to be some problem with the queries
parameter. Possibly the OpenAPI spec needs to be adjusted somehow.
annif/rest.py
Outdated
@@ -214,3 +215,72 @@ def learn( | |||
return server_error(err) | |||
|
|||
return None, 204 | |||
|
|||
|
|||
def _reconcile(project_id: str, query: dict[str, Any]) -> dict[str, Any]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return type hint dict
doesn't seem to match the actual returned type, which is a list of dicts.
(found by SonarCloud)
|
||
queries = body["queries"] | ||
results = {} | ||
for key, query in queries.items(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a for loop here means that the reconciliation isn't done in batches internally. It would be better (more efficient) to pass the whole batch to _suggest at once, similar to how the suggest_batch method works.
annif/openapi/annif.yaml
Outdated
@@ -314,6 +460,105 @@ components: | |||
type: string | |||
example: Vulpes vulpes | |||
description: A document with attached, known good subjects | |||
ReconcileMetadata: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be better to call this type ReconciliationServiceManifest (although it's pretty long...) so that it matches the reconciliation API spec.
assert "result" in results["q0"] | ||
|
||
|
||
def test_rest_reconcile_nonexistent(app): |
Check warning
Code scanning / CodeQL
Variable defined multiple times Warning test
redefined
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
This PR adds a Reconciliation Service API endpoint to REST API as mentioned in #338. It allows reconciling against a project using the
/v1/projects/{project_id}/reconcile
API method. It also includes a suggest service for entities with the/v1/projects/{project_id}/suggest/entity
API method.The main reconciliation endpoint accepts
GET
andPOST
requests:GET
request with no parameters returns a service manifest.GET
request with a URL query parameterqueries
serialized as JSON returns a reconciliation result batch.POST
request with anapplication/x-www-form-urlencoded
parameterqueries
serialized as JSON returns a reconciliation result batch.Only the fields
query
andlimit
are supported in thequeries
objects. Reconciliation result objects contain the fieldsid
,name
,score
andmatch
. Reconciliation doesn't support types or properties for now.The reconciliation API specification requires the usage of the
identifierSpace
field in the service manifest. Since Annif doesn't require entities to be in a specific URI space and doesn't have a way to extract this information from a project, the field is left empty for now.The suggest service endpoint accepts
GET
requests and returns a suggest response. It supports theprefix
andcursor
parameters.Closes #338