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

fix(nextcloud)!: Move all query parameters to body to fix many serialization problems #2129

Merged
merged 3 commits into from
Jul 4, 2024

Conversation

provokateurin
Copy link
Member

@provokateurin provokateurin commented Jun 4, 2024

Depends on nextcloud/openapi-extractor#105

I can't even begin to list all the problems this solves 😅
There were quite some bugs in the way we serialized and encoded request bodies and how we handle default values in those cases. I wasn't really able to separate the fixes, so I hope this is ok.

We can also already merge the dynamite fixes and stash the nextcloud changes until the openapi-extractor PR is merged (which could be very soon already as there is only one minor blocker with the ocs_api_viewer).

@provokateurin provokateurin requested a review from Leptopoda June 4, 2024 19:47
@provokateurin provokateurin force-pushed the fix/nextcloud/body-parameters branch from 34d55e1 to a0f3ecf Compare June 4, 2024 19:52
@provokateurin
Copy link
Member Author

CI failure is due to the bloc tests not being migrated.

@provokateurin provokateurin force-pushed the fix/nextcloud/body-parameters branch from a0f3ecf to d080089 Compare June 5, 2024 09:48
Copy link

codecov bot commented Jun 5, 2024

Codecov Report

Attention: Patch coverage is 55.69137% with 580 lines in your changes missing coverage. Please review.

Project coverage is 29.59%. Comparing base (7673cea) to head (64cedb2).
Report is 5 commits behind head on main.

Current head 64cedb2 differs from pull request most recent head db36e48

Please upload reports for the commit db36e48 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2129      +/-   ##
==========================================
- Coverage   30.73%   29.59%   -1.14%     
==========================================
  Files         259      260       +1     
  Lines       85163    95000    +9837     
==========================================
+ Hits        26174    28115    +1941     
- Misses      58989    66885    +7896     
Flag Coverage Δ *Carryforward flag
cookie_store 90.51% <ø> (ø) Carriedforward from 7de05bf
dynamite 31.01% <0.00%> (-0.07%) ⬇️
dynamite_end_to_end_test 61.06% <66.51%> (-0.29%) ⬇️
dynamite_runtime 85.40% <ø> (ø) Carriedforward from 7de05bf
neon_dashboard 96.22% <100.00%> (+0.17%) ⬆️
neon_framework 43.43% <86.66%> (-0.52%) ⬇️
neon_notifications 100.00% <ø> (ø)
neon_talk 98.81% <97.05%> (-0.08%) ⬇️
nextcloud 26.11% <46.17%> (-0.94%) ⬇️
sort_box 90.90% <ø> (ø) Carriedforward from 7de05bf

*This pull request uses carry forward flags. Click here to find out more.

Files Coverage Δ
...namite_end_to_end_test/lib/parameters.openapi.dart 78.76% <100.00%> (ø)
...s/neon/neon_dashboard/lib/src/blocs/dashboard.dart 100.00% <100.00%> (ø)
packages/neon/neon_talk/lib/src/blocs/room.dart 97.79% <100.00%> (+0.11%) ⬆️
.../neon/neon_talk/lib/src/widgets/message_input.dart 95.04% <100.00%> (-0.61%) ⬇️
...talk/lib/src/widgets/rich_object/file_preview.dart 100.00% <100.00%> (ø)
...s/neon_framework/lib/src/blocs/unified_search.dart 100.00% <100.00%> (ø)
...ages/neon_framework/lib/src/blocs/user_status.dart 95.50% <100.00%> (+0.63%) ⬆️
...s/neon_framework/lib/src/blocs/weather_status.dart 100.00% <100.00%> (ø)
...es/neon_framework/lib/src/testing/mock_server.dart 88.88% <100.00%> (-2.78%) ⬇️
packages/nextcloud/lib/src/api/core.openapi.dart 43.21% <ø> (-0.34%) ⬇️
... and 24 more

... and 20 files with indirect coverage changes

@provokateurin provokateurin force-pushed the fix/nextcloud/body-parameters branch from d080089 to d734922 Compare June 6, 2024 10:43
@provokateurin
Copy link
Member Author

Ready for review, openapi-extractor PR was merged 🚀

@provokateurin provokateurin force-pushed the fix/nextcloud/body-parameters branch from d734922 to 64cedb2 Compare June 12, 2024 11:07
@provokateurin provokateurin added this to the Nextcloud 7.0.0 release milestone Jun 12, 2024
@provokateurin provokateurin force-pushed the fix/nextcloud/body-parameters branch 2 times, most recently from 05ab259 to db36e48 Compare June 27, 2024 04:57
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.

There is nothing really against this. I'm not a big fan of the double __ but this can later be changed in a non breaking update.

Approving to not block this further

@provokateurin provokateurin force-pushed the fix/nextcloud/body-parameters branch from db36e48 to d2787bf Compare July 4, 2024 10:50
@provokateurin provokateurin enabled auto-merge July 4, 2024 10:50
@provokateurin provokateurin merged commit eaf1421 into main Jul 4, 2024
8 checks passed
@provokateurin provokateurin deleted the fix/nextcloud/body-parameters branch July 4, 2024 11:03
@provokateurin provokateurin removed this from the Nextcloud 8.0.0 release milestone Aug 13, 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

Successfully merging this pull request may close these issues.

2 participants