-
Notifications
You must be signed in to change notification settings - Fork 443
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
MySQL Driver does not populate TIMESTAMPs on record create. #455
Comments
@shmax Thought you might be interested in this as well if you haven't seen something similar already. |
@cvanschalkwijk have you seen this issue? |
@EVILoptimist Will investigate tonight after work |
@EVILoptimist |
If someone can start a PR with a failing test for this specific situation, that would be a great start. |
Maybe an idea to invalidate the cache on save, and create the cache on read? |
@koenpunt This doesn't have anything to do with caching of individual records. The bug happens whether you cache or not. The only time caching enters into it is that once you teach your adapter (or column) how to interpret a default value of CURRENT_TIMESTAMP, then you wind up with a collection of cached ActiveRecord\Column records that have the same current time baked into them. The first PR I posted gets around this by caching raw column info, but not the Column records themselves. |
I'm working on fixing the unit tests in my original PR. Should have something tonight. |
I understand, but if the cache is set on fetch of a model, instead of save, then the |
The use case provided by @EVILoptimist doesn't even get as far as fetching anything (well, he does a save, but you can remove that and still see the prob). You can repro the bug without a fetch, and without any cache, so I don't see how we're going to fix anything by discussing fetching/caching. When I mention caching being a problem, I'm talking about the cached table columns. They come into play when you new a model, because the constructor for a model walks over the columns and uses them to come up with suitable default values. So you can start by adding the code to properly convert CURRENT_TIMESTAMP to a current DateTime, but then you have a problem when caching (of table metadata) is turned on, because the current DateTime now stashed in the cached column models never gets refreshed. So, my first attempt at a fix is very simple; it caches the raw table metadata, but not the Column models themselves, such that they get rebuilt when table() is called for the first time. This will come at the expense of a little speed at run time. I'm going to do a second PR that does cache the column records, but adds a default() accessor method that applies the logic to convert CURRENT_TIME to a DateTime on the fly. More later. |
The problem now is that the |
@EVILoptimist can you test my branch https://github.com/koenpunt/php-activerecord/tree/reverse-cache-read-write. Also as PR #458. |
@EVILoptimist @koenpunt Anyway, can you explain a little more about what you mean by time being calculated in PHP vs MySQL? I still don't see the problem; by calculating the time in PHP and explicitly passing it in when doing our first write, we're not even giving MySQL a chance to invoke CURRENT_TIMESTAMP. And when we do the write, we use a format that has the timezone in it, so there should be no ambiguity, right? Not saying you're wrong--you're probably right--just that I need some help understanding. |
My explanation of the problem of @EVILoptimist (correct me if I'm wrong): $widget = new Widget();
$widget->category_id = $category_id;
$widget->manufacturer_id = $manufacturer_id;
$widget->name = $name;
$widget->save();
$widget->ts #=> null; At this point, the database has filled the The only way to get the database-filled # calling reload on the model
$widget->reload();
# or by initiating a new find
$widget = Widget::find($widget->id); The fact that the incorrect values were cached, was because it cached the created model, not the fetched one. That issue will be resolved by #458, where the cache will be created on first fetch, including the database-filled My final note, a quote of @bladenkerst from years ago (#2):
And I agree, just like the class Widget extends ActiveRecord\Model {
public function before_save(){
$this->ts = date('Y-m-d H:i:s');
}
} |
@jpfuentes2 @cvanschalkwijk @al-the-x @Rican7; I like your word on this, but I don't think that dynamic default columns is something we're going to implement, right? |
@shmax The problem with calculating time on PHP vs MySQL only manifests when you have multiple servers with dedicated roles (and possibly with different timezone settings). I am using my dedicated MySQL server in this instance for a common point of reference for time. The CURRENT_TIMESTAMP is perfect for making sure that records are not being stamped with a possible out-of-sync (or out-of-timezone) time. This is especially useful for clients that might be connecting via WAN. @koenpunt I agree that the addition of a before_save() function is a simple solution for this issue and might even be best practice. It, however, does not meet the needs of my implementation. You have given me a bit of inspiration, however: class Widget extends ActiveRecord\Model {
public function after_save(){
$this->reload();
}
} This seems to accomplish the same task without the need of appending the Model::reload() call to my creation functions. I could probably also further optimize by writing a conditional based on my TIMESTAMP columns. |
"At this point, the database has filled the ts column with the CURRENT_TIMESTAMP, but as it has been filled on the database side, the model does not know about it." @koenpunt But that's not correct; you have the order of operations backward. When you "new" a model, the first thing that happens is the constructor fires. Now, look at line 269 of Model.php. See that foreach, there? It's walking over each column defined by the table schema, and using its knowledge of the default property to pre-initialize the attributes. In our case, "CURRENT_TIMESTAMP" is the default, so we pre-emptively prime our ts with the current time (in my PR). When we do our first save, that time is written directly into the first INSERT statement, so MYSQL never gets a chance to come up with a default date itself. This is true of any default value; try it with a text field, for example; same behavior. Because MYSQL never gets a chance to fire its own CURRENT_TIMESTAMP, I don't see how the timezone the MySQL server is in makes any difference. There may be some other use case that presents a problem, but the one we've been working with does not suffer from the issue you describe. I've been testing with my changes, and so far it not only works perfectly, but passes the unit test. Doing "reload" is not a fix; it leaves the unit test I created unresolved. |
Yes, but the behavior of
What is the use of
True, because its a undesired use case. So to sum it up, there is no use in implementing a very dynamic default column value. |
Pretend for the moment that we're not talking about dates. Let's say we're talking about a text field, and we set it up with a default value of "puppies". In ActiveRecord, we come pre-armed with information about the table schema. We know about the "puppies" default before we've done our first write, or fetch, or reload, or what have you. When we instantiate a new record using "new", we use that advance knowledge to load our in-memory object with the default values. So we can do this:
This is before we've done even a single write or read from the database. Now, when we save our record for the very first time...
We pass the attributes into an insert statement, and those attributes have "puppies" in them. Do you follow that? Even though we defined "puppies" as a default value in our schema, MYSQL was never impelled to rely on it, because we wind up passing it in expliclty from the outside. No one seems to be alarmed by that behavior; it works very well and is at the heart of the active record pattern. So when you say "What is the use of CURRENT_TIMESTAMP on the column if it's not going to be used?", I can turn it around on you and say "What is the use of defining a default value on a text field if it's not going to be used?" My answer is well, that's how the active record pattern works. Now, let's consider a timestamp value with some explicit default, such as "12/4/2014 6:00:00 AM". Work through the same series of steps, again; we grab the default from the schema, new our object, save it, and create an insert statement with that value in it. Still no complaints, right? In both these examples, remember that we never get as far as needing MYSQL to rely on those default values when doing its first insert; we pass them in explicitly. Now, take a deep breath and consider CURRENT_TIMESTAMP. Walk through the same steps again. The only difference here is that we're interpreting the CURRENT_TIMESTAMP in code. Otherwise the pattern is exactly the same! We initialize our default date in our constructor, we do our first insert with the value filled-in, and MySQL never gets a chance to invoke its default value (because we pass it in). As for timezone issues, I still don't see the issue. Active record does its date writes with a format that has the timezone in it (which I find a little confusing; I would expect an ISO 8601 format to be easier to work with), so there's not much room to get things wrong (and if there is, it's fixable). Try it for yourself. Download my branch, fire up a debugger, and step through the following code:
|
I can be short, I know exactly what you mean, but |
So? What's the difference? Where's the bug? Can you show me a test case that illustrates it? |
There is no bug. There is simply no support for dynamic default values. And if it's for me to decide it stays that way (Rails' ActiveRecord doesn't support it either). @jpfuentes2 @cvanschalkwijk what do you guys think? |
If you want your column prefilled you can always add a getter to the corresponding model: class Widget extends ActiveRecord\Model {
public function get_ts(){
return $this->read_attribute('ts') ?: new DateTime();
}
} |
You would rather leave a timebomb of a bug in the code when a fix is not only possible, but in your possession? Do you want people to keep opening issues when they encounter this bug? To be clear, you are essentially requiring people to do three times as many database operations as they would otherwise need to in anything but the most vanilla of scenarios. Yes, it works, but it's creating more load on the server, and you're now requiring special handling that is not intuitive unless you have spent time analyzing the codebase:
I offer two compromises:
As for the timezone issue, I've asked for a specific use case demonstrating the problem, but have not yet received one. I'm willing to write some some test cases around the concept if it would help us understand the issue. |
@shmax There is a proposed solution, using the Nb. we appreciate your contributions, but the way you propose your compromises as an ultimatum is not how open source works. Sometimes, things that look very important to you, are not that important to the community. |
@koenpunt My fix does solve his problem. I even provided a test that illustrates it. It fails without my fix, and passes with it. If you are philosophically opposed to my method (coming up with the current time in PHP), then you should also be philosophically opposed to the before_save idea; they are fundamentally the same. The only difference is that mine is automatic and doesn't require the user to be aware of the issue or do anything special. I have not issued any ultimatums. I have asked that you not rush to close the issue until it is better understood by us all. You obviously didn't understand it very well, as evidenced by your failed fix, and your failed workaround. My understanding is not complete, either, as various folks have hinted that there may be a timezone issue. Let's work together to figure it out. |
@shmax I've re-opened your pull requests and we will review your code there. |
@cvanschalkwijk Thank you. I'm working hard to try to get all the tests to pass, but I'm having trouble with SQLite ("Database is locked") in my local environment. |
I will quote @EVILoptimist (#455 (comment)):
So he's saying he uses his db as date reference, not the date of php. but maybe @EVILoptimist himself can clear the sky for us? And you really should stop making accusations and suggesting I'm dumb or not understanding you (I understand you perfectly, I'm just not agreeing) if your going to talk about respect and all |
Yes. In my implementation. I cannot guarantee that timestamps that are
|
@koenpunt You're not dumb, just rash. You ignored my PR, and wrote your own "easy fix", which didn't even remotely work. Then you provided a workaround which was patently wrong. When pressed, you decided that because Ruby doesn't deal with it, we shouldn't either, and just slammed the whole thing closed in my face, apparently more out of frustration than a desire for resolution. That isn't exactly respectful, either. My fix may very well be wrong, too, but it would be nice if we could establish that through some test cases, and not just angry hand-waving. @EVILoptimist Can you explain with a code sample how the dates can diverge or be off? If you are using ActiveRecord to create your rows, and (with my proposed fix) the dates are always being taken from the server and consistent among themselves, where is the issue? I'm still having trouble understanding how a server's timezone makes a difference. If I give you, say, a date in GMT format, the timezone doesn't matter. The date is absolute, and the timezone only really comes into play when displaying it to the user, right? |
I'm not the one who's angry here.. You keep ranting about my behavior (what does that say about your behavior?). You keep saying my solutions are not working (they do, but they don't solve this particular issue). And last, what makes you think I'm frustrated? I just closed this issue with possible workarounds, and moved on with my live. You try to make an issue of a problem that actually only exist of the assumption you made of @EVILoptimist's problem. |
@shmax @koenpunt please keep it constructive and on topic. I took a look at the rails AR pattern and reviewing a few other ORM libraries to see how other people have handled this same issue. This just popped up a few days ago so it will take some time to figure out what the best solution might be, if any. |
👍 |
My fix addresses the original snippet he posted. Before my fix, his snippet fails. With my fix, it doesn't fail. This is not true of any of your solutions. And that's fine. You are allowed to make mistakes, as are we all. We've since learned that @EVILoptimist would prefer that any and all dates be generated solely by his MYSQL server, and that's fine. There is no point in arguing whether he has a point or not; he wants to do it the way he wants to do it. That doesn't mean it's time to throw up our hands and move on. |
@cvanschalkwijk sgtm |
@shmax I don't believe (and I could be wrong as I can't verify ATM) mysql @koenpunt https://github.com/koenpunt You're not dumb, just rash. You My fix may very well be wrong, too, but it would be nice if we could @EVILoptimist https://github.com/EVILoptimist Can you explain with a code I'm still having trouble understanding how a server's timezone makes a — |
Ahh, now we're getting somewhere. Okay, so that's valid for your situation, and by all means my fix is not appropriate for you. But the issue still stands for other people without the same concerns, such as myself. I use ActiveRecord on a production site, and I use MySQL, and I would very much like to be able to use CURRENT_TIMESTAMP without having to rely on extra reads and writes. It seems like something that could be easily handled with some configuration settings. There could be a global config, and one at the model level that can optionally override the global one. |
@EVILoptimist ActiveRecord aside, how do you prevent a malicious user from setting crazy time values directly via MySQL queries? |
I agree. I understand the issues with the driver, but since I'm able to
|
This made me realize $widget = new Widget();
$widget->id; #=> null And there is a efficient way to retrieve the insert id (PDO::lastInsertId) without doing an extra query for it, so its included after save: $widget = new Widget();
$widget->save();
$widget->id; #=> 1 But for |
That's actually a very good insight. |
You could always do this once per session:
That gets you the current time from the database. Store it in a static somewhere so you only have to do it once, and use it in place of time() or $_SERVER['REQUEST_TIME']; |
Do you like this any better? With this method, you supply your own current time to the ActiveRecord config (it defaults to letting PHP come up with it), and that gets used when interpreting CURRENT_TIMESTAMP:
|
I'm keeping my eyes on this now. Thanks to everyone for the descriptions, insights, and staying on topic. |
After discussing it with @kla and @jpfuentes2 we are going to leave the current implementation as is. There is no reliable way to predict storage engine timestamps and the reload() method already exists in cases where the timestamp is needed after a save() and was not explicitly set by the developer prior to saving. Other ORMs out there have already made a similar decision. |
Yeah, I thought about it some more, and nobody made this argument, but this illustrates the problem with my solution:
|
True enough. |
Hmm, I guess you could get pretty close by querying the database for the current time, then adding a delta whenever CURRENT_TIME is requested. But I'm going to move on. |
@shmax OK. I just commented on #465. And, I'm pretty excited about that kind of a solution if it's feasible. But I guess we're beating a dead horse at this point? @cvanschalkwijk |
@EVILoptimist I don't think we're going to win any arguments here, but if you're interested we can see if we can get it polished up enough to merge into my fork. |
When creating a new record in a model with caching enabled (or disabled), if the (MySQL) database table includes a TIMESTAMP column with a default of CURRENT_TIMESTAMP, the record will not include data for the TIMESTAMP column when ->save()d.
My current workaround is to call reload() after save()ing a new record.
The text was updated successfully, but these errors were encountered: