msavin / Mongol-meteor-explore-minimongo-devtools

In-App MongoDB Editor for Meteor (Meteor DevTools)
http://play.with.meteor.toys
824 stars 40 forks source link

Does Mongol work with Autoform / Collection2 #19

Closed SantoshSrinivas79 closed 9 years ago

SantoshSrinivas79 commented 9 years ago

I am running into errors with Mongol with updating, etc.

Here is the test project:

git clone https://github.com/yogiben/meteor-starter.git
meteor add msavin:mongol

I can see mongol but then editing posts, etc doesn't work

msavin commented 9 years ago

Looks like it doesn't work. This is the error I found:

I20150225-20:18:26.351(-7)? Exception while invoking method 'Mongol_insert' Error: Owner is required I20150225-20:18:26.353(-7)? at getErrorObject (packages/aldeed:collection2/collection2.js:363:1) I20150225-20:18:26.353(-7)? at [object Object].doValidate (packages/aldeed:collection2/collection2.js:346:1) I20150225-20:18:26.353(-7)? at [object Object].Mongo.Collection.(anonymous function) as insert I20150225-20:18:26.353(-7)? at [object Object].Meteor.methods.Mongol_insert (packages/msavin:mongol/server/methods.js:70:1) I20150225-20:18:26.353(-7)? at maybeAuditArgumentChecks (packages/ddp/livedata_server.js:1599:1) I20150225-20:18:26.353(-7)? at packages/ddp/livedataserver.js:648:1 I20150225-20:18:26.354(-7)? at [object Object]..extend.withValue (packages/meteor/dynamics_nodejs.js:56:1) I20150225-20:18:26.354(-7)? at packages/ddp/livedataserver.js:647:1 I20150225-20:18:26.354(-7)? at [object Object]..extend.withValue (packages/meteor/dynamicsnodejs.js:56:1) I20150225-20:18:26.354(-7)? at [object Object]..extend.protocol_handlers.method (packages/ddp/livedata_server.js:646:1) I20150225-20:18:26.354(-7)? Sanitized and reported to the client as: Owner is required [400]

Never used Collection2, will have to check it out

krawalli commented 9 years ago

Thats because of validation (and so the combo of simple schema and collection2).

Simply: all to be saved data must be valid. @msavin: you can simply override the validation if those packages are installed. However be warned: if you override, you could easily inconsistent records (from a simple schema point of view)

JackAdams commented 9 years ago

Yep. @krawalli is right. This isn't a Mongol issue -- it's just simple schema doing its job. I recommend not overriding validation (simple schema is added to projects for a reason!) and closing this issue.

SantoshSrinivas79 commented 9 years ago

@JackAdams Just to make sure.

I cannot use Mongol for updating collections using Simple Schema. Correct? Ability to see if a great help in itself.

JackAdams commented 9 years ago

@fountainhead -- that's correct at the moment. You can duplicate and remove documents with Mongol + Simple Schema, but insert and update don't work. I'm working on a PR for Mongol right now to get them fixed too.

JackAdams commented 9 years ago

Okay, contrary to what I said earlier, I've just overridden validation to allow updates and inserts with simple schema and added that to the PR. This means that it's going to be up to the dev to make sure they don't stuff up their own development database.

krawalli commented 9 years ago

@JackAdams: Sorry, but that's a not completely correct answer.

In case of only simple schema it should work, since there is no automatic validation on saving (update/insert) a record. It is Collections2 that causes an automatic validation.

And to be sure: it also requires the to be saved into collection to be member of collection2 and simple-schema.

However: it could be added a settings to ignore validator's on demand. But there are more packages for validation than only simple schema & co

JackAdams commented 9 years ago

@krawalli you are, of course, correct about collection2 and simple-schema working in concert on inserts and updates. (And about other validators -- but as long as they don't wrap the insert and update methods, like collection2 does, we're okay.)

Now, I didn't write collection2, but had if (typeof SimpleSchema !== 'undefined' && _.isFunction(MongolCollection.simpleSchema)) { in the code, which obviously takes collection2 into account (not to mention that the option hash only works with collection2 added and would cause an error to be thrown if it wasn't present).

For the insert function, why was it changed to if (SimpleSchema !== undefined && _.isFunction(MongolCollection.simpleSchema)) { which will throw an error if SimpleSchema is undefined -- the typeof was important there! Also, why is the insert function now commented out? It was working both with and without simple-schema/collection2 when I submitted the PR, wasn't it?

And lastly, isn't typeof SimpleSchema !== 'undefined' functionally equivalent to !!Package['aldeed:simple-schema'] (although I do prefer your detection method as more robust, at least in theory).

I recommend putting the !!Package['aldeed:simple-schema'] into the insert method and un-commenting out. At the moment, people with simple-schema/collection2 (for the most part) won't be able to click the insert button and get the result they're expecting.

msavin commented 9 years ago

Hey @JackAdams, great points. I think @krawalli did that here: https://github.com/krawalli/Mongol/commit/90efe9d7d563943f7f9ab6103382dfff7e8b6780

JackAdams commented 9 years ago

Yeah, but only for the update method, the insert method still needs to be fixed up. If no-one else does it first, I'll put a PR together in a few hours time, after I've had time to read through @krawalli 's PR.

krawalli commented 9 years ago

@JackAdams:

I learned this by jslint and its error messages. Using typeof results in a String comparison, which may take somewhat longer than a direct comparison with type check aka triple =

AFAIK you should go for direct comparison whereever possible. In this special case it wont work: You are inside a function an the (non-existent) variable (SimpleSchema) does not exist. Using typeof this will ignore the non-existance. Using the direct way throws an error. My fault for not thinking of that before. As workaround you could go with a this.SimpleSchema - but as said before - we are inside a function and "this" context will just show all inner vars, no globals.

I've been before at this point and re-used the !!Package check - picked it up somewhere in someone else code - sorry, cannot name the credits-holder.

For the insert - overseen. Just did a quick fix while blocking Max from visiting the dev shop. It was preparation for his lightning talk :)

@msavin: well done!

JackAdams commented 9 years ago

Haha... nice on-the-fly fix @krawalli !

For the record, SimpleSchema is a global variable when it does exist, so using typeof to check for its existence totally works in this context. Just tested my original code again with aldeed:collection2 added and removed and it gave the expected results (and I did have some good old console.log()s in there to make sure it was going through the right functions in each case). But anyway, that's all a moot point as I like !!Package['...'] as a test better. (I'd seen that before somewhere but couldn't remember where and was too tired to go looking, so just used typeof SimpleSchema === 'undefined' instead.)

Now, it's about time I went and took a look at Max's lightning talk ...