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

Sql export #1422

Merged
merged 16 commits into from Jul 7, 2018

Conversation

Projects
None yet
4 participants
@GortiZ6
Copy link
Contributor

GortiZ6 commented Jun 8, 2018

I've added the ability to copy fields from tables in SQL format.
I found much robust and easier copy data in SQL from one DB to another so that if the number of columns differs or if I want to build a single SQL file to share several fields from several table with someone I can do it.

Giuseppe Zizza and others added some commits Dec 18, 2017

Giuseppe Zizza
Giuseppe Zizza
Giuseppe Zizza
- Applied changes requested by mgrojo to uniform code with sqlitebrow…
…ser standards

- Add "history" when closing editor window, but reopen before closing preferences
- Revert some changes done by QtCreator
Giuseppe Zizza
Giuseppe Zizza
Additional changes requested by MKleusberg:
- [CHG] Always add "All files (*)" to filters
- [FIX] Removed unused include
@justinclift

This comment has been minimized.

Copy link
Member

justinclift commented Jun 8, 2018

Thanks @GortiZ6, this sounds interesting. 😄

Hopefully one of our C++ developers has time to review this in the next few days. 😄

@mgrojo

This comment has been minimized.

Copy link
Contributor

mgrojo commented Jun 9, 2018

Thanks @GortiZ6. The functionality will be useful and the implementation seems correct. I only have two concerns:

  • The HTML version is still attached to the clipboard, so if an application gives preference to the HTML version while pasting, the result wouldn't be the expected one. I think this is in practice no problem, because the usual receiving application will be a text editor or simple text tool, but in any case, it leaves me a bit uneasy that the HTML and the text version of the clipboard aren't semantically equivalent. So, what should be done? Not attaching the HTML version? But then most of the work done by the copy method is useless. Should then the new functionality separated in a new copyAsSql method? Not sure about it, so do not change anything until more opinions are heard.
  • It seems that the image data wouldn't restored as BLOB but as a base 64 text. It should be done using blob literals (e.g. X'53514C697465'). Although not sure about whether that would work in other DBMS.
@MKleusberg
Copy link
Member

MKleusberg left a comment

Thanks, @GortiZ6! This is looking pretty good 👍

Just two minor points from my side. Both should be easy to fix 😄

But I agree with Manuel that we need to think about HTML and BLOBs. I haven't put much thought into this yet but would guess that it's best to not attach the HTML version when inSQL is true because the user explicitly selected the SQL option here and this is what we should do then. And I would suggest using the X'...' notation for BLOBs despite the fact that they won't work on all DBMSs -- unlike BASE64 text the X'...' notation would at least produce an error.

@@ -314,35 +322,42 @@ void ExtendedTableWidget::copy(const bool withHeaders)
const QString fieldSepText = "\t";
const QString rowSepText = "\r\n";

QString sqlInsertStatement = QString("INSERT INTO \"%1\" ( '").arg(m->currentTableName().toString());

This comment has been minimized.

@MKleusberg

MKleusberg Jun 10, 2018

Member

Can you use sqlb::escapeIdentifier() here for quoting the table name? This way we have one function which does the quoting which makes changing the quotes relatively easy (somewhere we have an issue for this).

This comment has been minimized.

@GortiZ6

GortiZ6 Jun 12, 2018

Author Contributor

Done

result.append(rowSepText);
htmlResult.append(rowSepHtml);
sqlResult.append("\");\r\n"+sqlInsertStatement);

This comment has been minimized.

@MKleusberg

MKleusberg Jun 10, 2018

Member

Not sure if \r\n is the correct thing here. Because it's not CSV or something like that it might be better to use \r\n only on Windows and \n for all other platforms.

This comment has been minimized.

@GortiZ6

GortiZ6 Jun 12, 2018

Author Contributor

Mhmm do you have any mean do discriminate which line termination to use or should I write an #ifdef __WIN32 #else #endif block?

This comment has been minimized.

@mgrojo

mgrojo Jun 24, 2018

Contributor

In fact aren't C/C++ compilers converting \n to \r\n on Windows platforms? We are assumming that \n would produce a usual new line in several places. I would be surprised to know that those lines are not working under Windows.

Giuseppe Zizza
[CHG] Converted to HEX from Base64 the image SQL export
[CHG] Using sqlb::escapeIdentifier() for table quotation to uniform with the rest of the code
@GortiZ6

This comment has been minimized.

Copy link
Contributor Author

GortiZ6 commented Jun 12, 2018

Ok, I've made the two changes, I agree that the HTML is misleading so for now I simply omit it, we'll figure out if in the future someone comes up with a better idea.

In the mean time I was also checking if there's a way to export a complete table in SQL, as far as I can see you can export the complete database in SQL or a table in CSV/JSON format, so I'm planning to add that as well to this merge request if that's ok for you.

@mgrojo
Copy link
Contributor

mgrojo left a comment

All identifiers should use sqlb::escapeIdentifier. After PR #1436 users will be able to choose the quoting characters they prefer (from the set supported by SQLIte).

}

result.append(escapeCopiedData(headerText));
htmlResult.append(headerText);
sqlInsertStatement.append(escapeCopiedData(headerText));

This comment has been minimized.

@mgrojo

mgrojo Jun 24, 2018

Contributor

sqlb::escapeIdentifier should be used here too for column names instead of escapeCopiedData, and the ' single quotes' removed.

}
result.append(rowSepText);
htmlResult.append("</th></tr>");
sqlInsertStatement.append("') VALUES (\"");

This comment has been minimized.

@mgrojo

mgrojo Jun 24, 2018

Contributor

Values should use ' single quotes' as required by SQL standard.

@mgrojo

This comment has been minimized.

Copy link
Contributor

mgrojo commented Jun 24, 2018

In the mean time I was also checking if there's a way to export a complete table in SQL, as far as I can see you can export the complete database in SQL or a table in CSV/JSON format, so I'm planning to add that as well to this merge request if that's ok for you.

In the export dialog (File -> Export -> Database to SQL File...) you can select the tables to export. In the Database Structure you can drag and drop a row from a table to get the create and insert statements to recreate the table. The same from the Schema column of the DB Schema dock. These features are somewhat hidden, but take a look to them, just in case they fulfil your need or not.

@GortiZ6

This comment has been minimized.

Copy link
Contributor Author

GortiZ6 commented Jun 24, 2018

@mgrojo yes, I saw them (that's one of the reason I did not progress on this task, I was thinking about merging the two, making them less "hidden" and more homogenous with the rest of the exports or just dropping my idea).
Unfortunately I'm quite busy in this period with my job so this has been left a bit aside :( Anyway I'll add the fixes you suggested probably tomorrow so that the code is at least in a shape that reflects the standards :)

@mgrojo

This comment has been minimized.

Copy link
Contributor

mgrojo commented Jul 7, 2018

@GortiZ6 I will be making changes to this part of the code, so I think I will merge this to avoid conflicts and then I will fix the minor issues that are still pending.

Thanks again for the pull request.

@mgrojo mgrojo merged commit cff0795 into sqlitebrowser:master Jul 7, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

mgrojo added a commit that referenced this pull request Jul 7, 2018

Improvements for "Copy as SQL" feature (PR #1422)
- Set the line terminator according to operating system (text and SQL
versions).

- Quote identifiers using the user preference and string literals as 'SQL
standard'.

- Support BLOB literals for any kind of binary data and avoid converting
any kind of image to PNG in the process of setting the BLOB literal in SQL.
@MKleusberg

This comment has been minimized.

Copy link
Member

MKleusberg commented Jul 10, 2018

Many thanks from my side, too, @GortiZ6 😄 And thanks for @mgrojo for the good observations 👍

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