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: creating a Parse.File with base64 string fails for some file types #1467

Merged
merged 20 commits into from
May 2, 2022

Conversation

RazvanCristian
Copy link
Contributor

@RazvanCristian RazvanCristian commented Mar 29, 2022

New Pull Request Checklist

Issue Description

Related issue: #1466

Approach

Reworked logic for base64 formatting in ParseFile.js.

TODOs before merging

  • Add tests
  • Add entry to changelog

@parse-github-assistant
Copy link

parse-github-assistant bot commented Mar 29, 2022

Thanks for opening this pull request!

  • 🎉 We are excited about your hands-on contribution!

@RazvanCristian RazvanCristian changed the title Fix base64 formatting fix: base64 formatting Mar 29, 2022
changelogs/CHANGELOG_release.md Outdated Show resolved Hide resolved
src/__tests__/ParseFile-test.js Outdated Show resolved Hide resolved
@RazvanCristian RazvanCristian changed the title fix: base64 formatting fix: extract data from base64 with a filename parameter Mar 31, 2022
@RazvanCristian RazvanCristian requested a review from mtrezza March 31, 2022 15:30
src/ParseFile.js Outdated Show resolved Hide resolved
src/__tests__/ParseFile-test.js Show resolved Hide resolved
@RazvanCristian RazvanCristian requested a review from mtrezza March 31, 2022 17:47
src/ParseFile.js Outdated Show resolved Hide resolved
Copy link
Member

@mtrezza mtrezza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests failing, please request another review from @parse-community/js-sdk-review when this is ready.

@RazvanCristian
Copy link
Contributor Author

Tests failing, please request another review from @parse-community/js-sdk-review when this is ready.

Tests are failing because you're using node 14.19.1, while the current LTS version is node 16.14.2.
image
||= operator was added in 15.0.0. Logical OR assignment (||=)

@mtrezza
Copy link
Member

mtrezza commented Apr 4, 2022

Can you adapt the code to pass? We won't change the node engine version at this point. Node 12 and 14 are still under LTS.

@RazvanCristian
Copy link
Contributor Author

RazvanCristian commented Apr 4, 2022

Done.

@RazvanCristian RazvanCristian requested a review from mtrezza April 4, 2022 17:32
@mtrezza mtrezza changed the title fix: extract data from base64 with a filename parameter fix: creating a Parse.File with base64 string fails for some file types Apr 9, 2022
Copy link
Member

@mtrezza mtrezza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests still fail, could you take a look? And please let me know if the new PR title is correct.

@RazvanCristian
Copy link
Contributor Author

Tests still fail, could you take a look? And please let me know if the new PR title is correct.

Sorry for taking so long, I had some work to do.
I updated the PR and checked that it passes the tests with version v14.19.1. It passes all the tests and there should be no other problems.

@mtrezza
Copy link
Member

mtrezza commented Apr 18, 2022

I changed the PR title because the previous one was difficult to read. It seems you changed it back. What's incorrect or missing for the title:

fix: creating a Parse.File with base64 string fails for some file types

@RazvanCristian RazvanCristian changed the title fix: wrong processing of a base64 string in accordance to RFC 2397 when calling new Parse.File fix: creating a Parse.File with base64 string fails for some file types Apr 20, 2022
@codecov
Copy link

codecov bot commented Apr 20, 2022

Codecov Report

Merging #1467 (551b0a2) into alpha (62dc989) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##            alpha    #1467   +/-   ##
=======================================
  Coverage   99.94%   99.94%           
=======================================
  Files          61       61           
  Lines        5948     5949    +1     
  Branches     1354     1357    +3     
=======================================
+ Hits         5945     5946    +1     
  Misses          3        3           
Impacted Files Coverage Δ
src/ParseFile.js 100.00% <100.00%> (ø)

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 b90ae25...551b0a2. Read the comment docs.

src/ParseFile.js Outdated Show resolved Hide resolved
@RazvanCristian RazvanCristian requested a review from mtrezza April 28, 2022 19:34
Copy link
Member

@mtrezza mtrezza left a 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! Thanks for this fix, let's just wait for the CI to pass.

@mtrezza
Copy link
Member

mtrezza commented Apr 28, 2022

@rubenatorio Do you want to do a final review before we merge?

@mtrezza mtrezza merged commit c07d6c9 into parse-community:alpha May 2, 2022
parseplatformorg pushed a commit that referenced this pull request May 2, 2022
## [3.4.3-alpha.1](3.4.2...3.4.3-alpha.1) (2022-05-02)

### Bug Fixes

* creating a Parse.File with base64 string fails for some file types ([#1467](#1467)) ([c07d6c9](c07d6c9))
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 3.4.3-alpha.1

@parseplatformorg parseplatformorg added the state:released-alpha Released as alpha version label May 2, 2022
parseplatformorg pushed a commit that referenced this pull request Jun 18, 2022
## [3.4.3-beta.1](3.4.2...3.4.3-beta.1) (2022-06-18)

### Bug Fixes

* creating a Parse.File with base64 string fails for some file types ([#1467](#1467)) ([c07d6c9](c07d6c9))
* invalid name for `Parse.Role` throws incorrect error ([#1481](#1481)) ([8326a6f](8326a6f))
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 3.4.3-beta.1

@parseplatformorg parseplatformorg added the state:released-beta Released as beta version label Jun 18, 2022
parseplatformorg pushed a commit that referenced this pull request Jul 2, 2022
## [3.4.3](3.4.2...3.4.3) (2022-07-02)

### Bug Fixes

* creating a Parse.File with base64 string fails for some file types ([#1467](#1467)) ([c07d6c9](c07d6c9))
* invalid name for `Parse.Role` throws incorrect error ([#1481](#1481)) ([8326a6f](8326a6f))
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 3.4.3

@parseplatformorg parseplatformorg added the state:released Released as stable version label Jul 2, 2022
@Saquib764
Copy link

It seems there is still issue with Parse.File while creating base64 file.

I am generating data URL from canvas-

let url = canvas.toDataURL("image/png");

const file = new Parse.File(`shading_texture.png`, { base64: url.split('base64,')[1] });
await file.save()

Upon running this script, the frontend freezes for a couple of minutes and then fails.

The exact same script was working with version 3.4.2

I also tried the following script to extract base64 string. No success.

let base64 = url.replace('data:', '').replace(/^.+,/, '')

@mtrezza
Copy link
Member

mtrezza commented Jul 8, 2022

Could you open a new issue?

@samateja
Copy link

this fix is not working when i upload a video and convert the video to base64. The file size is around 17mb, do you suggest any other solution?

@FransGH
Copy link

FransGH commented Aug 18, 2022

See #1532

jpgupta pushed a commit to jpgupta/Parse-SDK-JS-UUID-Update that referenced this pull request Oct 24, 2022
## [3.4.3](parse-community/Parse-SDK-JS@3.4.2...3.4.3) (2022-07-02)

### Bug Fixes

* creating a Parse.File with base64 string fails for some file types ([parse-community#1467](parse-community#1467)) ([c07d6c9](parse-community@c07d6c9))
* invalid name for `Parse.Role` throws incorrect error ([parse-community#1481](parse-community#1481)) ([8326a6f](parse-community@8326a6f))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state:released Released as stable version state:released-alpha Released as alpha version state:released-beta Released as beta version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong processing of a base64 string in accordance to RFC 2397 when calling new Parse.File
7 participants