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

Allow createdat to be set/saved on new instances #887

Closed
chapmandu opened this issue Sep 3, 2018 · 10 comments
Closed

Allow createdat to be set/saved on new instances #887

chapmandu opened this issue Sep 3, 2018 · 10 comments
Assignees
Labels
Milestone

Comments

@chapmandu
Copy link
Member

@chapmandu chapmandu commented Sep 3, 2018

// I am importing historical data
user.new(name="Jack Flash", createdat=oneYearAgo());
user.save();
// user.createdat is now equal to today

// workaround is to update
user.update(createdat=oneYearAgo());

@andybellenie

This comment has been minimized.

Copy link
Contributor

@andybellenie andybellenie commented Sep 3, 2018

Second this, it's always bugged me you can't manually set timestamps, same applies to updatedAt

@dbelanger

This comment has been minimized.

Copy link
Member

@dbelanger dbelanger commented Sep 3, 2018

I agree with createdAt but updatedAt gets a bit sticky. If you were to edit an object and post it back to the ORM, the updatedAt field present would always override now() and we'd lose the usefulness of updatedAt altogether.

I'm not saying I don't do it because I do, I just don't do it often. At present, I use a follow up SQL call that does the overwriting.

My 2 cents: Rather that remove the protection that's present, why not create a function that overrides those dates. That way when we do object.update() and we know there's an override to be done, we then call object.updateCreatedAt(newDate) or object.updateUpdatedAt(newDate).

@andybellenie

This comment has been minimized.

Copy link
Contributor

@andybellenie andybellenie commented Sep 3, 2018

You can avoid that issue by only doing an automatic updatedAt when the column value hasn't been changed, i.e. if (!hasChanged('updatedAt')...

@neokoenig

This comment has been minimized.

Copy link
Member

@neokoenig neokoenig commented Sep 3, 2018

My only concern in changing this is introducing a possibility of mass assignment with pre-existing apps; Let's assume you were doing this (which on a user object you'd not do, but you might do something like it somewhere) and let's assume the createdAt date was important to your application in some critical way:

// incoming form params: todo[title]: "foo"
todo = model("todo").create(params.todo)

The current behaviour has createdAt as an immutable value in this context and defaults to now(); with this change, if you get a spoofed form input which now has createdAt as some older date, you've got a value being altered which previously wouldn't have been. But then, thinking about it, this condition presumably already exists on update() too, so I'm probably overthinking it. Maybe the point is I always forget about timestamps

@andybellenie

This comment has been minimized.

Copy link
Contributor

@andybellenie andybellenie commented Sep 3, 2018

I agree in principle but I think the issue there is passing an unsanitized form input directly into the object. There are multiple ways to deal with that, but currently it's impossible to use automatic timestamps if you ever need to set it manually.

@neokoenig neokoenig added this to the 2.1.0 milestone Sep 18, 2018
@chapmandu

This comment has been minimized.

Copy link
Member Author

@chapmandu chapmandu commented Sep 21, 2018

What's the consensus with this one?

@neokoenig

This comment has been minimized.

Copy link
Member

@neokoenig neokoenig commented Sep 21, 2018

Go ahead I think

@chapmandu chapmandu self-assigned this Sep 23, 2018
@dbelanger

This comment has been minimized.

Copy link
Member

@dbelanger dbelanger commented Sep 24, 2018

I also think we should move ahead with this but I believe a flag is required to allow the overwriting of any auto-timestamp (deletedAt too for consistency). This flag should default to false.

Similar to what we do with allowSoftDeletes, it would be allowManualTimestampUpdates or something to that effect. This would protect against Tom's concern that it's a critical field in many existing apps .

@neokoenig

This comment has been minimized.

Copy link
Member

@neokoenig neokoenig commented Sep 24, 2018

I actually really like that idea - after all, when you're doing timestamp overrides, it's usually going to be something like "import all this data here and use the postDate to be the createdAt date" - it's not a 'normal' operation. (Unless I've missed something).

chapmandu added a commit that referenced this issue Nov 29, 2018
chapmandu added a commit that referenced this issue Nov 29, 2018
@chapmandu

This comment has been minimized.

Copy link
Member Author

@chapmandu chapmandu commented Nov 29, 2018

Closed 9da9441

@chapmandu chapmandu closed this Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.