Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Begin isolating object creation code into an externalizable API. #1569

Merged
merged 13 commits into from
Apr 20, 2016
Merged

Begin isolating object creation code into an externalizable API. #1569

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
4c8afc2
Tidy up transformKeyValue
drew-gross Apr 19, 2016
16a56bf
Specialize transformKeyValue for object creation
drew-gross Apr 19, 2016
1641384
remove keys that never appear in creation requests
drew-gross Apr 19, 2016
6870cb9
rename function
drew-gross Apr 20, 2016
5681ab6
remove local var
drew-gross Apr 20, 2016
ec65663
early exit for simple keys
drew-gross Apr 20, 2016
7abfb5d
Refactor create
drew-gross Apr 20, 2016
11fc616
Force class creation when creating an object
drew-gross Apr 20, 2016
4dadfa6
Pass parameters to key value transformer
drew-gross Apr 20, 2016
17cdbcd
No need to check for array in this func
drew-gross Apr 20, 2016
ac3749b
start using Parse Format schema in MongoTransform
drew-gross Apr 20, 2016
3fceb22
Remove call to getExpectedType
drew-gross Apr 20, 2016
740187a
add tests to ensure client can't see _PushStatus
drew-gross Apr 20, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 29 additions & 13 deletions spec/MongoTransform.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,13 @@ var dummySchema = {
};


describe('parseObjectToMongoObject', () => {
describe('parseObjectToMongoObjectForCreate', () => {

it('a basic number', (done) => {
var input = {five: 5};
var output = transform.parseObjectToMongoObject(dummySchema, null, input);
var output = transform.parseObjectToMongoObjectForCreate(dummySchema, null, input, {
fields: {five: {type: 'Number'}}
});
jequal(input, output);
done();
});
Expand All @@ -37,7 +39,7 @@ describe('parseObjectToMongoObject', () => {
createdAt: "2015-10-06T21:24:50.332Z",
updatedAt: "2015-10-06T21:24:50.332Z"
};
var output = transform.parseObjectToMongoObject(dummySchema, null, input);
var output = transform.parseObjectToMongoObjectForCreate(dummySchema, null, input);
expect(output._created_at instanceof Date).toBe(true);
expect(output._updated_at instanceof Date).toBe(true);
done();
Expand All @@ -49,43 +51,53 @@ describe('parseObjectToMongoObject', () => {
objectId: 'myId',
className: 'Blah',
};
var out = transform.parseObjectToMongoObject(dummySchema, null, {pointers: [pointer]});
var out = transform.parseObjectToMongoObjectForCreate(dummySchema, null, {pointers: [pointer]},{
fields: {pointers: {type: 'Array'}}
});
jequal([pointer], out.pointers);
done();
});

it('a delete op', (done) => {
//TODO: object creation requests shouldn't be seeing __op delete, it makes no sense to
//have __op delete in a new object. Figure out what this should actually be testing.
notWorking('a delete op', (done) => {
var input = {deleteMe: {__op: 'Delete'}};
var output = transform.parseObjectToMongoObject(dummySchema, null, input);
var output = transform.parseObjectToMongoObjectForCreate(dummySchema, null, input);
jequal(output, {});
done();
});

it('basic ACL', (done) => {
var input = {ACL: {'0123': {'read': true, 'write': true}}};
var output = transform.parseObjectToMongoObject(dummySchema, null, input);
var output = transform.parseObjectToMongoObjectForCreate(dummySchema, null, input);
// This just checks that it doesn't crash, but it should check format.
done();
});

describe('GeoPoints', () => {
it('plain', (done) => {
var geoPoint = {__type: 'GeoPoint', longitude: 180, latitude: -180};
var out = transform.parseObjectToMongoObject(dummySchema, null, {location: geoPoint});
var out = transform.parseObjectToMongoObjectForCreate(dummySchema, null, {location: geoPoint},{
fields: {location: {type: 'GeoPoint'}}
});
expect(out.location).toEqual([180, -180]);
done();
});

it('in array', (done) => {
var geoPoint = {__type: 'GeoPoint', longitude: 180, latitude: -180};
var out = transform.parseObjectToMongoObject(dummySchema, null, {locations: [geoPoint, geoPoint]});
var out = transform.parseObjectToMongoObjectForCreate(dummySchema, null, {locations: [geoPoint, geoPoint]},{
fields: {locations: {type: 'Array'}}
});
expect(out.locations).toEqual([geoPoint, geoPoint]);
done();
});

it('in sub-object', (done) => {
var geoPoint = {__type: 'GeoPoint', longitude: 180, latitude: -180};
var out = transform.parseObjectToMongoObject(dummySchema, null, { locations: { start: geoPoint }});
var out = transform.parseObjectToMongoObjectForCreate(dummySchema, null, { locations: { start: geoPoint }},{
fields: {locations: {type: 'Object'}}
});
expect(out).toEqual({ locations: { start: geoPoint } });
done();
});
Expand Down Expand Up @@ -196,7 +208,9 @@ describe('transform schema key changes', () => {
var input = {
somePointer: {__type: 'Pointer', className: 'Micro', objectId: 'oft'}
};
var output = transform.parseObjectToMongoObject(dummySchema, null, input);
var output = transform.parseObjectToMongoObjectForCreate(dummySchema, null, input, {
fields: {somePointer: {type: 'Pointer'}}
});
expect(typeof output._p_somePointer).toEqual('string');
expect(output._p_somePointer).toEqual('Micro$oft');
done();
Expand All @@ -206,7 +220,9 @@ describe('transform schema key changes', () => {
var input = {
userPointer: {__type: 'Pointer', className: '_User', objectId: 'qwerty'}
};
var output = transform.parseObjectToMongoObject(dummySchema, null, input);
var output = transform.parseObjectToMongoObjectForCreate(dummySchema, null, input, {
fields: {userPointer: {type: 'Pointer'}}
});
expect(typeof output._p_userPointer).toEqual('string');
expect(output._p_userPointer).toEqual('_User$qwerty');
done();
Expand All @@ -219,7 +235,7 @@ describe('transform schema key changes', () => {
"Kevin": { "write": true }
}
};
var output = transform.parseObjectToMongoObject(dummySchema, null, input);
var output = transform.parseObjectToMongoObjectForCreate(dummySchema, null, input);
expect(typeof output._rperm).toEqual('object');
expect(typeof output._wperm).toEqual('object');
expect(output.ACL).toBeUndefined();
Expand Down
14 changes: 8 additions & 6 deletions src/Adapters/Storage/Mongo/MongoStorageAdapter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,14 +145,16 @@ export class MongoStorageAdapter {
// this adapter doesn't know about the schema, return a promise that rejects with
// undefined as the reason.
getOneSchema(className) {
return this.schemaCollection().then(schemasCollection => schemasCollection._fechOneSchemaFrom_SCHEMA(className));
return this.schemaCollection()
.then(schemasCollection => schemasCollection._fechOneSchemaFrom_SCHEMA(className));
}

// TODO: As yet not particularly well specified. Creates an object. Does it really need the schema?
// or can it fetch the schema itself? Also the schema is not currently a Parse format schema, and it
// should be, if we are passing it at all.
createObject(className, object, schema) {
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
Copy link
Contributor

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.

Copy link
Contributor Author

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.

// the schem only for the legacy mongo format. We'll figure that out later.
createObject(className, object, schemaController, parseFormatSchema) {
const mongoObject = transform.parseObjectToMongoObjectForCreate(schemaController, className, object, parseFormatSchema);
return this.adaptiveCollection(className)
.then(collection => collection.insertOne(mongoObject));
}
Expand Down
142 changes: 121 additions & 21 deletions src/Adapters/Storage/Mongo/MongoTransform.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,13 @@ var Parse = require('parse/node').Parse;
// validate: true indicates that key names are to be validated.
//
// Returns an object with {key: key, value: value}.
export function transformKeyValue(schema, className, restKey, restValue, options) {
options = options || {};

export function transformKeyValue(schema, className, restKey, restValue, {
inArray,
inObject,
query,
update,
validate,
} = {}) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

// Check if the schema is known since it's a built-in field.
var key = restKey;
var timeField = false;
Expand Down Expand Up @@ -62,7 +66,7 @@ export function transformKeyValue(schema, className, restKey, restValue, options
return {key: key, value: restValue};
break;
case '$or':
if (!options.query) {
if (!query) {
throw new Parse.Error(Parse.Error.INVALID_KEY_NAME,
'you can only use $or in queries');
}
Expand All @@ -75,7 +79,7 @@ export function transformKeyValue(schema, className, restKey, restValue, options
});
return {key: '$or', value: mongoSubqueries};
case '$and':
if (!options.query) {
if (!query) {
throw new Parse.Error(Parse.Error.INVALID_KEY_NAME,
'you can only use $and in queries');
}
Expand All @@ -91,7 +95,7 @@ export function transformKeyValue(schema, className, restKey, restValue, options
// Other auth data
var authDataMatch = key.match(/^authData\.([a-zA-Z0-9_]+)\.id$/);
if (authDataMatch) {
if (options.query) {
if (query) {
var provider = authDataMatch[1];
// Special-case auth data.
return {key: '_auth_data_'+provider+'.id', value: restValue};
Expand All @@ -100,7 +104,7 @@ export function transformKeyValue(schema, className, restKey, restValue, options
'can only query on ' + key);
break;
};
if (options.validate && !key.match(/^[a-zA-Z][a-zA-Z0-9_\.]*$/)) {
if (validate && !key.match(/^[a-zA-Z][a-zA-Z0-9_\.]*$/)) {
throw new Parse.Error(Parse.Error.INVALID_KEY_NAME,
'invalid key name: ' + key);
}
Expand All @@ -117,24 +121,24 @@ export function transformKeyValue(schema, className, restKey, restValue, options
(!expected && restValue && restValue.__type == 'Pointer')) {
key = '_p_' + key;
}
var inArray = (expected && expected.type === 'Array');
var expectedTypeIsArray = (expected && expected.type === 'Array');

// Handle query constraints
if (options.query) {
value = transformConstraint(restValue, inArray);
if (query) {
value = transformConstraint(restValue, expectedTypeIsArray);
if (value !== CannotTransform) {
return {key: key, value: value};
}
}

if (inArray && options.query && !(restValue instanceof Array)) {
if (expectedTypeIsArray && query && !(restValue instanceof Array)) {
return {
key: key, value: { '$all' : [restValue] }
};
}

// Handle atomic values
var value = transformAtom(restValue, false, options);
var value = transformAtom(restValue, false, { inArray, inObject });
if (value !== CannotTransform) {
if (timeField && (typeof value === 'string')) {
value = new Date(value);
Expand All @@ -150,7 +154,7 @@ export function transformKeyValue(schema, className, restKey, restValue, options

// Handle arrays
if (restValue instanceof Array) {
if (options.query) {
if (query) {
throw new Parse.Error(Parse.Error.INVALID_JSON,
'cannot use array as query param');
}
Expand All @@ -162,7 +166,7 @@ export function transformKeyValue(schema, className, restKey, restValue, options
}

// Handle update operators
value = transformUpdateOperator(restValue, !options.update);
value = transformUpdateOperator(restValue, !update);
if (value !== CannotTransform) {
return {key: key, value: value};
}
Expand Down Expand Up @@ -198,18 +202,114 @@ function transformWhere(schema, className, restWhere, options = {validate: true}
return mongoWhere;
}

const parseObjectKeyValueToMongoObjectKeyValue = (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not a function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a function?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an arrow function, they permeate this but I guess as it's top level it doesn't change anything

schema,
className,
restKey,
restValue,
parseFormatSchema
) => {
// Check if the schema is known since it's a built-in field.
let transformedValue;
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,
Copy link
Contributor

@flovilmart flovilmart Apr 20, 2016 •
edited
Loading

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor Author

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem

// just pass the _created_at through. Later, we should make sure the push status doesn't get transformed inside Parse Server.
case 'createdAt':
transformedValue = transformAtom(restValue, false);
coercedToDate = typeof transformedValue === 'string' ? new Date(transformedValue) : transformedValue
return {key: '_created_at', value: coercedToDate};
case 'updatedAt':
transformedValue = transformAtom(restValue, false);
coercedToDate = typeof transformedValue === 'string' ? new Date(transformedValue) : transformedValue
return {key: '_updated_at', value: coercedToDate};
case 'expiresAt':
transformedValue = transformAtom(restValue, false);
coercedToDate = typeof transformedValue === 'string' ? new Date(transformedValue) : transformedValue
return {key: 'expiresAt', value: coercedToDate};
case '_id': //TODO: for some reason, _PushStatus is already transformed when it gets here. For now,
// just pass the ID through. Later, we should make sure the push status doesn't get transformed inside Parse Server.
case '_rperm':
case '_wperm':
case '_email_verify_token':
case '_hashed_password':
case '_perishable_token': return {key: restKey, value: restValue};
case 'sessionToken': return {key: '_session_token', value: restValue};
default:
// Auth data should have been transformed already
if (restKey.match(/^authData\.([a-zA-Z0-9_]+)\.id$/)) {
throw new Parse.Error(Parse.Error.INVALID_KEY_NAME, 'can only query on ' + restKey);
}
// Trust that the auth data has been transformed and save it directly
if (restKey.match(/^_auth_data_[a-zA-Z0-9_]+$/)) {
return {key: restKey, value: restValue};
}
}
//skip straight to transformAtom for Bytes, they don't show up in the schema for some reason
if (restValue && restValue.__type !== 'Bytes') {
//Note: We may not know the type of a field here, as the user could be saving (null) to a field
//That never existed before, meaning we can't infer the type.
if (parseFormatSchema.fields[restKey] && parseFormatSchema.fields[restKey].type == 'Pointer' || restValue.__type == 'Pointer') {
restKey = '_p_' + restKey;
}
}

// Handle atomic values
var value = transformAtom(restValue, false, { inArray: false, inObject: false });
if (value !== CannotTransform) {
return {key: restKey, value: value};
}

// ACLs are handled before this method is called
// If an ACL key still exists here, something is wrong.
if (restKey === 'ACL') {
throw 'There was a problem transforming an ACL.';
}

// Handle arrays
if (restValue instanceof Array) {
value = restValue.map((restObj) => {
var out = transformKeyValue(schema, className, restKey, restObj, { inArray: true });
return out.value;
});
return {key: restKey, value: value};
}

// Handle update operators
value = transformUpdateOperator(restValue, true);
if (value !== CannotTransform) {
return {key: restKey, value: value};
}

// Handle normal objects by recursing
value = {};
for (var subRestKey in restValue) {
var subRestValue = restValue[subRestKey];
var out = transformKeyValue(schema, className, subRestKey, subRestValue, { inObject: true });
// For recursed objects, keep the keys in rest format
value[subRestKey] = out.value;
}
return {key: restKey, value: value};
}

// Main exposed method to create new objects.
// restCreate is the "create" clause in REST API form.
// Returns the mongo form of the object.
function parseObjectToMongoObject(schema, className, restCreate) {
function parseObjectToMongoObjectForCreate(schema, className, restCreate, parseFormatSchema) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this renaming? Would the forUpdate be any different?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. forUpdate would handle the atomic update operators like increment, pull, pullAll, addUnique, etc. For new object creation, those don't make any sense. Currently they are handled here by translating them to just field creation (eg. increment by 1 just becomes 1), but they probably shouldn't be. Willing to reconsider though, if you have opinions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for the sake of maintenance, maybe having the 2 code paths can introduce problems. Not sure about that. I feel that having separate transforms depending on the state of the object (create, update) may introduce other issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd imagine the transfromForUpdate code would first handle the atomic updates and then call transformForCreate with the remaining keys.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we lose performance throwing atomic operators to mongo on not existing object?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so. You would have to use update with upsert: true, to be able to do that, which requires a query. insert only supports documents, not update requests.

if (className == '_User') {
restCreate = transformAuthData(restCreate);
}
var mongoCreate = transformACL(restCreate);
for (var restKey in restCreate) {
var out = transformKeyValue(schema, className, restKey, restCreate[restKey]);
if (out.value !== undefined) {
mongoCreate[out.key] = out.value;
for (let restKey in restCreate) {
let { key, value } = parseObjectKeyValueToMongoObjectKeyValue(
schema,
className,
restKey,
restCreate[restKey],
parseFormatSchema
);
if (value !== undefined) {
mongoCreate[key] = value;
}
}
return mongoCreate;
Expand Down Expand Up @@ -933,7 +1033,7 @@ var FileCoder = {

module.exports = {
transformKey,
parseObjectToMongoObject,
parseObjectToMongoObjectForCreate,
transformUpdate,
transformWhere,
transformSelect,
Expand Down
Loading