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.

Sign up for GitHub

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

Jump to bottom

Feature/openapi automatic #465

Merged
merged 4 commits into from
Aug 4, 2023
Merged

Feature/openapi automatic #465

merged 4 commits into from
Aug 4, 2023

Conversation

provokateurin
Copy link
Member

Closes #1
Closes #417
Depends on #462 #463 #464 and nextcloud/server#39223

@provokateurin provokateurin requested a review from Leptopoda July 13, 2023 15:11
@provokateurin provokateurin force-pushed the feature/openapi-automatic branch from f9e3ec1 to e590c1c Compare July 13, 2023 15:19
@Leptopoda
Copy link
Member

Looks ok at the first glance.
I'll thoroughly review it in the coming days.

@Leptopoda
Copy link
Member

I might also rebase/redo my changes for #420 so we can land the automatic branch without any big changes afterwards (might be better for devs already using this lib to adjust only once).

@provokateurin
Copy link
Member Author

Sounds look a good plan, let's do it 👍

@provokateurin provokateurin force-pushed the feature/openapi-automatic branch 3 times, most recently from 455e5cf to 6c294a8 Compare July 15, 2023 15:38
@Leptopoda Leptopoda mentioned this pull request Jul 17, 2023
Merged
@Leptopoda
Copy link
Member

The ContentString serialization is fixed with #501 but the same test now fails with a 500 server error.
Let me know if this is also related to built_value

@provokateurin
Copy link
Member Author

I'll look into the test failure once your PR is merged

@provokateurin
Copy link
Member Author

I assume the problem is that we don't properly deserialize the parameters. OpenAPI can describe many ways to serialize values (https://swagger.io/docs/specification/serialization/) but we just assume something and then spec also doesn't say what it actually needs.

@provokateurin
Copy link
Member Author

Yep, server log confirms it: Argument #5 ($shareTypes) must be of type array, string given.

@provokateurin provokateurin force-pushed the feature/openapi-automatic branch from 6c294a8 to ff698ea Compare July 29, 2023 06:24
@provokateurin provokateurin mentioned this pull request Jul 31, 2023
Merged
@provokateurin provokateurin force-pushed the feature/openapi-automatic branch from ff698ea to fae6432 Compare August 1, 2023 07:42
@provokateurin provokateurin marked this pull request as ready for review August 1, 2023 07:43
@provokateurin
Copy link
Member Author

The autocomplete test is still failing because we are not properly serializing the parameter.

@Leptopoda
Copy link
Member

The autocomplete test is still failing because we are not properly serializing the parameter.

But this isn't a problem with dynamite?

Yep, server log confirms it: Argument #5 ($shareTypes) must be of type array, string given.

The spec says it's a string (ContentString to be exact). After changing this the AutocompleteResult.status appears to also be wrong.

https://github.com/provokateurin/nextcloud-neon/pull/465/files#diff-24dc2feb72ef0ff5bc5c6e6b4fe8f5efd9693883efc2893bd0d1afd9feea73a3R1576-R1592

@provokateurin
Copy link
Member Author

I think the spec is wrong, but the correct definition is also not implemented in dynamite.

@provokateurin provokateurin mentioned this pull request Aug 3, 2023
Merged
@provokateurin
Copy link
Member Author

I fixed the specs and when #522 is merged it will work correctly. The autocomplete tests is still broken because of a bug that was I fixed but that is only in 28+.

@provokateurin provokateurin force-pushed the feature/openapi-automatic branch from fae6432 to 8eed662 Compare August 3, 2023 08:22
@Leptopoda
Copy link
Member

We can still skip it until we switch to 28

@provokateurin
Copy link
Member Author

The 28 release is still a while in the future :/ We don't use the autocomplete in Neon yet, so we could just merge it

@provokateurin provokateurin force-pushed the feature/openapi-automatic branch 2 times, most recently from c20a918 to db038aa Compare August 3, 2023 09:19
Leptopoda
Leptopoda reviewed Aug 3, 2023
View reviewed changes
Copy link
Member

@Leptopoda Leptopoda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just reviewing the not generated code 😅

I'll still need to test it but it looks quite good.

packages/nextcloud/test/provisioning_api_test.dart Outdated Show resolved Hide resolved
packages/nextcloud/test/core_test.dart Outdated Show resolved Hide resolved
packages/neon/neon/lib/src/models/account.dart Show resolved Hide resolved
packages/nextcloud/lib/src/helpers/news.dart Outdated Show resolved Hide resolved
packages/nextcloud/lib/src/helpers/core.dart Outdated Show resolved Hide resolved
packages/nextcloud/lib/src/client.dart Outdated Show resolved Hide resolved
@provokateurin provokateurin mentioned this pull request Aug 4, 2023
Merged
@provokateurin provokateurin requested a review from Leptopoda August 4, 2023 13:19
@Leptopoda
Copy link
Member

I don't see any changes from my last review.
What should I review?

@provokateurin
Copy link
Member Author

Just approve :) Will push my rebase now

@provokateurin
Copy link
Member Author

Ah I forgot that I haven't pushed my changes yet 🙃

@provokateurin provokateurin force-pushed the feature/openapi-automatic branch from db038aa to 865d41c Compare August 4, 2023 16:16
@provokateurin provokateurin force-pushed the feature/openapi-automatic branch from 865d41c to e222ac5 Compare August 4, 2023 16:17
@provokateurin
Copy link
Member Author

@Leptopoda now you can review

Leptopoda
Leptopoda approved these changes Aug 4, 2023
View reviewed changes
Copy link
Member

@Leptopoda Leptopoda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@provokateurin
Copy link
Member Author

Actually to also close #1 we need files_sharing

@provokateurin provokateurin merged commit e78054c into main Aug 4, 2023
@provokateurin provokateurin deleted the feature/openapi-automatic branch August 4, 2023 17:01
@provokateurin
Copy link
Member Author

Woop woop! A dream came true! ❤️

@provokateurin provokateurin mentioned this pull request Aug 5, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Leptopoda Leptopoda Leptopoda approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use automatic OpenAPI specs Implement all API endpoints of dart-nextcloud
2 participants
@provokateurin @Leptopoda