reconciliation-api / specs

Specifications of the reconciliation API
https://reconciliation-api.github.io/specs/draft/
31 stars 9 forks source link

Make the API more REST-y #84

Closed wetneb closed 3 weeks ago

wetneb commented 2 years ago

I just had a very nice conversation with @stefanw who told me about his experience of implementing the reconciliation API for FragDenStaat.de. They use the Django REST Framework and he tried to implement the protocol in this context. You can see the source code here: https://github.com/okfde/froide/blob/main/froide/helper/api_utils.py#L104

One general issue that he has encountered is that the API does not really follow the REST principles. We depart from those principles in the following ways (on top of my head), with the checkbox indicating if the problem has been fixed already:

and perhaps other things I have missed?

This is not the first time I have heard this, so I think it would make sense to fix those problems, even though they induce breaking changes. Versioning should make it possible for clients such as OpenRefine to keep supporting the old API and the new one.

osma commented 2 years ago

I'm all for improving the API, but...if we did big changes like this in the spec, could we except OpenRefine to implement support for the modernized API soon-ish?

I'm thinking about this mostly from the perspective of someone who wants to implement an API endpoint (or several). Fixing these warts in the spec would certainly make implementation easier, but if OpenRefine won't support the new API spec, then what's the point - and should you just implement the old spec instead even though it's uglier and more difficult?

wetneb commented 2 years ago

Yes, I think it would be important that OpenRefine gets updated quickly to support the new version too. I also do not expect many services to adopt the new specs until that is done, since it remains the main client for this API as far as I can see.

Although I cannot commit to a specific time-frame yet, I would generally be keen to work on this myself, or helping others to do it. In any case, I would definitely expect that there is a period during which the REST-ified API has been already published but the latest stable version of OpenRefine does not support it yet - I would like to make this period short, but I think it is also fine if it lasts a few months. Or do you think this is not acceptable?

stefanw commented 2 years ago

Since I got tagged, here are my two cents as an implementer of the API:

osma commented 2 years ago

Thanks @stefanw for your input, it's very valuable to have actual, recent implementation experience!

I would have gladly used any kind of library that could have made the implementation easier. Something where I just need to implement some methods and it does the rest for me. Writing the parsing for the API requests was cumbersome, had lots of difficult to debug errors and took a long time. (I hear it's gotten better since then with more tooling.)

From my perspective, an OpenAPI spec (see #17) together with a toolkit such as Connexion would enable exactly this - just implement the necessary methods, the framework handles the rest. At least in an ideal world - in practice, you'd probably need to debug both the spec, the framework and your own code ;)

Reconciliation seems slow to me and I think it's because it's serialized and non-batched. Reconciling batches of records at once could lower the overhead per HTTP request. Most backends are probably fast enough to handle reconciliation of hundreds of records in one HTTP request. This would probably not be very RESTy, but maybe that's OK.

The current spec already has reconciliation query batches, was there some problem implementing or using this facility?

wetneb commented 2 years ago

The current spec already has reconciliation query batches, was there some problem implementing or using this facility?

I guess the main hurdle for a lot of users is that OpenRefine always uses batches of 10 queries (the batch size is not configurable yet): https://github.com/OpenRefine/OpenRefine/issues/2549

wetneb commented 1 year ago

In today's call @awagner-mainz asked whether making our JSON payloads be JSON-LD -compatible (#70) was also part of this effort. Personally I need to better understand what it would imply and enable, it is still a bit unclear to me. If there are JSON-LD enthusiasts around, perhaps they could show us the way?

fsteeg commented 3 weeks ago

This seems done, closing: the items in the original comment are all checked, #91 & #92 are merged.