🐛 Fixed uppercase file extensions ignored in content import #14268

SimonBackx · 2022-03-04T13:04:08Z

refs https://github.com/TryGhost/Team/issues/1363

When uploading a zip of images in Settings > Labs > [Import], it will skip images that have an uppercase extension, citing an 'unsupported file type' error.
Glob ignored those files when matching extensions in ImportManager
Added nocase option where needed
Extended tests to also test the processZip method of ImportManager with getFilesFromZip
Added isValidZip for zip with uppercase image
Cleaned up JSDoc in ImportManager, and replaced some older JS syntax

Fixed zipContainsMultipleDataFormats error never thrown:

When doing some refactoring on the processZip method of ImportManager in an effort to increase the test branches coverage, I discovered that the zipContainsMultipleDataFormats error is never thrown.

The promise error was only returned in an _.each loop, but never thrown
Previously when combining multiple data types in a zip file, no error got thrown
Added a test for this error
Also added a test for noContentToImport error

Other errors and fixes:

Added missing length in getBaseDirectory check
getContentTypes fixed (returned duplicate values). Type error came up after adding all JSDocs
updated tests to match real types from JSDoc and pass type validations
Rewrote some methods in the async await syntax
Added tests for ImportManager clean up

codecov · 2022-03-04T13:05:29Z

Codecov Report

Merging #14268 (66acfc3) into main (db6f174) will increase coverage by 0.18%.
The diff coverage is 97.36%.

@@            Coverage Diff             @@
##             main   #14268      +/-   ##
==========================================
+ Coverage   55.45%   55.64%   +0.18%     
==========================================
  Files         565      565              
  Lines       46883    46931      +48     
  Branches     3898     3906       +8     
==========================================
+ Hits        26001    26115     +114     
+ Misses      20843    20777      -66     
  Partials       39       39

Impacted Files	Coverage Δ
core/server/data/importer/import-manager.js	`97.76% <97.36%> (+8.79%)`	⬆️
core/server/data/importer/handlers/image.js	`97.95% <0.00%> (+65.30%)`	⬆️

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 db6f174...66acfc3. Read the comment docs.

SimonBackx · 2022-03-07T09:43:12Z

Unit Tests are currently failing only in Node 12 due to a weird coverage bug in that Node version.

refs https://ghost.slack.com/archives/C02G9E68C/p1646402101912059

SimonBackx · 2022-03-07T14:51:05Z

Coverage issue is solved with the tests. This is ready for review.

naz

The changes are great. Really like the "boyscout" cleanup and the async/await rewrites. Makes future maintenance much more pleasant!
Something I'm also guilty of and wanted to point out here - would be awesome to remember to make the change that fixes the actual problem as atomic as possible. For example, in this PR would make the change adding nocase: true as a one-liner with a minimal test case showing the root problem in one short commit. Going minimal allows the reviewer to see clearer the exact problem and would allow to merge/cherry-pick that changes into main waaaay faster 😜

naz · 2022-03-10T02:44:43Z

core/server/data/importer/import-manager.js

-        });
+    async preProcess(importData) {
+        for (const importer of this.importers) {
+            importData = importer.preProcess(importData);


I'm slightly confused by this substitution. It seems like we used to return a result of pipeline(ops) which is Promise<[any, any...]> (reference)

With just an assignment operation here we loose the "array" and resolve with a single object.

What I suspect has been misleading here from the very beginning is the * @returns {Promise<ImportData>} signature. It should've been * @returns {Promise<ImportData[]>} 🤔

Hmm, yes indeed! Something is wrong here. But if you look at the implementation of importFromFile, you can see that the output of preProcess is passed to doImport, where it certainly is not expecting an array (is is being used inside it as importData[importer.type]).

If you look at the implementation of pipeline, there is a Promise.reduce also at the end. I don't think it returns an array, but the type checker of my IDE (vscode) is showing the wrong return type (might be worth adding some JSDocs). I think it returns the last return value from all the tasks.

@naz Can you check this again? I do think that the change is okay here. Also note that importer.preProcess never returns a promise (that is why I also removed that async behaviour).

Yup. Just checked and the preProcess does return a single object. This piece of code is confusing! Let's get the PR going 🚀

core/server/data/importer/import-manager.js

naz

This looks pretty good. Let's get it out there!

refs https://github.com/TryGhost/Team/issues/1363 - When uploading a zip of images in Settings > Labs > [Import], it will skip images that have an uppercase extension, citing an 'unsupported file type' error. - Glob ignored those files when matching extensions in ImportManager - Added nocase option where needed - Extended tests to also test the processZip method of ImportManager with getFilesFromZip - Added isValidZip for zip with uppercase image - Cleaned up JSDoc a tiny bit in ImportManager

no issue Reason: lots of type errors in this file - Added JSDocs - Removed bluebird - Replaced some hard to follow code to make it easier to understand - importer.preProcess doesn't return promises -> removed pipeline - Replaced some older JS syntax Found an issue with getContentTypes returning the same content type twice - Fixed the issue - Updated wrong test for this method

no issue Because of the new JSDocs in ImportManager, some errors came up in the type checks

no issue When doing some refactoring on the processZip method of ImportManager in an effort to increase the test branches coverage. I discovered this bug when rewriting the older JS syntax. - The promise error was only returned in an _.each loop, but never thrown - Previously when combining images and data in a zip file, no error got thrown - Added a test for this error - Also added a test for noContentToImport error

SimonBackx force-pushed the fix/zip-import-uppercase-extensions branch from b0d7388 to 38f3e84 Compare March 4, 2022 13:40

SimonBackx requested a review from matthanley March 7, 2022 09:47

SimonBackx force-pushed the fix/zip-import-uppercase-extensions branch from db7f14d to 71f11e7 Compare March 7, 2022 13:48

matthanley requested a review from naz March 9, 2022 16:28

naz suggested changes Mar 10, 2022

View reviewed changes

SimonBackx requested a review from naz March 10, 2022 09:41

naz approved these changes Mar 10, 2022

View reviewed changes

matthanley approved these changes Mar 10, 2022

View reviewed changes

SimonBackx added 8 commits March 11, 2022 09:00

Added missing length in getBaseDirectory check

b38c742

Fixed type errors in import-manager tests

6e34863

no issue Because of the new JSDocs in ImportManager, some errors came up in the type checks

Made cleanUp method async in ImportManager

7dd934c

Added new tests to ImportManager

ccad0c1

Removed await for cleanUp when importing file

66acfc3

SimonBackx force-pushed the fix/zip-import-uppercase-extensions branch from ae18773 to 66acfc3 Compare March 11, 2022 08:00

SimonBackx merged commit 42ac8c4 into TryGhost:main Mar 11, 2022

SimonBackx deleted the fix/zip-import-uppercase-extensions branch March 11, 2022 08:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Fixed uppercase file extensions ignored in content import #14268

🐛 Fixed uppercase file extensions ignored in content import #14268

SimonBackx commented Mar 4, 2022 •

edited

Loading

codecov bot commented Mar 4, 2022 •

edited

Loading

SimonBackx commented Mar 7, 2022

SimonBackx commented Mar 7, 2022

naz left a comment •

edited

Loading

naz Mar 10, 2022

SimonBackx Mar 10, 2022

SimonBackx Mar 10, 2022

naz Mar 10, 2022

naz left a comment

🐛 Fixed uppercase file extensions ignored in content import #14268

🐛 Fixed uppercase file extensions ignored in content import #14268

Conversation

SimonBackx commented Mar 4, 2022 • edited Loading

Fixed zipContainsMultipleDataFormats error never thrown:

Other errors and fixes:

codecov bot commented Mar 4, 2022 • edited Loading

Codecov Report

SimonBackx commented Mar 7, 2022

SimonBackx commented Mar 7, 2022

naz left a comment • edited Loading

Choose a reason for hiding this comment

naz Mar 10, 2022

Choose a reason for hiding this comment

SimonBackx Mar 10, 2022

Choose a reason for hiding this comment

SimonBackx Mar 10, 2022

Choose a reason for hiding this comment

naz Mar 10, 2022

Choose a reason for hiding this comment

naz left a comment

Choose a reason for hiding this comment

SimonBackx commented Mar 4, 2022 •

edited

Loading

codecov bot commented Mar 4, 2022 •

edited

Loading

naz left a comment •

edited

Loading