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

Fix content length parameter for InlineDataPart #653

Merged
merged 5 commits into from
Jul 3, 2019

Conversation

ASKabanets
Copy link

@ASKabanets ASKabanets commented Jun 26, 2019

Description

Hi Fuel team, right now we are using your library as main networking tool and we have several multipart data request for uploading images.
What I have found, that in your InlineDataPart you are using content.length.toLong() which is not right. For example, you can have emoji in your content and measuing size of the DataPart by string length doesn't seem proper.
So instead I changed measuring size of the InlineDataPart to content.toByteArray().size

I didn't find a issue related to this problem and created pull request right away. I hope you will find and useful and merge into the master, because its really crucial for us.

Type of change

Check all that apply

  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactoring (a change which changes the current internal or external interface)
  • This change requires a documentation update

How Has This Been Tested?

Tested it with several strings with emoji.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation, if necessary
  • My changes generate no new compiler warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Inspect the bytecode viewer, including reasoning why

@codecov
Copy link

codecov bot commented Jun 26, 2019

Codecov Report

Merging #653 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #653   +/-   ##
=========================================
  Coverage     71.07%   71.07%           
  Complexity      297      297           
=========================================
  Files            57       57           
  Lines          1490     1490           
  Branches        193      193           
=========================================
  Hits           1059     1059           
  Misses          339      339           
  Partials         92       92
Impacted Files Coverage Δ Complexity Δ
.../kotlin/com/github/kittinunf/fuel/core/DataPart.kt 56.36% <100%> (ø) 0 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 58ac08b...43f13ae. Read the comment docs.

@SleeplessByte
Copy link
Collaborator

SleeplessByte commented Jun 26, 2019

content.toByteArray().size

Does this take in account the target encoding?

(Thank you for working on this ❤️)

@SleeplessByte SleeplessByte added the 🐛 bug Something isn't working label Jun 26, 2019
Repository owner deleted a comment from codecov bot Jun 27, 2019
@ASKabanets
Copy link
Author

@SleeplessByte Its a good question, I updated code so now it takes in account encoding

@SleeplessByte
Copy link
Collaborator

What I have found, that in your InlineDataPart you are using content.length.toLong() which is not right. For example, you can have emoji in your content and measuing size of the DataPart by string length doesn't seem proper.

It would be great if you could add one test case that shows this (have it have emojis and test the length, the old code should be incorrect and the new code should be correct).

Would you be able to do that?

@iNoles iNoles merged commit 7aa0454 into kittinunf:master Jul 3, 2019
@kittinunf kittinunf mentioned this pull request Jul 22, 2019
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants