Skip to content

Fix timestamp() setting a key named "null" in some cases - #1820

Merged
ricardograca merged 2 commits into
masterfrom
rg-null-key
Apr 24, 2018
Merged

ricardograca merged 2 commits into
masterfrom
rg-null-key

Conversation

@ricardograca

Copy link
Copy Markdown
Member

Introduction

The previous PR introduced a bug where a model could end up with a key named "null" if the user specified a custom hasTimestamps value that included a literal null as the second key name. This would lead to a SQL error complaining about a nonexistent column named "null" or "undefined".

Proposed solution

This checks if a updatedAt key exists first before attempting any other comparisons. The problem was in the first condition that would incorrectly return true if the key hadn't been set, even though it should only be checking if it had been set by the user.

- This would only happen if the user had set a custom value for
hasTimestamps that included a literal null value for the updated_at key.
@ricardograca ricardograca changed the title Rg null key Fix timestamp() setting a key named "null" in some cases Apr 23, 2018
@ricardograca
ricardograca merged commit c3812ad into master Apr 24, 2018
@ricardograca
ricardograca deleted the rg-null-key branch April 24, 2018 10:30
@amaranadh-wal

amaranadh-wal commented Jun 14, 2018 •

Copy link
Copy Markdown

If we give only created_at attribute in the array it is taking undefined in the insert query instead of updated_at.

For example

export default homeBookshelf.Model.extend({
  tableName: 'notifications',
  hasTimestamps: ['created_at'], ...

For above model declaration the query is going like

insert into 'notifications' ('created_at', 'initiator_id', 'initiator_type', 'notification_group_id', 'undefined', 'user_id') values ('2018-06-14 15:41:18.019', 333, 'user', 187, '2018-06-14 15:41:18.019', DEFAULT)

If we give like

export default homeBookshelf.Model.extend({
  tableName: 'notifications',
  hasTimestamps: ['created_at',null],

Then the query is going like

insert into notifications ('created_at', 'initiator_id', 'initiator_type', 'notification_group_id', 'null', 'user_id') values ('2018-06-14 15:40:41.754', 322, 'user', 180, '2018-06-14 15:40:41.754', DEFAULT)

So, I have given false value for the > hasTimestamps then it is working.

@ricardograca

Copy link
Copy Markdown
Member Author

Have you tried the code that is in this change or are you referring only to the published version 0.13.3?

@amaranadh-wal

amaranadh-wal commented Jun 14, 2018 via email

Copy link
Copy Markdown

@ricardograca

Copy link
Copy Markdown
Member Author

This PR you're commenting on fixes that then.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants