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.

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

Added support for Array property type; force update of composite properties; added support for non-ObjectID IDs. #387

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

rafaelkaufmann
Copy link
Contributor

I know it's bad form to put two different features on the same pull request, but they're both rather short, so.

Up until now, Array wasn't a valid property type -- something that was sorely missing for MongoDB-backed models. In order to make this possible, I had to make some substantial changes to the MongoDB driver, in particular the assumptions it makes about the changes parameter in update operations. Caution is required!

Property.js seemed to be the only module that cared about the type of properties, but maybe something else needs to be changed. That said, everything works normally.

As I added the Array type, I realized my updates to Array-typed properties weren't getting persisted. This goes down to the treatment of property changes on the custom setter. Thus obj.arrayProp = ['foo'] would work, but something like obj.arrayProp[0] = 'foo' would go unnoticed by ORM. (Obviously, this is also an issue for Object-typed properties.)

Rather than attempting to overload all possible methods that could cause changes to these properties, I forced savePersisted to update all Array- or Object-typed properties.

This sidesteps a bit of a pressing issue: as I've mentioned previously, ideally there should be support for MongoDB's atomic $push/$pull/etc operators on array properties (currently, the driver assumes $set). Clearly this is hard, but it might be worth it (and the lowly CakePHP driver does it, so...).

Finally, I've added support for having ids on MongoDB-backed instances that are NOT instances of mongodb.ObjectID. This is obviously undesirable but (unfortunately) occurs on one of my existing applications, so there it is. :)

@rafaelkaufmann
Copy link
Contributor Author

@spartan563 , is this what you had in mind? (Grumpy mode on: this would have been tremendously easier if these functions could directly access driver.config...)

@rafaelkaufmann
Copy link
Contributor Author

BTW, I did test it with my app and it did have the desired behavior. However, you're right that it's still missing the corresponding fixes for the hasMany accessor methods, which my app does not use.

@rafaelkaufmann
Copy link
Contributor Author

I can't even get npm test to run locally :/

As for your recommended change, there's a slight problem: when passing the DB config in URL format (which is what the db.js tests do, incidentally), a query string such as ...?forceObjectID=false&... would get parsed as {forceObjectID: "false"}, which is obviously not what we want.

I'd patch the relevant code in ORM.js to treat this case, but it would be hacky (there is parseFloat, but no parseBool):

var value = opts.query[k];

if (value.toLowerCase() === 'false') {
    opts[k] = false;
} else if (value.toLowerCase() === 'true') {
    opts[k] = true;
} else if (var x = parseFloat(value)) {
    opts[k] = x;
} else {
    opts[k] = value;
}

@notheotherben
Copy link
Collaborator

Yeah, there are a few tricks that I usually apply to get it running for myself. I've always had issues with the sqlite3 driver on my machine (Windows) so I make the following changes:

test/common.js

I change process.env.ORM_PROTOCOL to have a default value (whatever engine I'm testing on).

common.protocol = function () {
    return process.env.ORM_PROTOCOL || 'mongodb';
};

test/integration/orm_exports.js

I comment out the sqlite3 require.

///var sqlite = require('sqlite3');

Ah, I see what you mean. I don't see any problems with implementing the parsing logic you've shown - seems like it's the most straightforward way to handle it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants