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

'Duplicate Record' overwrites existing record #1255

Open
cyrano-68 opened this Issue Dec 6, 2017 · 8 comments

Comments

Projects
None yet
4 participants
@cyrano-68
Copy link

cyrano-68 commented Dec 6, 2017

Details for the issue

  1. I have a table with a CHECK constraint:

CREATE TABLE Table1
(
id INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL,
Date DATETIME NOT NULL CHECK(Date IS strftime('%Y-%m-%d', Date)),
Value REAL NOT NULL,
Description TEXT DEFAULT NULL
);

  1. I have some records in the table:

INSERT INTO Table1 (Date, Value, Description) VALUES ('2017-12-06', 1.0, 'Value1');
INSERT INTO Table1 (Date, Value, Description) VALUES ('2017-12-06', 2.0, 'Value2');
INSERT INTO Table1 (Date, Value, Description) VALUES ('2017-12-06', 3.0, 'Value3');
INSERT INTO Table1 (Date, Value, Description) VALUES ('2017-12-06', 4.0, 'Value4');
INSERT INTO Table1 (Date, Value, Description) VALUES ('2017-12-06', 5.0, 'Value5');

  1. I select the first row of the table (i.e. the row with _rowid_ = 1), I right-click on that row and then I apply the "Duplicate record". These are the queries executed by the program:

INSERT INTO main.Table1(id,Date,Value) VALUES (6,'','');
UPDATE main.Table1 SET Value=? WHERE _rowid_='5';
UPDATE main.Table1 SET Description=? WHERE _rowid_='5';

The first query (the 'INSERT INTO ...') fails (correct, because the empty field-value for 'Date' does not pass the check) but the program continues. Therefore it updates the wrong record (overwriting an existing record)

Useful extra information

I'm opening this issue because:

  • DB4S is crashing
  • DB4S has a bug
  • DB4S needs a feature
  • DB4S has another problem

I'm using DB4S on:

  • Windows: ( version: ___ )
  • Linux: ( _distro: Ubuntu 16.04 )
  • Mac OS: ( version: ___ )
  • Other: ___

I'm using DB4S version:

  • 3.10.99
  • 3.10.0
  • 3.9.1
  • Other: ___

I have also:

@justinclift

This comment has been minimized.

Copy link
Member

justinclift commented Dec 7, 2017

Ouch, that sounds like a potentially nasty bug. 😦

@justinclift justinclift added the bug label Dec 7, 2017

@epraveenns

This comment has been minimized.

Copy link

epraveenns commented Dec 9, 2017

Reproducible on Mac OS latest version 3.10.1 too

@justinclift

This comment has been minimized.

Copy link
Member

justinclift commented Dec 9, 2017

☹️

@justinclift

This comment has been minimized.

Copy link
Member

justinclift commented Dec 9, 2017

Thanks for double checking @epraveenns. 😄

@MKleusberg @mgrojo This one sounds important!

@epraveenns

This comment has been minimized.

Copy link

epraveenns commented Dec 9, 2017

Ideal query would be

INSERT INTO main.Table1(id,Date,Value) VALUES (null,'2017-12-06', 1.0, 'Value1');
sqlite will automatically take care of auto increment silently.

If the column is not auto increment, then all we can do is to blindly duplicate the row. It will then throw an error as the primary key is already taken.

I don't understand

  1. Why DBBrowser is first creating a row with empty values,
  2. Why DBBrowser is allocating the primary key value 6 (what happens if there is any concurrent request with the same primary key),
  3. If it executes multiple statements for duplicate row feature, it should at least be done inside a transaction.

I Need to walkthrough its code.

Correct me if I am wrong.

mgrojo added a commit that referenced this issue Dec 10, 2017

Do not ignore error when inserting rows in 'Duplicate record'
That avoids overwriting existing record as reported in issue #1255.

This doesn't improve the underlying situation, that is inserting empty rows
before duplicating the content. But it is safer to not ignore the error in
the initial row insertion.
@mgrojo

This comment has been minimized.

Copy link
Contributor

mgrojo commented Dec 10, 2017

The error in the first step, inserting an empty row, was ignored. I've improved that, by checking the error and aborting with the message to the user. Now the overwrite does not occur, that is the most important issue.

This doesn't improve the underlying situation, that is inserting empty rows before duplicating the content. But it is safer to not ignore the error in the initial row insertion.

@justinclift

This comment has been minimized.

Copy link
Member

justinclift commented Dec 10, 2017

Hmmm, I wonder if the reason it's inserting an empty row, is something to do with the way we do "Create a new Record?". (bad cut-n-paste maybe for initial duplication code?)

From memory, when the user presses the "Create new record" button, we insert an empty row first for some reason. I think that's to make sure any default values for fields get created, and we can populate them into the row data fields onscreen for the user. That's purely from really old memory of how I remember it being explained one time. So... don't take that as definitely 100% correct. 😄

@mgrojo

This comment has been minimized.

Copy link
Contributor

mgrojo commented Dec 10, 2017

Well, for this database we have also problems creating new records 😞 At least this time the error is displayed:

Error adding record:
CHECK constraint failed: Table1 (INSERT INTO `main`.`Table1`(`id`,`Date`,`Value`) VALUES (6,'','');)

@mgrojo mgrojo added the SQL label Oct 11, 2018

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