Skip to content

fix(mobile): add missing backward-compat defaults in openapi_patching #28755

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

Closed
timonrieger wants to merge 3 commits into from
Closed

fix(mobile): add missing backward-compat defaults in openapi_patching #28755

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8c11490
regression from #28665
timonrieger Jun 1, 2026
2b8ff25
regression from #28272
timonrieger Jun 1, 2026
d2ba106
followup for #24155
timonrieger Jun 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions mobile/lib/utils/openapi_patching.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,30 @@ dynamic upgradeDto(dynamic value, String targetType) {
addDefault(value, 'profileChangedAt', DateTime.now().toIso8601String());
addDefault(value, 'hasProfileImage', false);
}
case 'AssetEditActionItemDtoParameters':
if (value is Map) {
addDefault(value, 'angle', 0);
addDefault(value, 'x', 0);
addDefault(value, 'y', 0);
addDefault(value, 'width', 0);
addDefault(value, 'height', 0);
addDefault(value, 'axis', 'horizontal');
}
break;
case 'SyncAssetV1':
if (value is Map) {
addDefault(value, 'isEdited', false);
addDefault(value, 'createdAt', null);
}
case 'SyncAssetV2':
if (value is Map) {
addDefault(value, 'createdAt', null);
}
case 'ServerVersionResponseDto':
if (value is Map) {
addDefault(value, 'prerelease', null);
}
break;
Comment on lines +70 to +73

@jrasm91 jrasm91 Jun 1, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm surprised this is needed, since it's nullable.

@timonrieger timonrieger Jun 1, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

but not optional 😁

@timonrieger timonrieger Jun 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

it does not raise on main atm, due to this patch stripping assertions. While testing #27231, this raised tho in the mobile app

Logs 

'package:openapi/model/server_version_response_dto.dart': Failed assertion: line 95 pos 16: 'json.containsKey(r'prerelease')': Required key "ServerVersionResponseDto[prerelease]" is missing from JSON.

@timonrieger timonrieger Jun 1, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

so effectively, prerelease is still required (see openapi spec) and thus needs this…unless we mark the field as optional, too

@jrasm91 jrasm91 Jun 1, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We might need to fix other issues when we make that change. There are other required, nullable properties.

@jrasm91 jrasm91 Jun 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah so these aren't strictly required. Nullable values will fallback to null by default. Idk if need/want to add back in the required assertions.

@shenlong-tanwen shenlong-tanwen Jun 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@timonrieger This might be an issue whenever a server promotes some field to required and the mobile app has to patch it. Rather than tracking this, maybe we should drop the assertions

@timonrieger timonrieger Jun 2, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

i disagree. In debug/dev these assertions catch bugs which otherwise stay silent and could cascade further down if the response was used. i don't see why we would want to ignore these bugs, even if they don't affect us at the moment 🤔

@timonrieger timonrieger Jun 2, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

i mean it's basically response validation on the client-side (which we discussed server-side alr btw. #27901 😄 ). in prod build, these assertions get stripped anyways.

@jrasm91 jrasm91 Jun 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I personally don't find any value in assertions for required null values. The only thing it actually does it make the mobile App break in development when testing against an old server.

case 'ServerFeaturesDto':
if (value is Map) {
addDefault(value, 'ocr', false);
Expand Down
Loading