-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Begin isolating object creation code into an externalizable API. #1569
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
Begin isolating object creation code into an externalizable API. #1569
Conversation
@@ -91,7 +91,7 @@ const requiredColumns = Object.freeze({ | |||
_Role: ["name", "ACL"] | |||
}); | |||
|
|||
const systemClasses = Object.freeze(['_User', '_Installation', '_Role', '_Session', '_Product']); | |||
const systemClasses = Object.freeze(['_User', '_Installation', '_Role', '_Session', '_Product', '_PushStatus']); |
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.
I recall discussing that change earlier with you and were advised not to put it there :)
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.
Yeah. At this point I think it's the best plan. _PushStatus in Parse.com is not a Parse Object, but in Parse Server it does seem to be a Parse Object, kinda. We don't want it to be changeable by clients though, maybe I should add a test for that. Making it visible in the dash and/or modifiable by master key seems fine though.
We definitely want to have the ability to store _PushStatus in a separate DB, so having it go through the database adapter is the right way to go.
Current coverage is
|
const mongoObject = transform.parseObjectToMongoObject(schema, className, object); | ||
// TODO: As yet not particularly well specified. Creates an object. Shouldn't need the | ||
// schemaController, but MongoTransform still needs it :( maybe shouldn't even need the schema, | ||
// and should infer from the type. Or maybe does need the schema for validations. Or maybe needs |
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.
from the adapter point of view, we should not need the schema, validation should be taken care of by the DBController.
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.
That is my thought as well. The legacy format needs to know if a field is a pointer though, because it saves them differently 😢 either way, later PR.
@drew-gross updated the pull request. |
let coercedToDate; | ||
switch(restKey) { | ||
case 'objectId': return {key: '_id', value: restValue}; | ||
case '_created_at'://TODO: for some reason, _PushStatus is already transformed when it gets here. For now, |
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.
we should update the created at from _PushStatus as it's a bug that emerged when I moved it to use the DBController instead of the direct mongo access.
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.
Huh? if _PushStatus is a Parse Object, it should have createdAt
when in Parse Format and _created_at
when in Mongo Format. It should never have _created_at
when inside Parse Server.
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.
that's what I'm saying :) originally it was a pure mongo object, then I updated to use the DBController and it passed review without changing that. I'll do a PR
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.
Ah OK. Your original note was a little confusing. Can you wait for this to merge, then do a single PR that fixes the bug and also removed the special casing and TODOs from this function?
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.
No problem
Reviewed in slack |
🎉 This change has been released in version 5.0.0-beta.1 |
🎉 This change has been released in version 5.0.0 |
This makes a copy of transformKeyValue, strips out all the parts not relevant to creating new objects, and uses that for
createObject
in the db adapter. It will still require some more tweaks later.