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

[discussion] On "enforce: missing" timing #29

Closed
nikaspran opened this issue Oct 23, 2013 · 3 comments
Closed

[discussion] On "enforce: missing" timing #29

nikaspran opened this issue Oct 23, 2013 · 3 comments

Comments

@nikaspran
Copy link
Contributor

As I am not sure what the desired behavior should be, I wanted to start a discussion on enforce: missing and it's timing.

At the moment, this option is enforced in Model.checkType(), which is called when creating a new instance of the Model. Meanwhile in traditional RDBMS/ORMs the constraint checks are usually performed on save to DB.

I realize this is an entirely different paradigm, and the missing field enforcement doesn't actually happen in RethinkDB unlike traditional constraints (as in, it's a concept of Thinky), but I am not sure if the current way is preferable.

IMO, there are a few issues with the way Thinky enforces missing fields:

  • At odds with how most traditional DBs/ORMs do it.
  • Disallows incremental building of the Model objects. It might be cumbersome to supply all the required properties in the constructor.
  • Requires exception handling every time we're trying to create a new Model variable.
  • If by any chance a faulty record appears in my table (say by manual insert, or by someone adding an enforce: missing field to an existing schema), any queries I do on that table that return the document will explode. I'd say this is the main problem with the current approach.

Here is what I would suggest:

  • Call the Model.checkType() during Model.save() (before passing on to RethinkDB driver) and only then. If it fails, return an error to the callback. This would let us handle the enforcement error in the same place as any other RethinkDB errors, slightly reducing code. It would also mirror the way a lot of traditional DBs/ORMs do it, reducing adoption confusion :)

Any other ideas would be welcome. Does this sound reasonable at all?

@neumino
Copy link
Owner

neumino commented Oct 24, 2013

I didn't think of your case when adding enforce, but it makes sense to me and I can see why it can be useful.

Instead of having a boolean, we can have multiple option for enforcing missing:

  • false
  • "always"
  • "onSave"

How does it sound to you @nikaspran?

@nikaspran
Copy link
Contributor Author

I'd have to agree that flexibility would probably be the best solution. I can see why the option to check the model for consistency on creation might be useful too.

👍

@neumino neumino mentioned this issue Jan 20, 2014
@neumino neumino mentioned this issue Apr 14, 2014
@neumino
Copy link
Owner

neumino commented Apr 28, 2014

It's now an option, and by default it validates the document before saving it.
Fixed with 1.0.0

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

No branches or pull requests

2 participants