Properly allow fields of type 'File' within _GlobalConfig#3458
Properly allow fields of type 'File' within _GlobalConfig#3458mross22 wants to merge 4 commits intoparse-community:masterfrom
Conversation
|
Looks like the test failures are unrelated to my change and there is a PR out to improve test flakiness, so I will push again later to re-trigger the build |
|
It would be appreciated if you could also add support for GeoPoints (they have the same issue). Also, could you add a test to https://github.com/ParsePlatform/parse-server/blob/master/spec/ParseGlobalConfig.spec.js that adds a file/geopoint so that further rewrites don't cause this fix to disappear. |
|
@aontas sure, ill make the update for GeoPoint as well and update the PR. I added a test for the file that I touched because I noticed that if I tried to repro the issue through ParseGlobalConfig.spec.js it didn't repro (so thus my test to demonstrate the failure passed from the start). I didn't have a chance to look into it too deeply but i assume there is some difference in the testing case (or in the adapter) that differs from the prod behavior. |
|
@mross22 updated the pull request - view changes |
| return GeoPointCoder.JSONToDatabase(atom); | ||
| // normally we transform a 'GeoPoint' to just the name when storing to the db, but | ||
| // _GlobalConfig has no schema so we must preserve type information and other fields | ||
| return className === "_GlobalConfig" ? atom : GeoPointCoder.JSONToDatabase(atom); |
There was a problem hiding this comment.
I'm really not a big fan of using that className hard coded like that in this method that should know nothing about the className.
There was a problem hiding this comment.
sure, at the very least I can move this logic outside of this function but I get its still not ideal to have this component care about the className
There was a problem hiding this comment.
yup... Try moving it outside and see how it goes :)
There was a problem hiding this comment.
|
@mross22 thanks for getting onto that problem, however at the moment, I believe the implementation is not bullet proof, we'll need something else. I'll have a look later today for a proper suggestion of the implementation. |
|
@mross22 updated the pull request - view changes |
|
@mross22 Thanks for the PR, actually I found the real issue behind it. The Config object is stored with a _Schema with only one property, Hoewever, those 'keys', were considered 'topLevel' which is bad, as they clearly are NOT. (they's interior keys). Interior keys do not suffer transformation, therefore the fix is cleaner and has a larger scope. There was actually a bug that would affect all updates using the dot notation. Thanks for your PR however, that helped a lot identifying the root of the issue :) I've opened #3531 and we can expect a merge and a release soon. |
5 participants
Fixes #3457