fix: 🐛 Convert integer properties to int, fix model parsing #1207
Conversation
WalkthroughThe Dart model template at templates/dart/lib/src/models/model.dart.twig updates fromMap parsing for non-array fields. Integers now use guarded parsing: accept int directly or parse from String, with null handling. Numbers (doubles) now accept double directly or parse from String, with null handling. Strings now use explicit toString() with existing nullability behavior. Other types remain direct map access. Arrays continue using List.from with existing null fallback. No changes to constructors or toMap signatures. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
templates/dart/lib/src/models/model.dart.twig (2)
48-49: Improve number parsing with error handling and int-to-double conversion.The double parsing has similar issues to the integer case:
double.parse()can throw: invalid strings or null in required fields causeFormatException- Inefficient int handling: int values (e.g.,
42) fail theis doublecheck and go throughtoString()+parse()instead of direct.toDouble()conversionConsider this improved version:
- {%- elseif property.type == "number" -%} - (map['{{property.name | escapeDollarSign }}'] is double ? map['{{property.name | escapeDollarSign }}'] : {% if property.required %}double.parse(map['{{property.name | escapeDollarSign }}'].toString()){% else %}map['{{property.name | escapeDollarSign }}'] != null ? double.parse(map['{{property.name | escapeDollarSign }}'].toString()) : null{% endif %}) + {%- elseif property.type == "number" -%} + (map['{{property.name | escapeDollarSign }}'] is double + ? map['{{property.name | escapeDollarSign }}'] + : map['{{property.name | escapeDollarSign }}'] is int + ? (map['{{property.name | escapeDollarSign }}'] as int).toDouble() + : {% if property.required %}(double.tryParse(map['{{property.name | escapeDollarSign }}']?.toString() ?? '') ?? 0.0){% else %}(map['{{property.name | escapeDollarSign }}'] != null ? double.tryParse(map['{{property.name | escapeDollarSign }}'].toString()) : null){% endif %})This approach:
- Uses
tryParse()to avoid exceptions- Explicitly handles int-to-double conversion efficiently
- Provides a default value (0.0) for required fields when parsing fails
52-53: Consider type validation for non-primitive types.The fallback case (booleans, objects, etc.) uses direct map access without type checking. While this is generally safe for well-formed JSON, consider whether additional type validation would help catch unexpected data:
// Example for boolean validation (if needed) {% if property.type == "boolean" %} (map['{{property.name | escapeDollarSign }}'] is bool ? map['{{property.name | escapeDollarSign }}'] : {% if property.required %}false{% else %}null{% endif %}) {% else %} map['{{property.name | escapeDollarSign }}'] {% endif %}This is optional and depends on how strictly you want to validate server responses.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
templates/dart/lib/src/models/model.dart.twig(1 hunks)
🔇 Additional comments (2)
templates/dart/lib/src/models/model.dart.twig (2)
46-54: Verify type conversions for reported scenarios and finalize error-handling strategy.
- Add or confirm tests covering:
- Decide on a consistent parse-failure policy: tryParse with defaults, throw descriptive exceptions, or enable debug-mode logging.
50-51: Add null safety for required string properties.For required string properties, if the map value is
nullor the key is missing, calling.toString()onnullwill throw at runtime.Consider adding a null check with a fallback:
- {%- elseif property.type == "string" -%} - map['{{property.name | escapeDollarSign }}']{% if not property.required %}?{% endif %}.toString() + {%- elseif property.type == "string" -%} + {% if property.required %}(map['{{property.name | escapeDollarSign }}'] ?? '').toString(){% else %}map['{{property.name | escapeDollarSign }}']?.toString(){% endif %}This ensures required fields have a default empty string if the server omits the field, preventing runtime crashes.
Likely an incorrect or invalid review comment.
| {%- if property.type == "integer" -%} | ||
| (map['{{property.name | escapeDollarSign }}'] is int ? map['{{property.name | escapeDollarSign }}'] : {% if property.required %}int.parse(map['{{property.name | escapeDollarSign }}'].toString()){% else %}map['{{property.name | escapeDollarSign }}'] != null ? int.parse(map['{{property.name | escapeDollarSign }}'].toString()) : null{% endif %}) |
There was a problem hiding this comment.
Add error handling for int.parse() failures.
The integer parsing logic handles the String-to-int conversion (issue #1171) and null handling (issue #1150), but int.parse() can throw FormatException in several cases:
- Invalid string format: non-numeric strings like
"abc"or"1.5"(decimals) - Double/float values: if the server returns
1.5(a double),toString()produces"1.5", whichint.parse()rejects - Required properties with null: if a required field is null,
int.parse("null")throws
Consider these options:
Option 1 (recommended): Use int.tryParse() with fallback for required fields:
- {%- if property.type == "integer" -%}
- (map['{{property.name | escapeDollarSign }}'] is int ? map['{{property.name | escapeDollarSign }}'] : {% if property.required %}int.parse(map['{{property.name | escapeDollarSign }}'].toString()){% else %}map['{{property.name | escapeDollarSign }}'] != null ? int.parse(map['{{property.name | escapeDollarSign }}'].toString()) : null{% endif %})
+ {%- if property.type == "integer" -%}
+ (map['{{property.name | escapeDollarSign }}'] is int
+ ? map['{{property.name | escapeDollarSign }}']
+ : {% if property.required %}(int.tryParse(map['{{property.name | escapeDollarSign }}']?.toString() ?? '') ?? 0){% else %}(map['{{property.name | escapeDollarSign }}'] != null ? int.tryParse(map['{{property.name | escapeDollarSign }}'].toString()) : null){% endif %})Option 2: Handle doubles explicitly by truncating/rounding:
- {%- if property.type == "integer" -%}
- (map['{{property.name | escapeDollarSign }}'] is int ? map['{{property.name | escapeDollarSign }}'] : {% if property.required %}int.parse(map['{{property.name | escapeDollarSign }}'].toString()){% else %}map['{{property.name | escapeDollarSign }}'] != null ? int.parse(map['{{property.name | escapeDollarSign }}'].toString()) : null{% endif %})
+ {%- if property.type == "integer" -%}
+ (map['{{property.name | escapeDollarSign }}'] is int
+ ? map['{{property.name | escapeDollarSign }}']
+ : map['{{property.name | escapeDollarSign }}'] is double
+ ? (map['{{property.name | escapeDollarSign }}'] as double).toInt()
+ : {% if property.required %}int.parse(map['{{property.name | escapeDollarSign }}'].toString()){% else %}(map['{{property.name | escapeDollarSign }}'] != null ? int.tryParse(map['{{property.name | escapeDollarSign }}'].toString()) : null){% endif %})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {%- if property.type == "integer" -%} | |
| (map['{{property.name | escapeDollarSign }}'] is int ? map['{{property.name | escapeDollarSign }}'] : {% if property.required %}int.parse(map['{{property.name | escapeDollarSign }}'].toString()){% else %}map['{{property.name | escapeDollarSign }}'] != null ? int.parse(map['{{property.name | escapeDollarSign }}'].toString()) : null{% endif %}) | |
| {%- if property.type == "integer" -%} | |
| (map['{{property.name | escapeDollarSign }}'] is int | |
| ? map['{{property.name | escapeDollarSign }}'] | |
| : {% if property.required %}(int.tryParse(map['{{property.name | escapeDollarSign }}']?.toString() ?? '') ?? 0){% else %}(map['{{property.name | escapeDollarSign }}'] != null ? int.tryParse(map['{{property.name | escapeDollarSign }}'].toString()) : null){% endif %}) |
| {%- if property.type == "integer" -%} | |
| (map['{{property.name | escapeDollarSign }}'] is int ? map['{{property.name | escapeDollarSign }}'] : {% if property.required %}int.parse(map['{{property.name | escapeDollarSign }}'].toString()){% else %}map['{{property.name | escapeDollarSign }}'] != null ? int.parse(map['{{property.name | escapeDollarSign }}'].toString()) : null{% endif %}) | |
| {%- if property.type == "integer" -%} | |
| (map['{{property.name | escapeDollarSign }}'] is int | |
| ? map['{{property.name | escapeDollarSign }}'] | |
| : map['{{property.name | escapeDollarSign }}'] is double | |
| ? (map['{{property.name | escapeDollarSign }}'] as double).toInt() | |
| : {% if property.required %}int.parse(map['{{property.name | escapeDollarSign }}'].toString()){% else %}(map['{{property.name | escapeDollarSign }}'] != null ? int.tryParse(map['{{property.name | escapeDollarSign }}'].toString()) : null){% endif %}) |
|
@iamngoni the issue "Type 'String' is not a subtype of type 'int'" has been fixed server side and does not require sdk change, please create new pr for the other issue |
|
@ChiragAgg5k thank you 🙏. I've created a new PR for the other issue. |
2 participants
What does this PR do?
fix: 🐛 Convert integer properties to int, fix model parsing
Fixes #1171 - Type 'String' is not a subtype of type 'int'
Fixes #1150 - Type 'Null' is not a subtype of type 'int'
Test Plan
[x] Ran both Dart & Flutter tests (both passed)
Related PRs and Issues
#1171
#1150
Have you read the Contributing Guidelines on issues?
Yes
Summary by CodeRabbit