-
-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
Make moduleObject.mustache confirms to JSONEncodable. #11202
Conversation
|
||
extension JSONEncodable where Self: Encodable { | ||
func encodeToJSON() -> Any { | ||
let encoder = JSONEncoder() |
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.
@0x0c here instead of creating a new JSONEncoder, please use CodableHelper.jsonEncoder
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.
Done with 1c06b4d
guard let data = try? encoder.encode(self) else { | ||
fatalError("Could not encode to json: \(self)") | ||
} | ||
return data.base64EncodedString(options: Data.Base64EncodingOptions()) |
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.
@0x0c I'm a little worried about this.
When do we want to encode json as base64?
Only in multipart/form-data requests?
Can't we just return the json in Data
?
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 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.
I see.
Could you please reuse that extension please?
return data.encodeToJSON()
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.
I updated line 216 so please check 7836e27
Hey @0x0c, |
@@ -1,5 +1,5 @@ | |||
{{^objcCompatible}}{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} {{#useClasses}}final class{{/useClasses}}{{^useClasses}}struct{{/useClasses}} {{{classname}}}: {{#useVapor}}Content{{/useVapor}}{{^useVapor}}Codable{{/useVapor}}{{#vendorExtensions.x-swift-hashable}}, Hashable{{/vendorExtensions.x-swift-hashable}} { | |||
{{/objcCompatible}}{{#objcCompatible}}@objc {{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} class {{classname}}: NSObject, Codable { | |||
{{^objcCompatible}}{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} {{#useClasses}}final class{{/useClasses}}{{^useClasses}}struct{{/useClasses}} {{{classname}}}: {{#useVapor}}Content{{/useVapor}}{{^useVapor}}Codable{{/useVapor}}{{#vendorExtensions.x-swift-hashable}}, Hashable{{/vendorExtensions.x-swift-hashable}}, JSONEncodable { |
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.
@0x0c to make the CI pass, could you please update this line to be
{{^objcCompatible}}{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} {{#useClasses}}final class{{/useClasses}}{{^useClasses}}struct{{/useClasses}} {{{classname}}}: {{#useVapor}}Content{{/useVapor}}{{^useVapor}}Codable, JSONEncodable{{/useVapor}}{{#vendorExtensions.x-swift-hashable}}, Hashable{{/vendorExtensions.x-swift-hashable}}{
Because when using Vapor, we can't make the model conform to JSONEncodable
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.
Done with aae8e6d
@4brunu Thank you for your review. I updated codes. Please check it again. Thanks. |
@0x0c thanks for making the changes, let's wait for CI to see if passes. |
@@ -206,3 +206,13 @@ extension Set: RequestDecodable where Element: Content { | |||
extension Set: Content where Element: Content { } | |||
|
|||
extension AnyCodable: Content {}{{/useVapor}} | |||
|
|||
extension JSONEncodable where Self: Encodable { |
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.
@0x0c could you please move this entire block of code to line 63, so that it's not generated when using vapor, for CI to pass?
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.
Done with ebd7d01
{{^objcCompatible}}{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} {{#useClasses}}final class{{/useClasses}}{{^useClasses}}struct{{/useClasses}} {{{classname}}}: {{#useVapor}}Content{{/useVapor}}{{^useVapor}}Codable{{/useVapor}}{{#vendorExtensions.x-swift-hashable}}, Hashable{{/vendorExtensions.x-swift-hashable}} { | ||
{{/objcCompatible}}{{#objcCompatible}}@objc {{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} class {{classname}}: NSObject, Codable { | ||
{{^objcCompatible}}{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} {{#useClasses}}final class{{/useClasses}}{{^useClasses}}struct{{/useClasses}} {{{classname}}}: {{#useVapor}}Content{{/useVapor}}{{^useVapor}}Codable, JSONEncodable{{/useVapor}}{{#vendorExtensions.x-swift-hashable}}, Hashable{{/vendorExtensions.x-swift-hashable}}{ | ||
{{/objcCompatible}}{{#objcCompatible}}@objc {{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} class {{classname}}: NSObject, Codable, JSONEncodable { | ||
{{/objcCompatible}} | ||
|
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.
Could you please also add the JSONEncodable
conformance after Codable
in the following files, please?
modelEnum.mustache
modelInlineEnumDeclaration.mustache
modelOneOf.mustache
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.
@4brunu I fixed issues which you mentioned. Let's wait the result of CI again. |
} | ||
{{/useVapor}}{{#generateModelAdditionalProperties}} |
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.
@0x0c just a small nit, could you please make this in one line to improve the generated code?
}{{/useVapor}}{{#generateModelAdditionalProperties}}
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.
Done with 8b1529f
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.
Thanks, could you please update the sample projects also?
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.
OK!
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 last commit brought a lot of undesired changes, could you please revert it?
Maybe you need to first merge the master branch into yours and then run the following commands.
./mvnw clean package
./bin/generate-samples.sh
./bin/utils/export_docs_generators.sh
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 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.
Could you please explain why some enums are not JSONEncodable?
Is there any issue that you found?
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 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.
You are right, thanks 👍
@@ -1,4 +1,4 @@ | |||
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} enum {{classname}}: {{dataType}}, {{#useVapor}}Content, Hashable{{/useVapor}}{{^useVapor}}Codable{{/useVapor}}, CaseIterable{{#enumUnknownDefaultCase}}{{#isInteger}}, CaseIterableDefaultsLast{{/isInteger}}{{#isFloat}}, CaseIterableDefaultsLast{{/isFloat}}{{#isDouble}}, CaseIterableDefaultsLast{{/isDouble}}{{#isString}}, CaseIterableDefaultsLast{{/isString}}{{/enumUnknownDefaultCase}} { | |||
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} enum {{classname}}: {{dataType}}, {{#useVapor}}Content, Hashable{{/useVapor}}{{^useVapor}}Codable{{#isContainer}}{{#isString}}{{^isInteger}}{{^isFloat}}{{^isDouble}}, JSONEncodable{{/isDouble}}{{/isFloat}}{{/isInteger}}{{/isString}}{{/isContainer}}{{/useVapor}}, CaseIterable{{#enumUnknownDefaultCase}}{{#isInteger}}, CaseIterableDefaultsLast{{/isInteger}}{{#isFloat}}, CaseIterableDefaultsLast{{/isFloat}}{{#isDouble}}, CaseIterableDefaultsLast{{/isDouble}}{{#isString}}, CaseIterableDefaultsLast{{/isString}}{{/enumUnknownDefaultCase}} { |
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.
@0x0c Could you please explain this logic please?
{{#isContainer}}{{#isString}}{{^isInteger}}{{^isFloat}}{{^isDouble}}, JSONEncodable{{/isDouble}}{{/isFloat}}{{/isInteger}}{{/isString}}{{/isContainer}}
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.
Same reason of #11202 (comment).
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.
So an Enum should conform to JSONEncodable if it's a string os is a container? Or if it's a string and is a container?
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.
Sorry I copied and pasted modelInlineEnumDeclaration.mustache
and it is not appropriate. modelInlineEnumDeclaration.mustache
confirms String
when it is a container. I fixed with 235b824
@@ -1,5 +1,5 @@ | |||
{{^objcCompatible}}{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} {{#useClasses}}final class{{/useClasses}}{{^useClasses}}struct{{/useClasses}} {{{classname}}}: {{#useVapor}}Content{{/useVapor}}{{^useVapor}}Codable{{/useVapor}}{{#vendorExtensions.x-swift-hashable}}, Hashable{{/vendorExtensions.x-swift-hashable}} { | |||
{{/objcCompatible}}{{#objcCompatible}}@objc {{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} class {{classname}}: NSObject, Codable { | |||
{{^objcCompatible}}{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} {{#useClasses}}final class{{/useClasses}}{{^useClasses}}struct{{/useClasses}} {{{classname}}}: {{#useVapor}}Content{{/useVapor}}{{^useVapor}}Codable, JSONEncodable{{/useVapor}}{{#vendorExtensions.x-swift-hashable}}, Hashable{{/vendorExtensions.x-swift-hashable}}{ |
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.
@0x0c another nit that I found is that at the end of the line, there is a space before the final {
that was acidently removed and is creating some noise for example in the vapor client that shouldn't have any changes with this PR.
Could you please restore that space, to avoid noise in the generated code?
This command should generate the Swift sample projects.
./bin/generate-samples.sh bin/configs/swift5-*
`
``
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.
@@ -1,4 +1,4 @@ | |||
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} enum {{enumName}}: {{^isContainer}}{{dataType}}{{/isContainer}}{{#isContainer}}String{{/isContainer}}, {{#useVapor}}Content, Hashable{{/useVapor}}{{^useVapor}}Codable{{/useVapor}}, CaseIterable{{#enumUnknownDefaultCase}}{{#isInteger}}, CaseIterableDefaultsLast{{/isInteger}}{{#isFloat}}, CaseIterableDefaultsLast{{/isFloat}}{{#isDouble}}, CaseIterableDefaultsLast{{/isDouble}}{{#isString}}, CaseIterableDefaultsLast{{/isString}}{{#isContainer}}, CaseIterableDefaultsLast{{/isContainer}}{{/enumUnknownDefaultCase}} { | |||
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} enum {{enumName}}: {{^isContainer}}{{dataType}}{{/isContainer}}{{#isContainer}}String{{/isContainer}}, {{#useVapor}}Content, Hashable{{/useVapor}}{{^useVapor}}Codable{{^isContainer}}{{^isString}}{{^isInteger}}{{^isFloat}}{{^isDouble}}, JSONEncodable{{/isDouble}}{{/isFloat}}{{/isInteger}}{{/isString}}{{/isContainer}}{{/useVapor}}, CaseIterable{{#enumUnknownDefaultCase}}{{#isInteger}}, CaseIterableDefaultsLast{{/isInteger}}{{#isFloat}}, CaseIterableDefaultsLast{{/isFloat}}{{#isDouble}}, CaseIterableDefaultsLast{{/isDouble}}{{#isString}}, CaseIterableDefaultsLast{{/isString}}{{#isContainer}}, CaseIterableDefaultsLast{{/isContainer}}{{/enumUnknownDefaultCase}} { |
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.
@0x0c is it necessary to check is it's not container?
This implementation is different from modelEnum.mustache
, is it normal?
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.
Maybe it's related to #11202 (comment)?
If it's correct being different and you could give a bit of more context, I would appreciate it.
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.
If modelEnumDeclaration.mustache
is container, the enum confirms String. Therefore, we have to check whether the enum is container or not.
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.
And this doesn't apply also to modelInlineEnumDeclaration.mustache
?
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.
Oh, sorry I missed type. In previous my post "If modelEnumDeclaration.mustache
is container ..." is not correct.. "If modelInlineEnumDeclaration.mustache
is container ..." is correct.
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.
This PR looks good to me.
If you could explain why we need to check for container in one but not the other I would appreciate it, because this was something that was always a bit confusing to me
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.
If you could explain why we need to check for container in one but not the other I would appreciate it, because this was something that was always a bit confusing to me
Sorry, I can't answer your question because a code for checking container is already implemented before I modify modelEnumDeclaration.mustache
. I respect original mustache and don't change the mustache is better I think.
Originally, according to the first commit of these files, there is difference between modelEnum.mustache
and modelInlineEnumDeclaration.mustache
. One is checking for container and other is not.
See here and here.
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.
I see, thanks for the explanation 👍
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.
Looks good to me 👍
Thank you! Can't wait for merging this PR :) |
Thank you for the patient, making the requested changes and answering my questions 👍 |
@4brunu @0x0c Hello, I think I found an issue with this PR. Within request, one may append query items, such as:
If the variable is a string, the new encodeToJSON behaviour will break existing code. Previously, the variable was set as is. Now, the String is encoded as JSON... Is this really intended? |
But it does.... :/ If you look at the extension, both apply to a String (as it is Encodable).
|
@0x0c @honkmaster do you have a solution other than revert this PR? Thanks |
I think the solution should be something along those lines. But I'm getting the following error Any suggestions? Or maybe restrict the extension |
I think I found a solution. |
I have a similar problem on 5.4.0 FWIW. |
Could you please test if the version in master branch works for you? |
Yep, will give it a try. Do you know of a quick way to get the master branch installed via Homebrew, or do I have to clone and build myself? 😄 |
You have to clone it and build it yourself.
|
@4brunu For what it's worth, I have the exact same issue as @rhys-rantmedia in 5.4.0. I've tried debugging it, but I really can't figure out where in the whole process the URL gets turned into a local path string. |
For now the only solution is to build from source or to use an older version. |
@denizdogan could you please confirm if that works for you? |
@4brunu Yeah, I just resorted to using 5.3.1 again, no issues :) Thanks. |
Sorry for the trouble, this will be fixed when the next version is out |
This PR resolves #8925. Classes that is generated from
moduleObject.mustache
now confirms JSONEncodable to send user-defined model asmultipart/form-data
. Please let me know if any issue to merge this PR. Thank you :)PR checklist
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*
.For Windows users, please run the script in Git BASH.
master
(5.3.0),6.0.x