-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Move mongo schema format related logic into mongo adapter #1385
Conversation
098b15e
to
d9dd991
Compare
d9dd991
to
0d5a1e8
Compare
@@ -776,7 +728,7 @@ function buildMergedSchemaObject(mongoObject, putRequest) { | |||
} | |||
var fieldIsDeleted = putRequest[oldField] && putRequest[oldField].__op === 'Delete' | |||
if (!fieldIsDeleted) { | |||
newSchema[oldField] = mongoFieldTypeToSchemaAPIType(mongoObject[oldField]); | |||
newSchema[oldField] = MongoSchemaCollection._DONOTUSEmongoFieldToParseSchemaField(mongoObject[oldField]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Current coverage is
|
Changed my mind about this being WIP, since it passed tests and the horrible parts mostly got removed already before I submitted. |
// createdAt and updatedAt are wacky and have legacy baggage | ||
parseFormatSchema.createdAt = { type: 'String' }; | ||
parseFormatSchema.updatedAt = { type: 'String' }; | ||
this.data[schema.className] = _.mapValues(parseFormatSchema, parseField => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we really need to add lodash for 1 function?
Object.keys(parseFormatSchema).map( key => {
let parseField = parseFormatSchema[key];
return schemaAPITypeToMongoFieldType(parseField).result
})
1 small nit, not really into adding an extra lib if it's just for one function. |
We already indirectly depend on lodash anyway, and there are also a lot of other things it will be useful for like set intersection, set union, deep flatten, deep clone, etc. If you guys don't want lodash we can remove it but I think it's a pretty sweet utility library and will be able to simplify a lot of stuff for us. Also I think you misunderstand mapValues, it's more like
So it can replace nearly all of our uses of |
Alright then, yes this is very useful, I usually use it. |
return response; | ||
} | ||
|
||
const defaultCLPS = Object.freeze({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't belong here. Mongo should have 0 clue on what is a CLP in my opinion, since it's purely top level abstraction in Parse world.
Also, if we are going to have this on other database adapter - do we probably would need to go copy/paste at this point.
A single comment about default CLPs, since I don't think that's the proper place for them. |
Yeah I think probably the best way to store schema metadata like CLPs + pointer permissions + future features is probably to just give the adapter an opaque blob and expect it to return the same blob back. It's worth having a larger discussion about, though, I think. I'm going to merge this anyway, and refactor that later. |
Soon, Parse Server will have no idea what the mongo format looks like. This PR is a work in progress, there are some tests that fail locally that seems unrelated to my changes, I want to see if they fail in Travis. Also it needs some cleanup.