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

bugfix type error in multipart-form payload #1863

Merged
merged 4 commits into from Sep 27, 2018
Merged

Conversation

@aya-eiya
Copy link
Contributor

@aya-eiya aya-eiya commented Sep 19, 2018

bugfix for #1862

type errors occur when set MultipartForm flag

@aya-eiya aya-eiya force-pushed the aya-eiya:master branch from 4dd4575 to 0e92fdd Sep 20, 2018
Copy link
Member

@raphael raphael left a comment

Thank you for the detailed report and the great PR! I left a couple of comments that would be good to address. Thanks again.

case design.NumberKind:
return "ParseFloat"
}
panic("undefined strconv function")

This comment has been minimized.

@raphael

raphael Sep 23, 2018
Member

Looking at the code calling this function is seems the type could be another kind? If that's true then we can't use panic here. Maybe return an empty string and test for it in the template (and skip code generation if empty string - the code should still compile but won't unmarshal the corresponding attribute).

@@ -451,6 +456,34 @@ func arrayAttribute(a *design.AttributeDefinition) *design.AttributeDefinition {
return a.Type.(*design.Array).ElemType
}

func valueTypeOf(pr string, att *design.AttributeDefinition) string {

This comment has been minimized.

@raphael

raphael Sep 23, 2018
Member

Please add a short header comment explaining the point of the function.

Copy link
Contributor Author

@aya-eiya aya-eiya left a comment

@raphael
I fixed the codes according to your comments. please review.
thank you.

return varName + ", (error)(nil)"
case design.ArrayKind:
case design.HashKind:
return valueTypeOf("", att) + "{}, (error)(nil)"

This comment has been minimized.

@aya-eiya

aya-eiya Sep 25, 2018
Author Contributor

I added an test case for an array has hash type elements, it finished green but
I found hash type in the array cause some runtime errors via type conversion.
I think those errors should not be targeted in this PR.
so I let those leave.

return varName + ", (error)(nil)"
case design.ArrayKind:
case design.HashKind:
return valueTypeOf("", att) + "{}, (error)(nil)"

This comment has been minimized.

@aya-eiya

aya-eiya Sep 25, 2018
Author Contributor

I did not support an Array-Array type as payload type but let it causes no error at runtime.

case design.HashKind:
return valueTypeOf("", att) + "{}, (error)(nil)"
}
return "(" + valueTypeOf("", att) + ")(nil), (error)(nil)"

This comment has been minimized.

@aya-eiya

aya-eiya Sep 25, 2018
Author Contributor

this line will not cause error, but this line has no sense.

@raphael
Copy link
Member

@raphael raphael commented Sep 27, 2018

Thank you, definitely an improvement compared to the original code!

@raphael raphael merged commit 3402d49 into goadesign:v1 Sep 27, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@raphael
Copy link
Member

@raphael raphael commented Sep 27, 2018

One last request please, could you port this fix to master as well? (cherry-pick the merge commit).

aya-eiya added a commit to aya-eiya/goa that referenced this pull request Sep 28, 2018
* bugfix type error in multipart-form payload

* bugfix Array attribute error in multipartform

* fix wrong comment line and add comment to new function

* fix fromString func to do not cause panic and support some other types for array element
@aya-eiya
Copy link
Contributor Author

@aya-eiya aya-eiya commented Sep 28, 2018

@raphael
I send a PR for master branch.
thank you.

raphael added a commit that referenced this pull request Sep 28, 2018
* bugfix type error in multipart-form payload

* bugfix Array attribute error in multipartform

* fix wrong comment line and add comment to new function

* fix fromString func to do not cause panic and support some other types for array element
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants