Added the ability to decode base64 and plist data #1716

apjarvis · 2019-01-22T12:13:39Z

Added the ability to decode base64 and 'plist' data structures.

Additional code is in C (not C++) so really needs re-working
but only if of wider interest.

Added the ability to decode 'plist' data structures. Additional code is in C (not C++) so really needs re-working but only if of wider interest.

justinclift · 2019-01-22T12:25:04Z

Interesting. 😄

I'll leave our more experienced C++ developers to review this, but I did notice something a bit weird. The formatting of the code here is extremely inconsistent. For example, some seems to use 2 letter space indent, some what (looks like) 8 space indent, etc.

Is some of this code cut-n-pasted from elsewhere?

Asking specifically because that could be a problem, depending on the license of the er... source code. 😄

apjarvis · 2019-01-22T12:38:32Z

'Interesting' can be taken many ways :-)

The code is original and so no license issues.

While I have cut and pasted (started on Mac, tried to move to Windows though failed setting up the build environment, and finished on Ubuntu) I generally use 2 spaces except in 'switch' statements which I indent more. I have spotted at least one 'rogue' indent but don't think it is worth another commit.

justinclift · 2019-01-22T12:54:00Z

'Interesting' can be taken many ways :-)

Heh Heh Heh. Good point. "Interesting" in this case is a "this looks like it could be useful".

Thanks for the confirm about original code. That's good news. 😄

With the formatting, any chance of making it consistent? From memory, I think we generally go with 4 space indents, but it wouldn't surprise me if there's the occasional bit that isn't. eg one of those things that gets cleaned up over time, when people are touching a given bit of code

With the top section:

//  The following section of code should really be in a separate class
//  but this is only a test!

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <ctype.h>
#include <time.h>

What's that "this is only a test" bit about? 😄

apjarvis · 2019-01-22T13:04:34Z

I love coding standards - there are so many to choose from :-)

OK, I can reformat but I think I will hold fire until I know it is going to be adopted. Also it should really be C++ which would be a bit of a re-write anyway.

Probably the 'Test' comment could come out but maybe wait until it has had a bit of real 'testing'. Currently the code's status is 'it works for me so it must be OK' - so not really an exhaustive test regime.

justinclift · 2019-01-22T13:07:35Z

OK, I can reformat but I think I will hold fire until I know it is going to be adopted.

No worries at all, sounds like decent plan. 😄

@MKleusberg @mgrojo When you have time... (etc)

tiendq · 2019-01-24T03:33:47Z

src/EditDialog.cpp

@@ -21,6 +21,591 @@
 #include <QPainter>
 #include <QClipboard>

+//  The following section of code should really be in a separate class


Why two spaces instead of just one?

Just my style. Goes back to the days of internal documentation and the 'docment' program on CDC mainframes.

tiendq · 2019-01-24T03:37:18Z

src/EditDialog.cpp

+#define ERROR_INVALID_OBJECT_LENGTH  5
+#define ERROR_INVALID_REFERENCE      6
+
+typedef struct KEY {


typedef is unnecessary.

Yes, but only if you also change lines 60 and 234. Personally I think typdefs make the code more readable.

struct abc {}; without typedef is exactly same as class abc {}; so hardly see readability issue here, it's just C style IMO :)

tiendq · 2019-01-24T03:40:01Z

src/EditDialog.cpp

+
+int outputText(CONFIG *cfg, const char *text)
+{
+  size_t textLength = strlen((const char *)text);


Is this casting necessary?

No, cast removed, left over from when the parameter was 'unsigned'.

tiendq · 2019-01-24T03:41:26Z

src/EditDialog.cpp

+  while (textLength >= availableSpace) {
+    cfg->outputBufferLength += textLength + 1024;
+    cfg->outputBuffer = (unsigned char *)realloc(cfg->outputBuffer, cfg->outputBufferLength);
+    if (cfg->outputBuffer == NULL)


Is nullptr more modern? :)

NULL is C, nullptr is C++, not sure if it is now also in C

tiendq · 2019-01-24T03:45:45Z

Just made some review comments and found that the PR is pure C code :)

And yes, there're many coding standards but we should love just one of them in a project :)

mgrojo · 2019-01-26T23:05:10Z

Thanks for the contribution, @apjarvis. I have a suggestion. I think this would be more useful as an SQLite extension. Then the user could use the conversion functions in display formats or in queries. This has also the advantage, that you wouldn't have to convert the code to C++ and could select your own coding style. We could ship this extension for our users, but it could also be used in other SQLite projects.

This doesn't mean that the current approach is rejected. For example, other members may have other opinions or myy approach could have some unnoticed difficulties.

justinclift · 2019-01-27T06:02:46Z

As an extension, it could be added to our new extensions directory, alongside Liam Healy's math ones. 😄

https://github.com/sqlitebrowser/sqlitebrowser/tree/master/src/extensions

apjarvis · 2019-01-27T09:13:19Z

An extension might be the way to go - I'll now have to learn how to do extensions and how to interface to the browser! Currently my code is 'display only' and I expect that as an extension input parsing would also be wanted. For base64 no problem but 'plist' format may be more of a challenge.

mgrojo · 2019-01-27T10:46:40Z

Currently my code is 'display only' and I expect that as an extension input parsing would also be wanted. For base64 no problem but 'plist' format may be more of a challenge.

Our display formats are 'display only' too (see issue #1198). But you can provide both conversions: from base64 and to base64. By the way, I suppose you are using base64 as a codification of text, as you use the text editor for displaying it. The advantage of a display format would be that the base64 decodification could return either a BLOB or a text, and then the hexadecimal editor or the text editors would be automatically selected in Edit Database Cell.

Once we have the conversions as SQLite functions it's easy to add them to the Browser, see ColumnDisplayFormatDialog.cpp. But we should either allow user to define custom formats (see #573) or detect whether an extension is loaded for providing the display format. Another option is adding the conversion functions inside DB4S itself. They're simple enough to be a good option. We do that for our internal regexp extension (see sqlitedb.cpp:155).

By the way, I could not test the plist feature, as I don't have one file for it. Could you attach one?

apjarvis · 2019-01-29T16:03:03Z

OK, I have created an 'extensions-plist.c' file with all the C code that I had previously added to 'EditDialog.cpp'. The file compiles as per notes in the 'extension-functions.c' and I have added the wrappers as per that file. However I cannot see where to put the compiled library or get the browser to load it so I must be missing something.
I have looked at the 'ColumnDisplayFormatDialog' and see how to add entries. However all the implementations appear to reference SQL primitives and I cannot get either my 'plist' or any from 'extension-functions.c' to work so I am going round in circles.
Any guidance would be appreciated.

chrisjlocke · 2019-01-29T17:14:39Z

Have you read this page and this one?
Also, "Extension loading must be enabled using sqlite3_enable_load_extension() or sqlite3_db_config(db,SQLITE_DBCONFIG_ENABLE_LOAD_EXTENSION,1,NULL) prior to calling this API, otherwise an error will be returned."
But I'm not 100% sure what error you're getting and what part you've got an issue with, so excuse me.

mgrojo · 2019-01-29T20:58:56Z

@apjarvis Do you already have a compiled shared library? If so, you can use Edit > Preferences > Extensions to load it. There isn't currently a location for automatically adding extensions, althugh it has been talked about in the issue abouth the Math extension #1224.

Then you'l be able to use the new functions from SQL. You can then add a predefined display format programatically, but you'll have to solve the issue of detecting whether the extension has been loaded. If my pull request #1720 is merged, you'll be able to define the display format manually inside the application and then save it to a project file.

Has this been helpful? Don't doubt to ask again if something is unclear.

apjarvis · 2019-01-30T13:34:46Z

Thanks Chris for the two links. I had seen those and based on those I was trying to find where the math extensions were loaded so that I could hang off the end of those. I couldn't find anything however the subsequent comment revealed to me the Edit > Preferences > Extensions mechanism so I can now load my binary which seems to be loaded automatically on subsequent runs. If I load my test data base I get the following errors whether or not I have my extension loaded :-(

13:23:51: Starting /home/paul/sqlitebrowser/src/sqlitebrowser...
libpng warning: iCCP: known incorrect sRGB profile
libpng warning: iCCP: known incorrect sRGB profile
libpng warning: iCCP: known incorrect sRGB profile
libpng warning: iCCP: known incorrect sRGB profile
libpng warning: iCCP: known incorrect sRGB profile
Empty filename passed to function
Gtk-Message: 13:24:11.464: GtkDialog mapped without a transient parent. This is discouraged.
Sqlite parse error:
line 1:225: unexpected token: partition
(
CREATE TABLE cfurl_cache_response(entry_ID INTEGER PRIMARY KEY AUTOINCREMENT UNIQUE, version INTEGER, hash_value INTEGER, storage_policy INTEGER, request_key TEXT UNIQUE, time_stamp NOT NULL DEFAULT CURRENT_TIMESTAMP, partition TEXT)
)

Not a good sign, however it gets worse... If I select my 'plist' extension through Edit Display Format every column of the data changes to 'Loading' and just sits there. I have added a printf right at the start of my parse function but nothing gets written out (I'm using QT Creator). Setting up a breakpoint in my dynamically loaded code is not trivial. Any pointers would be appreciated.

Should I close this pull request and do another with the current code?

'Edit display format'. Compiles but gets stuck when 'plist' is selcted, however selecting a different format clears the issue.

mgrojo · 2019-01-30T20:03:05Z

Once you have loaded the extension, you can test it simply by executing SQL code calling the function, something like this:

SELECT myFunction(myField) FROM myTable;

I've seen that you have changed your branch with these changes. I'll take a look.

mgrojo · 2019-01-30T20:38:30Z

The problem is that you've called your function plist and not parsePlist.

I've been able to run it, but I don't have data to test it. This one behaves well and returns NULL:

SELECT plist(NULL);

but this random test:

SELECT plist('plist');
results in:

Execution finished with errors.

-- At line 10:
select plist('plist');
-- Result: Llam
select plist('plist');

At the terminal:
plistFunc, argc = 1plist

I've been able to set a breakpoint in gdb for plistFunc.

Hope this helps.

item to be parsed.

apjarvis · 2019-02-01T10:57:59Z

Thanks for all the assistance - I'm slowly learning about SQLite3! This is the first time that I have ever used it and I have made a test database which is attached. I am not 100% certain that it is correct though it does seems to display OK.

I was just writing up my understanding of how loading extensions worked when I spotted that I was not correctly calculating the length of the object to parse. I have uploaded my latest changes.

test2.zip

mgrojo · 2019-02-01T18:38:54Z

Congratulations for the progress, @apjarvis. I've been able to test with your database and it's working fine. There are some issues, though, when you pass invalid data. It should return NULL, '', the input or whatever you consider meaningful for those cases, but something predictable. Currently these queries give random results:

SELECT plist('plist');
SELECT plist('33');

This one finishes with error, but no indication of which is the problem:

SELECT plist(X'1234ABCD');

This one even crashed DB4S:

SELECT plist(33);

mgrojo · 2019-02-01T18:44:14Z

src/ColumnDisplayFormatDialog.cpp

@@ -17,6 +17,7 @@ ColumnDisplayFormatDialog::ColumnDisplayFormatDialog(const QString& colname, QSt
    ui->comboDisplayFormat->addItem(tr("Hex number"), "hex");
    ui->comboDisplayFormat->addItem(tr("Octal number"), "octal");
    ui->comboDisplayFormat->addItem(tr("Round number"), "round");
+    ui->comboDisplayFormat->addItem(tr("Plist"), "plist");


This will require some existence test for inclusion in DB4S, but don't know how to test for existence of a loaded SQLite function (maybe only executing it). If we include my PR #1720, it wouldn't be really necessary, since users could enter a custom function as display format and save it to the project file.

…ossible memory leak.

apjarvis · 2019-02-01T21:48:55Z

I think that I have stopped it parsing anything that is not a Blob or Text and if not of this type return NULL. If the parsing fails I return the original but as text so maybe I need something more generic here so I can return a Blob if appropriate.
I'm already looking at the Base64 which was part of the original checkin. Should this now be a separate extension?

mgrojo · 2019-02-27T19:39:13Z

@justinclift @karim What if we add this new extension as an option in the Windows installer? Is that possible?

justinclift · 2019-02-27T20:46:00Z

Sounds feasible for 3.12.0 (the next major release). 😄

apjarvis · 2019-02-28T11:48:30Z

I have just done a re-base and when I build I get the following:

g++ -c -pipe -std=c++11 -g -Wall -W -D_REENTRANT -fPIC -DQT_PRINTSUPPORT_LIB -DQT_WIDGETS_LIB -DQT_GUI_LIB -DQT_NETWORK_LIB -DQT_CONCURRENT_LIB -DQT_XML_LIB -DQT_CORE_LIB -DQT_QML_DEBUG -I. -I../libs/antlr-2.7.7 -I../libs/qhexedit -I../libs/qcustomplot-source -I../libs/qscintilla/Qt4Qt5 -I../../sqlitebrowser -I../../Qt/5.12.0/gcc_64/include -I../../Qt/5.12.0/gcc_64/include/QtPrintSupport -I../../Qt/5.12.0/gcc_64/include/QtWidgets -I../../Qt/5.12.0/gcc_64/include/QtGui -I../../Qt/5.12.0/gcc_64/include/QtNetwork -I../../Qt/5.12.0/gcc_64/include/QtConcurrent -I../../Qt/5.12.0/gcc_64/include/QtXml -I../../Qt/5.12.0/gcc_64/include/QtCore -Idebug -isystem /usr/include/libdrm -I.ui -I../../Qt/5.12.0/gcc_64/mkspecs/linux-g++ -o debug/AddRecordDialog.o AddRecordDialog.cpp
AddRecordDialog.cpp:343:10: fatal error: AddRecordDialog.moc: No such file or directory
#include "AddRecordDialog.moc"
^~~~~~~~~~~~~~~~~~~~~
compilation terminated.
Makefile.Debug:7635: recipe for target 'debug/AddRecordDialog.o' failed

What have I missed?

justinclift · 2019-02-28T12:09:48Z

Looks like we've recently added a new "Add Record" dialog to the development branch, which is missing something important for compiling to work. 😦

justinclift · 2019-02-28T12:10:54Z

Hang on.. the last change to AddRecordDialog.cpp was 17 days ago. Guess it's something else then?

mgrojo · 2019-02-28T22:33:02Z

Don't know. Maybe you need to clean the build?

make clean ; make

If that doesn't work, you could push the branch as is and we would see whether Travis compile it and I could check where is the problem.

SQLite allows some keywords to be used for table names and some other keywords to be used for column names without using any quotes. Our grammar parser needs to know which keywords are allowed and which are not. In the two lists (one for table names and one for column names) there were a few errors and omissions. This commit should fix them. This was pointed out in #1716.

apjarvis · 2019-03-01T09:26:09Z

make clean ; make did not fix, same error so I have pushed the branch again. It is probably my error with the rebase.

mgrojo

The branch has compiled in Travis, so I gues it is ready fo merging, except for these two minor changes that have been left over from the previous commits. I'll squash and merge your branch once this two minor issues are addressed.

Thanks again!

mgrojo · 2019-03-01T20:16:34Z

src/ColumnDisplayFormatDialog.cpp

@@ -37,7 +37,6 @@ ColumnDisplayFormatDialog::ColumnDisplayFormatDialog(DBBrowserDB& db, const sqlb
    ui->comboDisplayFormat->addItem(tr("Custom"), "custom");

    ui->labelDisplayFormat->setText(ui->labelDisplayFormat->text().arg(column_name));
-


You should undo this change, so the PR has a clean history when squashed and merged.

src/ColumnDisplayFormatDialog.h

apjarvis · 2019-03-05T08:53:27Z

Sorry for the delay, I think that I have made the requested changes.

mgrojo · 2019-03-06T19:54:07Z

Thanks again, @apjarvis, and congratulations, your PR is merged and the formats extension is now part of the SQLite extensions that we provide for our users. Any interest in adding it to the Windows installer as was done for the math extension? If yes, take a look at 161e71b and #1224. Not sure what approach we should take for macOS or Linux.

See PR #1716

justinclift · 2019-03-07T01:41:34Z

The macOS builds have an equivalent line too: 😄

sqlitebrowser/installer/macos/build_sqlitebrowser_nightly.sh

Line 102 in ef90d30

    
           gcc -I/usr/local/opt/sqlitefts5/include -L/usr/local/opt/sqlitefts5/lib -fno-common -dynamiclib src/extensions/extension-functions.c -o src/DB\ Browser\ for\ SQLite.app/Contents/Extensions/math.dylib >>$LOG 2>&1

It should (in theory) just be a matter of duplicating that line, then updating the input and output file names. 😄

justinclift · 2019-03-10T09:52:46Z

I'm ok to add this to the macOS and Windows nightly builds, so it's compiled and installed with those.

The last "missing piece" is that DB4S doesn't yet automatically load extension files found in our extension installation folder. If we add that at startup, our macOS and Windows users will be able to make use of this and the other bundled extension (math functions), without any manual steps. That'd probably be a win. 😄

Added new argument to command line for saving a value for a setting and not only running it temporarily: -O/--save-option Fix string lists settings so they are saved as an actual QStringList and not a string. This assumes these settings are always named "*list". Currently they, and only they, are. Together with --quit, this argument allows saving the extensions/list and other settings after an installation depending on options selected by user in the installer. For example, running (Linux syntax): ./sqlitebrowser --save-option extensions/list=/path/to/libsqlitefunctions.so,/path/to/libsqlite-formats.so --quit will save the two extensions to the preferences and they will be automatically loaded every time DB4S is loaded. See issue #1224 and PR #1716.

mgrojo · 2019-03-10T12:55:26Z

The last "missing piece" is that DB4S doesn't yet automatically load extension files found in our extension installation folder. If we add that at startup, our macOS and Windows users will be able to make use of this and the other bundled extension (math functions), without any manual steps. That'd probably be a win. 😄

I wouldn't know how to implement that in a portable way, but I've come up with other idea which was easy to implement. I've added a new command-line option similar to --option, but for immediately saving the passed value. Using -O or --save-option together with --quit, we can save the selected extensions so they will be automatically loaded at startup. The installer has to call DB4S with the --save-option extensions/list=\path\to\extension1.dll,\path\to\extension2.dll --quit arguments (adapted to the OS particular conventions). That could be useful in the future for saving other options selected in the installer.

mgrojo · 2019-03-10T13:02:22Z

@apjarvis I've seen some issues in the toBase64 function.

Sometimes there are garbage at the end of the output.
BLOBs aren't converted to base64. Maybe this isn't implemented, but it would be useful.

Please, let me know if you need more details.

justinclift · 2019-03-11T03:55:58Z

I wouldn't know how to implement that in a portable way ...

It'd probably be a matter of figuring out which directory the application binary is running in (as a reference point), then looking for extensions in a directory relative to that.

On Windows, it'd be in extensions under the directory the binary is running in.
On macOS, it'd be ../Extensions under the directory the binary is running in.

Not sure where they'd generally go for Linux/BSD/etc. Likely different places depending on distribution, etc. We'd could probably skip trying on those ones (at least initially).

but I've come up with other idea which was easy to implement.

Heh Heh Heh. The extra options are likely useful, but we have no way to automatically run DB4S on install with macOS, so it's not a fully working solution all around. We should be able to make it work on Windows though, as we used to offer to launch DB4S in a dialog anyway. It'd just be a variation on that. 😉

mgrojo · 2019-03-11T20:34:20Z

Heh Heh Heh. The extra options are likely useful, but we have no way to automatically run DB4S on install with macOS, so it's not a fully working solution all around. We should be able to make it work on Windows though, as we used to offer to launch DB4S in a dialog anyway. It'd just be a variation on that.

Is it not possible to run a post-install script in the macOS installer? Then we could at least bundle a script to configure the extensions and direct users to run it.

justinclift · 2019-03-23T11:33:51Z

Is it not possible to run a post-install script in the macOS installer?

Nope. The macOS installer literally just copies the contents of the .app bundle (collection of files in a special directory structure) across to the user's PC. Nothing gets run or executed.

apjarvis added 2 commits January 22, 2019 09:26

Added the ability to decode base64 data.

1efaa6d

Added the ability to decode 'plist' data structures. Additional code is in C (not C++) so really needs re-working but only if of wider interest.

Fix incorrect parameter type

4845e77

justinclift added the enhancement Feature requests. label Jan 22, 2019

tiendq reviewed Jan 24, 2019

View reviewed changes

mgrojo mentioned this pull request Jan 27, 2019

Allow custom display formats #1720

Merged

Extract out plist processing as an extension and invoke via

10c6409

'Edit display format'. Compiles but gets stuck when 'plist' is selcted, however selecting a different format clears the issue.

Corrected name confusion and now also pass the the object length of

add4599

item to be parsed.

mgrojo reviewed Feb 1, 2019

View reviewed changes

apjarvis added 2 commits February 1, 2019 21:37

Only parse if Text or Blob, return input if parsing fails, remove a p…

fb9b777

…ossible memory leak.

Oops, rushing in to Base64 already

968e96f

Correction to header length test

f7a6574

Merge remote-tracking branch 'upstream/master'

fc39add

Removed extra format function definitions

d05be72

mgrojo requested changes Mar 1, 2019

View reviewed changes

Fix minor layout error

ae75f79

mgrojo approved these changes Mar 6, 2019

View reviewed changes

mgrojo merged commit c8318a1 into sqlitebrowser:master Mar 6, 2019

mgrojo added the sqlite-extension SQLite runtime loadable extensions label Mar 6, 2019

mgrojo added a commit that referenced this pull request Mar 6, 2019

Give appropriate compilation instruction for extension-formats

ef90d30

See PR #1716

justinclift added the response requested label Mar 10, 2019

mgrojo mentioned this pull request May 16, 2019

Add support for displaying BSON data #1895

Open

mgrojo mentioned this pull request Jul 16, 2019

Create SQLite extensions pack installer for Windows #1930

Open

mgrojo mentioned this pull request Nov 12, 2023

[Feature]: Add ability to view & edit binary property lists #3503

Open

		@@ -37,7 +37,6 @@ ColumnDisplayFormatDialog::ColumnDisplayFormatDialog(DBBrowserDB& db, const sqlb
		ui->comboDisplayFormat->addItem(tr("Custom"), "custom");

		ui->labelDisplayFormat->setText(ui->labelDisplayFormat->text().arg(column_name));

Added the ability to decode base64 and plist data #1716

Added the ability to decode base64 and plist data #1716

Conversation

apjarvis commented Jan 22, 2019

justinclift commented Jan 22, 2019

apjarvis commented Jan 22, 2019

justinclift commented Jan 22, 2019

apjarvis commented Jan 22, 2019

justinclift commented Jan 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiendq Jan 24, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiendq commented Jan 24, 2019

mgrojo commented Jan 26, 2019

justinclift commented Jan 27, 2019

apjarvis commented Jan 27, 2019

mgrojo commented Jan 27, 2019

apjarvis commented Jan 29, 2019

chrisjlocke commented Jan 29, 2019 • edited

mgrojo commented Jan 29, 2019

apjarvis commented Jan 30, 2019

mgrojo commented Jan 30, 2019

mgrojo commented Jan 30, 2019

apjarvis commented Feb 1, 2019

mgrojo commented Feb 1, 2019

Choose a reason for hiding this comment

apjarvis commented Feb 1, 2019

mgrojo commented Feb 27, 2019

justinclift commented Feb 27, 2019

apjarvis commented Feb 28, 2019

justinclift commented Feb 28, 2019

justinclift commented Feb 28, 2019

mgrojo commented Feb 28, 2019

apjarvis commented Mar 1, 2019

mgrojo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apjarvis commented Mar 5, 2019

mgrojo commented Mar 6, 2019

justinclift commented Mar 7, 2019

justinclift commented Mar 10, 2019

mgrojo commented Mar 10, 2019 • edited

mgrojo commented Mar 10, 2019

justinclift commented Mar 11, 2019

mgrojo commented Mar 11, 2019

justinclift commented Mar 23, 2019

tiendq Jan 24, 2019 •

edited

chrisjlocke commented Jan 29, 2019 •

edited

mgrojo commented Mar 10, 2019 •

edited