Proposal: Add an option that allows one to not validate mongo query results

nickrobillard commented 4 years ago

Disclaimer

I have used mongo-models in several projects now and I really like it. At this point, I feel (and hope) like I have a pretty good understanding of its intended purpose. As your readme clearly states mongo-models is "A light weight abstraction where we can interact with collections via JavaScript classes and get document results as instances of those classes". So, data passed in via the constructor as well as mongo results are an instance of mongo-models. And thus, they are both subject to validation.

The Problem

Most of my projects have existing or migrated data sets, and as one would expect, my mongo-models schema isn't followed perfectly in this existing data set. This means that even find methods implemented in mongo-models (and all methods that pass results through this.resultFactory) can potentially fail validation and throw a ValidationError. The first time I noticed this happening when doing a find operation, I was very surprised. It wasn't intuitive to me that existing data would be validated (I was used to Mongoose behavior). In these situations, it's usually not very feasible to fix my entire data set to match my schema. So I've avoided using find methods and used direct mongo queries (while still using other features of mongo-models). Of course work arounds in general are not ideal, but also, for other devs who work on the same code later, it is not immediately clear why I avoided using a mongo-models method and did something directly. So I've ended up adding "Here be dragons" comment blocks and documentation.

Proposal

Allow users to turn off validation of all mongo query results. After careful analysis of the current implementation, I think the only way this would work is to have an option that completely removes the result = new this(...) logic in this.resultFactory for all methods (or just don't pass results through this.resultFactory at all). The only time validation would happen is when the developer is instantiating their custom model that extends mongo-models (like the new Customer(...) example in the readme). I do realize that validating the mongo result is sometimes the main or only reason why methods are redefined - so if this is removed, they no longer have a purpose and their presence is a bit awkward.

Is this problem relatable? Thoughts? (@jedireza and anyone else)

jedireza commented 4 years ago

Is this problem relatable?

Yes this can be really annoying.

I'm time poor but I'm open to working through a PR that attempts to address this.

One potential escape hatch that already exists is to use the Model.collection() method. That way you interact with the db directly and skip the model entirely.

irothschild commented 4 years ago

I agree. The design of when validation gets done seems very odd. It lets you write an invalid model data to the db and then does validation when you read it. I would have expected validation to be done in insertOne() (for example) and not when reading the result.

tcurdt commented 3 years ago

@jedireza Would it be a major undertaking to move the validation to the write side of things?

tcurdt commented 3 years ago

I just checked. That does not look very hard to change. Would you be willing to accept a PR for this, @jedireza?

jedireza commented 3 years ago

I'd be happy to review and work through a PR. Ideally this would be an opt-in change.

tcurdt commented 3 years ago

@jedireza I took a first stab at it and implemented it only for insertOne https://github.com/tcurdt/mongo-models/commit/42a9141ad4a0ddf52cfacb1d0a641c03a5427aef

Do you agree on the approach?

...and maybe you have an idea on how to get lab.expect(blamo).to.not.throw() working with an async funtion?

jedireza commented 3 years ago

@tcurdt Thanks for taking the time to work on this. I'm in agreement with the general opinion in this thread, that it makes sense to validate on write only. So let's drop the optionality, which should clean the code around that. We can make this a breaking change and bump to v4.

Please open a PR, it's fine that it's WIP. This way we can comment on specific lines of code going forward.

tcurdt commented 3 years ago

I'm in agreement with the general opinion in this thread, that it makes sense to validate on write only. So let's drop the optionality, which should clean the code around that. We can make this a breaking change and bump to v4.

Glad you see it that way. I felt the same when looking into it. I will work on it a bit more on it and then create a PR.

tcurdt commented 3 years ago

Some progress. I think it comes down to implementing just the two functions now. https://github.com/tcurdt/mongo-models/commit/31686e12593439b7d54cc74ab839dc5e52a98003 The trickiest part (well...) will be to reduce the schema to what is applicable for update queries for a partial validation.

After that re-activate and improvement of some of tests. I am not sure how to replace the it constructs an instance using the schema though.

Are we otherwise still in agreement on the approach?

tcurdt commented 3 years ago

Partial updates are actually more of a problem than expected. While not a problem to implement for the $set operator, there are of course quite a few more operators that would required to load the value for validation.

Maybe it's more advisable to use or leverage native mongodb schema validation instead.

Any thoughts?

tcurdt commented 3 years ago

I guess using the native schema could be done in a similar fashion to https://github.com/yoitsro/joigoose Basically generating a joi schema from the native mongodb schema - or the other way around.

Then mongo-models would not have to deal with this at all.

jedireza commented 3 years ago

The trickiest part (well...) will be to reduce the schema to what is applicable for update queries for a partial validation.

Partial updates are actually more of a problem than expected.

This is bringing back faint memories of how the complexity can balloon pretty quickly. 😅

Maybe it's more advisable to use or leverage native mongodb schema validation instead. Any thoughts?

I don't know much about that functionality, so I have nothing constructive to add.

For the most part, I've moved on from using mongo-models. I only have one project that I maintain that uses it and the annoyance brought up in this issue goes unnoticed.

These days most of my development is done with TypeScript and I've been using TypeORM with Postgres. It doesn't come without it's own issues, but it's been fine.

When I created this project I wanted a thinner/simpler approach than Mongoose. Looking back now, it would've just been better to reach for that instead. If I was going to start a new "plain js" project targeting Node/JavaScript, I'd probably just use Mongoose. But that's a far fetch. I wouldn't use plain JavaScript if I could help it (TS for the win). 🏆

@tcurdt I can make you a contributor to this repo and you can take the lead. The only requirement I have is the next version that gets published to npm be a major version bump.

If you're not interested in that, it might be best for us to just update the readme letting folks know this project isn't going to be maintained and to look at Mongoose and TypeORM instead.

Thanks for getting involved and contributing. Please share your thoughts.

ddfridley commented 9 months ago

@tcurdt I been working on a successor for mongo-models for use with newer versions of mongodb because I like the pattern. It happens to support joi style validation, or mondodb validation, or as you request - no validation.

@enciv/mongo-collections

It's new, I'm just looking for feedback and people who want to try it out.

tcurdt commented 9 months ago

Interesting approach, @ddfridley

We ended up using a simplified fork where we have joi validation for writes. If there is a need to switch we certainly will have a look.

jedireza / mongo-models