-
-
Notifications
You must be signed in to change notification settings - Fork 7k
[Dart] Add UInt8List support, allowing for type:string format:binary definitions #13451
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are alot of whitespace changes here that are polluting the PR, it would be great if the PR had the bare minimum changes
class PetApi { | ||
PetApi([ApiClient? apiClient]) : apiClient = apiClient ?? defaultApiClient; | ||
|
||
final ApiClient apiClient; | ||
|
||
/// Add a new pet to the store | ||
/// | ||
/// | ||
/// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please avoid whitespace changes since they pollute the PR
/// | ||
/// Note: This method returns the HTTP [Response]. | ||
/// | ||
/// Parameters: | ||
/// | ||
/// * [Pet] pet (required): | ||
/// Pet object that needs to be added to the store | ||
Future<Response> addPetWithHttpInfo(Pet pet,) async { | ||
Future<Response> addPetWithHttpInfo( | ||
Pet pet, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whitespace changes here too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The newest version of this branch should have corrected the extraneous whitespace.
When I get home I'll try to fix it, but I didn't touch any of those files by hand. I only ran the generate samples script. I didn't even open these in my editor. |
5e715aa
to
819c9f1
Compare
return responseBytes; | ||
} | ||
|
||
final decodedString = const Utf8Decoder().convert(responseBytes); | ||
// If the expected target type is String, nothing to do... | ||
return targetType == 'String' | ||
? json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this return decodedString
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eagle-eye'd catch there. json
isn't even a variable in the new method anymore, my mistake New push of the branch has it returning decodedString
now.
* Adds Uint8List as a valid return type for type: string format: binary definitions * Adds support for using bodyBytes instead of body for deserializing response data
819c9f1
to
d5ed19f
Compare
@kuhnroyal @wing328 Can you check why the tests were passing here when there was a compile error for the generated code? |
@0xNF did you make any changes in the java files? I can't see them in the PR |
The Java changes seem to have gotten lost. Sorry this PR is kinda messy. I'm flipping between too many branches rn. |
The test did run and passed. Does it has anything to do with Dart version?
That's what we use in the CI. |
dart 2 uses 2.12 in the generated code, which 2.14 should be able to run |
Can you please paste the compilation error here as well? I'll try to repeat that locally. |
It's probably because I force pushed the update to my own branch, and the tests didn't re-run here. |
To trigger the run, please update the samples and the Github action (dart job) will run again. |
Should I be regenerating all samples? Up til now I've just been regenerating the relevant samples for my changes (Dart). But then the CI complains. |
yes please update the samples as the CI fails: https://github.com/OpenAPITools/openapi-generator/actions/runs/3086648029/jobs/4991237644 |
@0xNF @wing328 Is there any movement on this? It looks like this PR is functionally ready to land. Just needing some clean-up. Currently, I'm facing an issue around this as passing a
Relevant Code (Generated API's model.dart):
|
@zmoshansky this PR is for |
Summary
tl;dr
UInt8List
support, which Dart-Dio has, has been added to the regular Dart2 generator.Fixes #12161.
Details
Changes to the Generators:
To support UInt8List in the generator, Typed Data imports had to be added. Additionally, some OpenAPI data types have been switched to Uint8List.
Uint8List/dart:typed_data
to the imports.typeMapping
of {file, binary} toUint8List
away fromMultiPartFile
postProcessOperationsWithModels
from DartDioClientCodegen.javaimport 'dart:typed_data';
Changes to the API classes
To support Uint8List in the generated API output, parts of the *api.dart files had to be refactored. Presently Dart2 relies on passing utf8 decoded response byte strings through its API calls, but this leads to errors because not all response objects are json strings. To handle this, I refactored many of the calls to take Uint8List's straight from
response.bodyBytes
. Decoding into aString
object doesn't happen until it needs to, which is right before passing the object down into the Json Decoding section.response.bodyBytes
decodeError
method to deal with the byte response, which decodes the resposne into a utf-8 String.To confirm this change, use the following specfile:
specfile
PR checklist
master
(6.1.0) (minor release - breaking changes with fallbacks),7.0.x
(breaking changes without fallbacks)@jaumard @josh-burton @amondnet @sbu-WBT @kuhnroyal @agilob @ahmednfwela