JackAdams / meteor-editable-text

Drop-in editable text widget for meteor
http://editable-text-demo.taonova.com
MIT License
59 stars 14 forks source link

Bypass simpleSchema / collection 2 validation #64

Closed 0xpatrickdev closed 8 years ago

0xpatrickdev commented 8 years ago

Hey, big fan of the package so far. I am trying integrate it with meteor-file-collection, but am running into some issues. Long story short, I can't attach a schema to a gridFS collection, but need to have the simple-schema and collection2 packages in my app for other functions.

When I try to make an update, I get an error saying, Exception while simulating the effect of invoking '_editableTextWrite' TypeError: Cannot read property 'simpleSchema' of undefined.

On line 187 of the editable_text_common.js file, there's a variable that checks to see if simple-schema and collection2 are installed, and if Collection.simpleSchema and Collection._c2 are valid functions. In the instance where these packages are installed but a schema is not defined, Collection.simpleSchema and Collection._c2 will return as valid functions, even though the results are 'null' and 'undefined', respectively. I think this sets c2optionsHashRequired to true, even though it really shouldn't be.

Do you think you could modify the logic in line 187 so that Collection.simpleSchema --> null does not return a false positive? Could be wrong, but not sure if _.isFunction is the best approach given the instance described above. Or maybe add a field the options parameters, something like schemaValidation=false, which would, in turn, set c2optionsHashRequired to false?

0xpatrickdev commented 8 years ago

More generally speaking, what does meteor-editable-text default to for validation when simple-schema and collection2 are not present?

I made another app that does not have simple-schema and collection2, but I can't seem to make any updates to the gridFS collection. The package im using for gridFS only allows updates to be made on the server, and it seems meteor-editable-text complies with that. I am no longer getting the Exception while simulating the effect of invoking '_editableTextWrite' TypeError: Cannot read property 'simpleSchema' of undefined error, but the updates I try and make do not persist and no error messages are getting printed to the client or server consoles.

JackAdams commented 8 years ago

Thanks for looking into this so thoroughly. I'll accept at PR if you can get a fork of the package to work for you. Just a bit swamped right now, so probably won't get to it very quickly myself.

JackAdams commented 8 years ago

To answer your question: In the absence of simple-schema and collection2, there is no validation unless you write it. There are beforeUpdate and beforeInsert hooks in which you can modify the data that is being sent from the client before it is written to the db.

JackAdams commented 8 years ago

Ah yes... just had a think about it. This package uses automatic collection detection. It doesn't pick up the collections defined in other packages (#39). That's what's causing all this. In your .meteor/packages file, try reversing the vsivsi:meteor-file-collection and babrahams:editable-text order. If that doesn't help, I'll need to think a bit harder.

0xpatrickdev commented 8 years ago

No problem, thanks for making this ! I have limited experience working with packages, but I can try and whip up a PR.

And just to be clear, do you agree that the logic in line 187 is flawed, or should I try and add a new options parameter? Think I was getting null for _.isFunction(Collection.simpleSchema) and undefined for Collection._c2, but neither kicked out a "does not exist" error.

JackAdams commented 8 years ago

I think the code is fine. The errors you're describing are consistent with a collection not being detected by the auto-detection mechanism because it's in a package (#39). I really should put some developer-friendly error messages in the package ...

If switching the positions of vsivsi:meteor-file-collection and babrahams:editable-text in the .meteor/packages file doesn't work, I can think of an extension to the package API to allow you to declare collections from other packages, but I'd have to spend a bit of time coding and testing that.

0xpatrickdev commented 8 years ago

Ordering of the packages did not end up mattering, but the issue has been resolved.

I ran Mongo.Collection.getAll() in the dev console after reading https://github.com/JackAdams/meteor-editable-text/issues/39, and saw the FileCollection collections in results. They were referenced as "fs.files", so switching the function from collection="fs" to collection="fs.files" resolved the issue.

Thanks for the help!

JackAdams commented 8 years ago

Nice. Thanks for sharing the fix.

Sent from my iPhone

On Dec 17, 2015, at 3:48 PM, Patrick Cooney notifications@github.com wrote:

Ordering of the packages did not end up mattering, but the issue has been resolved.

I ran Mongo.Collection.getAll() in the dev console after reading #39, and saw the FileCollection collections in results. They were referenced as "fs.files", so switching the function from collection="fs" to collection="fs.files" resolved the issue.

— Reply to this email directly or view it on GitHub.