Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Invalid models aren't retrieved properly #208

Closed
MiguelMadero opened this Issue Oct 3, 2012 · 14 comments

Comments

Projects
None yet
4 participants
Contributor

MiguelMadero commented Oct 3, 2012

It looks like if we change a rule in the model it affects the way we retrieve the old models saved before that rule existed. For exmaple, I saved a TODO with a short title, then add a rule {min: 5} and the title of the old TODOs is empty. There's no easy way to retrieve and compare using Geddy so that someone can fix the old, now invalid, TODOs. I tried this with the mongo adapter. I'm not sure if the problem is with the adapter or some other core component.

Contributor

polotek commented Oct 3, 2012

Interesting. How does rails or other frameworks handle this kind of thing when changing models? My expectation is that all models would still be returned. But if you had the old model with the short title and then you tried to save it again, the validation would fail and give you and error. You wouldn't lose data or anything, you should couldn't really update that model without turning off the validation.

The way this is usually handled in my experience is that when you roll out the model update, you also run a data migration script that updates any models to meet the new schema. It's an application level consideration.

Contributor

Techwraith commented Oct 7, 2012

Yeah, this is a case that will be solved once we roll out a way to handle migrations. Migrations are a ton of work though (especially across different DBs), so it may take a while.

Contributor

MiguelMadero commented Oct 16, 2012

@Techwraith having migrations will only address the scenario where we actually want to fix the data when we change the validation rules. The other scenario is to leave the old data the way it's (in an invalid state) and only fix it when needed (e.g. on save if we make changes).

The current implementation calls model.create to map the data retrieved from our storage. create always validates the model. IMO, retrieving existing data should be treated differently than creating it. While we want to create models in an valid state, we should be able to retrieve stored models in order to fix them to comply with the new validation rules.

The change is not that complex (I hope), but I would like to discuss this before going ahead.
Thoughts?

Contributor

Techwraith commented Oct 16, 2012

@mde what do you think about this one? Can you see any issues here?

@MiguelMadero What is the current behavior when pulling invalid stuff out of the db? Does shit just break?

Contributor

MiguelMadero commented Oct 16, 2012

From memory the model is retrieved and the invalid properties are just ignored (undefined). Ironically this behavior could actually result in an invalid model anyway, for example if we have a required property with min len of 5. So even if the argument was not to retrieve invalid models the implementation is still wrong.

Contributor

mde commented Oct 16, 2012

@polotek is right; this is something usually handled in the migration. Migrations that change the schema should also update existing data to conform to the new schema.

This is a little different for us because Model supports several NoSQL stores, which are schema-less. But in the end, what Model should do is make sure that what you have when you create new items, or retrieve items from storage, you have something that conforms to whatever the model for that type of thing is defined as.

The only alternative to this would be to add support for versioned definitions, which would be awful to support. (We actually talked about doing this with Yammers "YModules" code that uses an older version of this code.) It's extremely complicated, and would require potentially upgrading stored instances through multiple upgrades to the schema -- pretty horrible.

The right place to do this is in the migration. If you change the schema, upgrade the existing data to conform to it.

Bottom line is, we need to implement migrations.

Contributor

MiguelMadero commented Oct 16, 2012

I'm not talking about schema changes only, but validation rules, which, even in a traditional SQL DB are defined at the App Layer.

Let's assume that before we weren't validating emails comply to the email format (silly mistake just for the example). We have thousands of customers now. We identify there're some odd emails, we add a new validation rule to prevent this in the future and we have to deploy anew version, but we already have a few clients that are in an 'invalid' state and already stored in the DB. Are we going to clean this data now? We can't really update this with a migration, but we'll have to live with the invalid data until the client's login and we ask them to fix it, someone updates it or until someone from the business fixes it or, well maybe forever.

My point is, migrations are only one option for one scenario and as @mde said not even an easy one to do at this stage. So I think we should support the other scenario and be able to retrieve invalid models.

Model should do is make sure that what you have when you create new items, or retrieve items from storage

@mde I agree about the create, but why do you consider this important on retrieve?
If it's, let's keep in mind that they're invalid even if we don't map the invalid properties when the property is required.

Contributor

Techwraith commented Oct 16, 2012

Yeah, I think Model should at least have the invalid value on the instance, it really shouldn't be undefined. I'm fine with keeping them as "invaid" though, as long as we have a way to force saving for an invalid model instance.

Contributor

MiguelMadero commented Oct 16, 2012

I'm personally against saving invalid models, but I would consider adding
it for a generic solution. I know other ORMs support this and some people
like it.

My preference would be to support retrieving invalid models, but only save
valid ones.

Miguel

On Tue, Oct 16, 2012 at 7:31 PM, Daniel Erickson
notifications@github.comwrote:

Yeah, I think Model should at least have the invalid value on the
instance, it really shouldn't be undefined. I'm fine with keeping them as
"invaid" though, as long as we have a way to force saving for an invalid
model instance.


Reply to this email directly or view it on GitHubhttps://github.com/mde/geddy/issues/208#issuecomment-9509565.

Contributor

Techwraith commented Oct 16, 2012

@MiguelMadero There are cases where you'd like to programmatically update a set of model instances. If one of those happens to be invalid (but was previously valid), then you wouldn't be able to do these kinds of mass updates.

Contributor

MiguelMadero commented Oct 16, 2012

There's always one or more exceptions to any rule.

Miguel

On Tue, Oct 16, 2012 at 7:40 PM, Daniel Erickson
notifications@github.comwrote:

@MiguelMadero https://github.com/MiguelMadero There are cases where
you'd like to programmatically update a set of model instances. If one of
those happens to be invalid (but was previously valid), then you wouldn't
be able to do these kinds of mass updates.


Reply to this email directly or view it on GitHubhttps://github.com/mde/geddy/issues/208#issuecomment-9509795.

Contributor

mde commented Oct 17, 2012

I was just taking a look at the AR docs, and they make a distinction between new and create that we don't -- but in either case, it looks like they go ahead and set the values on props irrespective of validity. I guess as long as you can't save an invalid instance without a special flag, I'm okay with this. Here's the ticket: mde/model#7

Contributor

mde commented Oct 17, 2012

Also for a flag for saving invalid instances: mde/model#8

The work will happen over on Model, so I'm going to close this one.

@mde mde closed this Oct 17, 2012

Contributor

mde commented Oct 17, 2012

Fixed and pushed in Model v0.0.13.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment