Move filename validation out of the Router and into the FilesAdaptor #6157

mpatnode · 2019-10-25T00:16:55Z

Didn't seem to make sense to me that the router would be deciding the syntax of my filenames. Specifically, I wanted to be able to create "directories" in AWS, and the router was rejecting the '/' character. Note, there is no behavior change in this code WRT how / is treated in a filename, because there is no AWS code here. That's a decision for the FilesAdaptor

src/Adapters/Files/FilesAdapter.js

src/Adapters/Files/GridFSBucketAdapter.js

src/Routers/FilesRouter.js

dplewis · 2019-10-25T02:10:28Z

@mpatnode Thanks for the PR!

Can you check travis and the failing tests?

codecov · 2019-10-25T18:15:00Z

Codecov Report

Merging #6157 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #6157      +/-   ##
==========================================
- Coverage   93.78%   93.75%   -0.03%     
==========================================
  Files         166      166              
  Lines       11271    11285      +14     
==========================================
+ Hits        10570    10580      +10     
- Misses        701      705       +4

Impacted Files	Coverage Δ
src/Adapters/Files/FilesAdapter.js	`100% <100%> (ø)`	⬆️
src/Adapters/Files/GridStoreAdapter.js	`46.37% <100%> (+0.78%)`	⬆️
src/Controllers/FilesController.js	`93.02% <100%> (+0.52%)`	⬆️
src/Adapters/Files/GridFSBucketAdapter.js	`68.85% <100%> (+0.51%)`	⬆️
src/Routers/FilesRouter.js	`92.72% <100%> (-0.26%)`	⬇️
src/Adapters/Storage/Mongo/MongoStorageAdapter.js	`92.27% <0%> (-0.71%)`	⬇️
src/RestWrite.js	`93.56% <0%> (-0.17%)`	⬇️
src/ParseServer.js	`97.59% <0%> (+0.05%)`	⬆️

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 f50f8be...3687a0e. Read the comment docs.

mpatnode · 2019-10-25T19:10:37Z

That looks like some sort of race condition with the Redis config. No?
https://travis-ci.org/parse-community/parse-server/builds/602954308?utm_medium=notification&utm_source=github_status

mpatnode · 2019-10-26T00:00:21Z

Hall happy now.

dplewis

Thanks for the PR! I added a few comments and nits. We'll review the s3 adapter PR shortly after.

src/Adapters/Files/FilesAdapter.js

dplewis · 2019-10-26T02:12:44Z

src/Adapters/Files/FilesAdapter.js

@@ -56,6 +59,37 @@ export class FilesAdapter {
 * @return {string} Absolute URL
 */
 getFileLocation(config: Config, filename: string): string {}
+
+ /** Validate a filename for this adaptor type (optional)1G


1G? I also see you are using adaptor instead of adapter. Not to nitpick but can you change it for consistency.

How embarrassing

src/Adapters/Files/FilesAdapter.js

dplewis · 2019-10-26T02:15:24Z

src/Adapters/Files/FilesAdapter.js

+ *
+ * @param {string} filename
+ *
+ * @returns {null|*|Parse.Error} null if there are no errors


@returns {null|Parse.Error} What does asterisk mean?

Intellij being over-zealous

src/Adapters/Files/FilesAdapter.js

dplewis · 2019-10-26T02:16:35Z

src/Adapters/Files/FilesAdapter.js

+ 'Filename contains invalid characters.'
+ );
+ }
+ return null; // No errors


Remove the comment.

src/Adapters/Files/GridStoreAdapter.js

src/Adapters/Files/FilesAdapter.js

mpatnode · 2019-10-26T21:33:16Z

@dplewis Process question: Do you prefer we squash when addressing PR issues, or wait till all review is complete?

dplewis · 2019-10-26T22:22:15Z

Good Job! There are 2 failing tests on travis, after that we will squash and merge this PR.

mpatnode · 2019-10-26T22:34:50Z

Yeah, remembered that was going to fail after I walked away.

mpatnode · 2019-10-26T23:06:29Z

postgres 😿

dplewis · 2019-10-26T23:10:23Z

@mpatnode I can pull this down and look into it

mpatnode · 2019-10-26T23:12:09Z

I think it's just a race condition in the test suite. Is it being ran randomized?

dplewis · 2019-10-27T01:54:54Z

@mpatnode The problem with the test lies here Postgres uses FS adapter for testing. Although the adapters don't extend FileAdapter.js. FileAdapter is still used for validation

…arse-community#6157) * Move filename validation out of the Router and into the FilesAdaptor * Address PR comments * Update unittests to handle FilesAdapter interface change * Make validateFilename optional

mpatnode commented Oct 25, 2019

View reviewed changes

src/Adapters/Files/FilesAdapter.js Show resolved Hide resolved

mpatnode commented Oct 25, 2019

View reviewed changes

src/Adapters/Files/FilesAdapter.js Show resolved Hide resolved

mpatnode commented Oct 25, 2019

View reviewed changes

src/Adapters/Files/GridFSBucketAdapter.js Show resolved Hide resolved

mpatnode commented Oct 25, 2019

View reviewed changes

src/Routers/FilesRouter.js Outdated Show resolved Hide resolved

mpatnode force-pushed the separation-of-concerns branch from 9cf3c4a to 37be59e Compare October 25, 2019 18:14

Move filename validation out of the Router and into the FilesAdaptor

efe12f4

mpatnode force-pushed the separation-of-concerns branch from 37be59e to efe12f4 Compare October 25, 2019 18:39

mpatnode marked this pull request as ready for review October 25, 2019 19:06

mpatnode mentioned this pull request Oct 26, 2019

Move filename validation into adaptor for AWS "directories". Add preserveFileName Support parse-community/parse-server-s3-adapter#76

Merged

dplewis suggested changes Oct 26, 2019

View reviewed changes

Address PR comments

afdd2d0

Update unittests to handle FilesAdapter interface change

0353b33

Make validateFilename optional

3687a0e

dplewis approved these changes Oct 27, 2019

View reviewed changes

dplewis merged commit 1c8d4a6 into parse-community:master Oct 27, 2019

yomybaby mentioned this pull request Oct 28, 2019

encodeURI vs. encodeURIComponent parse-community/parse-server-s3-adapter#75

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move filename validation out of the Router and into the FilesAdaptor #6157

Move filename validation out of the Router and into the FilesAdaptor #6157

mpatnode commented Oct 25, 2019

dplewis commented Oct 25, 2019

codecov bot commented Oct 25, 2019 •

edited

Loading

mpatnode commented Oct 25, 2019

mpatnode commented Oct 26, 2019

dplewis left a comment •

edited

Loading

dplewis Oct 26, 2019

mpatnode Oct 26, 2019

dplewis Oct 26, 2019 •

edited

Loading

mpatnode Oct 26, 2019

dplewis Oct 26, 2019

mpatnode Oct 26, 2019

mpatnode commented Oct 26, 2019

dplewis commented Oct 26, 2019

mpatnode commented Oct 26, 2019

mpatnode commented Oct 26, 2019

dplewis commented Oct 26, 2019

mpatnode commented Oct 26, 2019

dplewis commented Oct 27, 2019

Move filename validation out of the Router and into the FilesAdaptor #6157

Move filename validation out of the Router and into the FilesAdaptor #6157

Conversation

mpatnode commented Oct 25, 2019

dplewis commented Oct 25, 2019

codecov bot commented Oct 25, 2019 • edited Loading

Codecov Report

mpatnode commented Oct 25, 2019

mpatnode commented Oct 26, 2019

dplewis left a comment • edited Loading

Choose a reason for hiding this comment

dplewis Oct 26, 2019

Choose a reason for hiding this comment

mpatnode Oct 26, 2019

Choose a reason for hiding this comment

dplewis Oct 26, 2019 • edited Loading

Choose a reason for hiding this comment

mpatnode Oct 26, 2019

Choose a reason for hiding this comment

dplewis Oct 26, 2019

Choose a reason for hiding this comment

mpatnode Oct 26, 2019

Choose a reason for hiding this comment

mpatnode commented Oct 26, 2019

dplewis commented Oct 26, 2019

mpatnode commented Oct 26, 2019

mpatnode commented Oct 26, 2019

dplewis commented Oct 26, 2019

mpatnode commented Oct 26, 2019

dplewis commented Oct 27, 2019

codecov bot commented Oct 25, 2019 •

edited

Loading

dplewis left a comment •

edited

Loading

dplewis Oct 26, 2019 •

edited

Loading