Api rework

[Leptopoda]

implements #1421 and superseds #1445

this covers all Problems I faced when trying to implement a generated API.

---

If anyone else uses code generation and faces Issues with the now renamed objects (Category, Keyword or Url) many languages (if not all) support typedefs so you could use something simple like:

typedef MyOldBadlyNamedClass = TheGreatNewNamedClass;

although I think a simple crtl+f will do the Trick :)

The api itself does not cahnge jsut it's documentation does. So there isn't any need to update your client in the first place

---

The commits are very self explanatory but feel free to ask anything you don't understand.

[github-actions]

Test Results

     21 files     952 suites   5m 22s ⏱️
   496 tests    496 ✔️ 0 💤 0 ❌
3 472 runs  3 471 ✔️ 1 💤 0 ❌

Results for commit 66b2319.

♻️ This comment has been updated with latest results.

[Leptopoda]

Marking as draft as I've been in contact with the dev working on the openapi documentation within nextcloud.
They are working on a script that will generate the spec from the code so thing like #1464 wouldn't even happen.

The script isn't even public yet and the current cookbook code might need some cleanup to be generated into a nice looking spec.
Again this will not change anything in the api, nor would it break anything just the documentation for it would be auto generated.

Some related information can be found here: nextcloud/neon#189

[christianlupus]

I've been in contact with the dev working on the openapi documentation within nextcloud

I am not sure who you are referring to. Maybe you can ping him/her or post a link to the discussion?

They are working on a script that will generate the spec from the code

I honestly doubt that this will work for any non-trivial example. At least with the cookbook app, I see no chance. The JSON is generated from a file on the cloud. The data is not generated on the fly.

This is also the problem we are facing there: Older versions of the cookbook did work slightly differently. We have to check all these inconsistencies on each invocation.
Example: The timing was stored as a string as it was given during import. Later it became problematic as it needed to be parsable. So, we added the parsing logic to the import. However, the older recipes are still not fixed. These will still break the UI/API. There is an idea to store the recipes with minimal modifications (the raw file so to call) and use a set of filters/mappings/... to extract a clean representation from that (#1110). Then, it might be possible to predict the content schemas of the returned values.

In general, I am a friend of defining an API schema and implementing everything in accordance with that. The cookbook had an implementation already, so this was not possible. The OpenAPI spec was the first step to go toward such a structure. If this API spec is clear, one can write tests to be sure that no code regression is introduced.
Thus, I am no friend of auto-magically generating the spec on each commit. That would risk breaking other things. So, I would even second to review and merge this PR as a basis for future enhancements.

[seyfeb]

I'd like to second @christianlupus on this. The Openapi spec should be the one source of truth that defines the interface between the back- and frontend. Whenever one is working on the spec document it is then clear that the person needs to be extra cautious.

Also AFAIK there are good code generators for creating API clients from the spec already so this is a tested path. Turning it around now and creating a spec from code seems to be dangerous.

[Leptopoda]

I am not sure who you are referring to. Maybe you can ping him/her or post a link to the discussion?

The discussion happened on matrix and the dev in question @provokateurin
I also have mixed feelings about the generation of an API-spec form the code but then just seeing #1464 some checking is obviously needed.

Maybe @provokateurin could elaborate on their choice as I also haven't gotten it yet.

Nevertheless I marked this one as a draft as I'm still checking some stuff with generation and want to get things right first.

[provokateurin]

Hi,
yes I'm working on the OpenAPI spec generation for Nextcloud. You can see the current progress here: https://github.com/nextcloud/server/pulls?q=is%3Apr+author%3Aprovokateurin+openapi. This is an official project of the company and will extend to most if not all official apps. It would be great if community apps also adopt this, but it's not required at all. I understand that it's more difficult to do this for some apps than for other ones.

Regarding @christianlupus concerns about incompatibility: The specs will be versioned of course, so that these things can be handled. It is also possible to define a field as having multiple types like string and int so that multiple versions of the recipe format can be handled at the same time, but that has to be handled on the client side then as well (I guess this is what's already happening?).

@seyfeb I also agree with you that there should only be a single source of truth. Cookbook is a bit special because it already has an OpenAPI spec, but as it seems it's not correctly representing the actual API at the moment. Almost all apps (except Cookbook and Maps) don't have any machine readable specification so the code will become the source of truth, since we need one source and the code is almost always the only thing that exists already (e.g. Talk has at least some good docs written down).

I know this is very complicated, but I've been working on these things for over a year now and I know many of the problems and solutions. I might make it sound easy, but it's definitely not.
Take nextcloud/server#36408 for example. I've written a OpenAPI spec by hand for it and now I was able to replace it with the automatically generated version and it worked with minimal changes. I have unit tests that validate that the spec/generated code works. Ideally I'd like to write one or more unit tests for every endpoint that will be documented with OpenAPI, but this is a lot of work of course. Cookbook could definitely be one of the apps that will get unit tests, especially with the concerns you expressed.

Cheers,
Kate

[christianlupus]

How about another suggestion?

The script is not working correctly at the moment and is still under development. Yet, @Leptopoda did write some work on the source manually.
It's there any drawback in merging this PR (or a slightly modified version of it) and keeping the automatic code script in the back of our heads?

No offense, @provokateurin, I cannot imagine this to work on a reproducible manner. This might be my personal inability to imagine the things. I would like to see that in action. Until then, i would rather avoid freezing any API development. If it works, we can still change.

Any opinions on that?

[provokateurin]

I don't have any objections about this PR or any other work. I was just asked to explain what I'm doing. If it ever comes so far that I try to adapt Cookbook, then I'll just open a PR as usual. I really didn't mean to stop anyone from working on this :/

[christianlupus]

I am sorry, i had to break this PR apart. Some parts are valid but others are not. It might be better to create smaller chunks that can be understood better.

This allows to fix the implementation of there is anything to fix.

I am not sure I got all implications of your suggestions, though. Why were you changing/adding the default values e.g.?

[Leptopoda]

As mentioned above this is still a work in progress especially on how the generator uses the information provided.
I agree that it will be easier to handle when breaking everything apart (and it will probably happen anyways if your operationId PR gets through).
I put this PR up mainly to document my changes and show how I handled the operationIds and everything I had done until then.

I am not sure I got all implications of your suggestions, though. Why were you changing/adding the default values e.g.?

The problem is that quite many fields are marked as required in the spec but not really required in the frontend (vue). In the frontend it is enough to provide a name and it will populate the creation date and every other required field. It is mainly for documenting that the requied fields will be auto populated with the given values.

Also dart is null safe meaning that if I don't mark a field as 'nullable' I have to provide a value otherwise a runtime error is thrown. Now I could provide a default value in my client or let the generated code do it. I haven't used kotlin but also is nullsafe so this will probably apply there as well.

---

TL;DR

• we could re evaluate what fields are required and adjust those.
• we could annotate required fields as nullable so we don't need to always provide them (see what I did with the config)
• we could provide a default value documented in the spec
• we could provide a default value in the client but those might be inconsistent across Projects

I guess providing a default in the spec is the least breaking as the api will still provide non null responses. As most in this pr is only documenting more of the behavior instead of changing it.

It's getting late and I hope my comment still makes sense :)

[christianlupus]

Let's get that merged. Looks good now.