Skip to content
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

Make the API more REST-y #84

Closed
5 tasks done
wetneb opened this issue May 19, 2022 · 7 comments
Closed
5 tasks done

Make the API more REST-y #84

wetneb opened this issue May 19, 2022 · 7 comments
Assignees

Comments

@wetneb
Copy link
Member

wetneb commented May 19, 2022

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
Copy link
Contributor

osma commented Jun 3, 2022

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
Copy link
Member Author

wetneb commented Jun 3, 2022

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
Copy link

stefanw commented Jun 8, 2022

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

  • 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.)
  • 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.
  • Currently, the API seems driven by the UI: it has specific extensions that are closely coupled to the current OpenRefine UI. This puts a lot more work on API implementers than necessary. Instead, I would suggest having more generic endpoints for entities that can optionally provide more infos. E.g. instead of the yet another endpoint to implement (looking at you "flyout entry point" that gives an HTML snippet for a specific UI feature), I could imagine a REST-style resource that can provide that information in a namespaced representation.

@osma
Copy link
Contributor

osma commented Jun 21, 2022

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
Copy link
Member Author

wetneb commented Jun 21, 2022

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): OpenRefine/OpenRefine#2549

@wetneb wetneb self-assigned this Jul 26, 2022
wetneb added a commit that referenced this issue Nov 10, 2022
…st (#91)

* Impose pre-determined routes rather than announcing them in the manifest, for #84

* Remove mention of iframe

* Remove the mention of form elements.

Co-authored-by: Fabian Steeg <steeg@hbz-nrw.de>

Co-authored-by: Fabian Steeg <steeg@hbz-nrw.de>
@wetneb
Copy link
Member Author

wetneb commented Nov 10, 2022

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 added a commit that referenced this issue Jul 13, 2023
…st (#91)

* Impose pre-determined routes rather than announcing them in the manifest, for #84

* Remove mention of iframe

* Remove the mention of form elements.

Co-authored-by: Fabian Steeg <steeg@hbz-nrw.de>

Co-authored-by: Fabian Steeg <steeg@hbz-nrw.de>
@fsteeg
Copy link
Member

fsteeg commented Sep 11, 2024

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

@fsteeg fsteeg closed this as completed Sep 11, 2024
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

No branches or pull requests

4 participants