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

if count($_schema) > count(database) #524

Closed
hans-d opened this Issue Jun 3, 2012 · 14 comments

Comments

Projects
None yet
4 participants
Contributor

hans-d commented Jun 3, 2012

if a defined schema in a model has more rows as the database backend (mysql in my case), li3 dies with:

PHP Fatal error: Unsupported operand types in [...]\libraries\lithium\data\Entity.php on line 408

due to a difference in array sizes and the usage of array_combine. Expectation is that li3 detects this on time and comes with a proper warning (and possibly, perhaps dependend on a setting, will continue to function while ignoring the extra field and assumes it has a null value).

use case: multiple people are working on development that effects both model and db, and not all changes are a applied in a place (eg database alteration not done/forgotten). A message like 'oops, you forgot to apply changes to both model schema and db' would save a lot of time.

see for test: #523

Member

Howard3 commented Jun 3, 2012

Generally speaking you should not define a schema for db's which have them, they utilize the 'describe' method which pulls their schema. This can be cached via a filter.

Contributor

hans-d commented Jun 4, 2012

Yes, in general you are right... When you cache the describe and
somebody does an alter table on the db with the app still running, you
may run into the same issues.

I ran into this issue because I was feeding my database derived mock
source, which happened to have some data issues.

I'm also busy with an li3_migrate, and was thinking about having the
schema present in the model, so it could drive any needed migrations
of the current database.

Member

Howard3 commented Jun 4, 2012

I'm curious. A table should rarely change in production, what is the application here that the database could change outside of new deploys of new versions of the application?

Contributor

hans-d commented Jun 4, 2012

I agree this shold normally not happen.

It could be by accident: deploy new app, apply changes, start using app. 'Oops, did something wrong with manual database patch and add that column'... whatever.
In an environment that I know (not yet li3 based), they run multiple environments (eg the personal dev envs) against the same db...

Owner

nateabele commented Jun 4, 2012

For the time being, I think we should just document the default behavior of the schema cache, and leave it up to the user to clear it as appropriate.

Owner

nateabele commented Jun 4, 2012

Does this also apply to your tests in #519 - #523?

Contributor

hans-d commented Jun 4, 2012

Yep, I've included tests for all variants.

I can add an expect exception for this one... to make it pass...

Owner

nateabele commented Jun 4, 2012

Okay, I guess I don't understand what the expectation is here. That Lithium should automatically detect that the cached schema is different from the database everywhere schema data is used, and code around it accordingly?

This sounds hugely inefficient both in terms of time and effort expending to code and maintain, as well as framework performance. Given the simple, straightforward alternative of just remembering to clear your schema cache when you alter your tables, I don't see how this is a reasonable solution.

Now, if you want to write a plugin that manages schema changes and updates your cache accordingly, great. However, writing tests that expect operations to work when the input to those operations is invalid doesn't make any sense to me.

Contributor

hans-d commented Jun 4, 2012

there is only a tiny bit of code where it hurts on differences... That is https://github.com/UnionOfRAD/lithium/blob/master/data/source/Database.php#L311, in read where it does a $records[] = array_combine($columns, $data);.

so the solutions for the framework could be:

  1. do nothing
  2. apply try/catch, and throw a li3 exception - explaining what could be wrong but do not try anything else
  3. apply try/catch, and make the counts the same by adding null columns so it matches.

1 and 2 are simple, 3 is not very difficult but makes nobody aware of the issue at hand.

I think option 2 would be best, elegant and informative (for those few times it may happen). The user/developer/sysadmin/... still has to fix things himself.

The plugin has nothing todo how the framework handles things, but was only to indicate the is a possible use case that for a SQL environment the schema could be in the models.

Owner

nateabele commented Jun 4, 2012

Your suggeted fix is inadequate, as it masks the real issue. To illustrate, suppose you have a posts table that looks like the following:

id: integer,
title: varchar,
body: text,
slug: varchar,
created: datetime

Now, suppose you drop the slug column (let's suppose it's not needed anymore), and added a an updated column, such that your table now looks like this:

id: integer,
title: varchar,
body: text,
created: datetime,
updated: datetime

I bet you can guess what'll happen: the data from the updated column gets mapped to the created field. Even worse, the created column gets mapped to the slug field, and they aren't even the same data type! In this scenario, we never even see that there's an error, since the column lengths are still the same, and even more subtle bugs are introduced.

Again, I think it's a much better idea to document the schema caching behavior, and explain to people that they should clear it when altering tables.

Contributor

hans-d commented Jun 4, 2012

I have an other issue outstanding for the difference in column names (#519). The comment you are making is better placed there... My comment was only in the context of this issue as stated in the first comment.

But regarding your remark, perhaps for li3 this could be the best. Document that defining $_schema yourself in the model has the noted risky side effects. We can than leave it up to plugins to do any extra's when/if needed.

(i will certainly not use $_schema for the migrate plugin due to this nature).

@hans-d hans-d closed this Jun 4, 2012

Member

marcghorayeb commented Jun 20, 2012

@nateabele are you saying that lithium caches the models' schemas somewhere? because if I don't hardcode the schemas in my models, lithium will always do a describe, is there an option somewhere to set up schema caching??

Contributor

hans-d commented Jun 26, 2012

@marcghorayeb, I suspect that @nateabele refers to cacheing during the request cycle.

And most likely the answer to cacheing over mutiple requests: filters (eg for https://github.com/UnionOfRAD/lithium/blob/master/data/source/database/adapter/MySql.php#L211)

Member

marcghorayeb commented Jun 26, 2012

yup that's what I ended up doing. My schemas are cached in redis and it works like a charm. thanks for clearing that up!

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