search

yitsushi / go-misskey

Misskey Go SDK
GNU General Public License v3.0
51 stars 12 forks source link

Update create.go #82

Closed itsalltoast closed 3 years ago

itsalltoast commented 3 years ago

Having MultiPartRequest.Request passed in as a pointer caused a crash in core.parseMultipartFields() [core/multipart.go:18].

Removing the pointer and embedding the object directly fixes the crash.

Requirements for Contributing

Description of the Change

Un-pointerize a data field that is causing a client crash due to incorrect Go reflection.

Issue or RFC

Issue #81

Alternate Designs

It's possible to add more safety to the reflection in parseMultipartFields() as an alternative fix to this, but I'm not sure that would be better than just ensuring pointers are not passed in.

Possible Drawbacks

Increased stack memory usage.

Verification Process

Ran a test with and without this fix to confirm it corrects the crash.

Release Notes

yitsushi commented 3 years ago

Makes sense and looks good, but I have to look into it a bit deeper. I think I had a reason to use pointers for that as I try to not use them if not necessary especially when it's part of an outer abstract layer.

coveralls commented 3 years ago

Coverage Status

Coverage remained the same at 79.036% when pulling b383ea513eede121900750aa025b6f25c382b36a on itsalltoast:main into 79312a47e16b7a155a30a9123dc78eb7ed6ae66e on yitsushi:main.

yitsushi commented 3 years ago

Thank you for opening this PR. I did a bit or digging around and decided to take a different approach with #83 .

Reason (mostly described in the PR): Fixing in createFile does not fix the issue under the hood and would fix only those two endpoints. We have only 2 endpoints with form data, but we can't guarantee we will not add new ones later. Other endpoint implementations are using pointers, therefore if someone jumps in and tries to implement a new one, but with MultipartRequest, there is a high chance they will try to do it with pointers as all other endpoints are using pointers.