Allow custom display formats #1720
Conversation
Allow user to define their own display formats. The SQL code is editable and the combo box changes automatically to custom if the user edits one of the predefined formats. The display format (either custom or predefined) is validated with a regex so at least it is a function containing the column name. Note that this might be fooled in some way, but the users should know what they're doing. See issue #573
There was a problem hiding this comment.
Thanks @mgrojo 👍 This is looking pretty good and seems to be a much expected feature. I made a couple of suggestions which might make sense to take care of before merging - but nothing major 😄
src/ColumnDisplayFormatDialog.cpp
Outdated
| ui->editDisplayFormat->text().contains(QRegExp("[a-z]+[a-z_0-9]* *\\(.*" + QRegExp::escape(sqlb::escapeIdentifier(column_name)) + ".*\\)")))) | ||
| errorMessage = tr("Custom display format must be a function call applied to %1").arg(sqlb::escapeIdentifier(column_name)); | ||
| else if(!pdb.executeSQL(QString("SELECT %1 FROM %2 LIMIT 1").arg(ui->editDisplayFormat->text(), curTable.toString()))) | ||
| errorMessage = tr("Error in custom display format. Message from database engine:\n\n%1").arg(pdb.lastError()); |
There was a problem hiding this comment.
I would suggest one more check here if it doesn't mean too much work: We could make two test queries, one with the display format and one without, and get the column count of each result set. These counts must match before we accept the custom display format. This avoids any problems with extra commas or similar in display formats like upper("field"), 'hi' which add an extra column and therefore shift all the other columns to the right. I feel like this could happen by accident when preparing a complex display format string and if it goes unnoticed it might even cause some data loss.
There was a problem hiding this comment.
I thought about that check, but only testing for one column (could the display format apply to more than one?), but finally thought it was enough with the regexp check. Indeed, I was thinking that your example would be rejected because it does not end in ')' but I've already seen that the regexp does not have to match with all the string. I'll add ^ and $ to the regexp, since I think it wouldn't stop any legitimate case (I cannot think of one). Nevertheless, the column check would add more safety, and better going too far than too short in this case.
There was a problem hiding this comment.
This is ready for a new release.
I've added the column check using a new callback that would avoid making two queries and could have other future uses.
I also changed the regexp so it matches my initial intention. One thing to discuss is whether this regexp would reject any legitimate use. I cannot imagine one so I think we should choose here the safest approach: validating both the form of the query and the results.
There was a problem hiding this comment.
This is ready for a new release.
Trying to grok that. Is this meaning I need to build something? 😄
Added a new callback for executeSQL following the model of sqlite3_exec. Using this, the number of columns is got from the query execution. If it is not one, the custom display format is rejected. The regexp is also narrowed to accept only SQL functions applied to the column name (it now matches the entire string). See issue #573
|
Sorry, I meant for a new review. 😄
|
|
Cool. 😄 |
|
This is looking very good! The callback feature might turn out pretty useful too 👍 One last problem I see is that the regular expression is too strict now. For example these display format isn't working now although it could and probably should: datetime("column")+1 -- the 'tomorrow' format
CAST("bytes" / 1024.0 AS TEXT) || " KB" -- 'convert to kilobytes' format |
|
Ok, I was unable to think of any legitimate case. Then the regexp could be returned to the original form and it would only require a function call applied to the column somewhere in the expression. That would allow your examples and similar ones. |
|
Sounds good 👍 Feel free to just merge it with the new old regular expression 😉 |
Restore the initial "containing" version and make it case insensitive.
|
Whooo Hoooo! Well done @mgrojo. People have been asking for this for ages. 😁 Seemed like it took a large amount of effort too. 👍 |
Allow user to define their own display formats. The SQL code is editable
and the combo box changes automatically to custom if the user edits one
of the predefined formats.
The display format (either custom or predefined) is validated with a regex
so at least it is a function containing the column name. Note that this
might be fooled in some way, but the users should know what they're doing.
See issue #573
The change is pretty simple. The only aspect worthy of review migh be whether the used validation is enough (simple regex and syntax errors).
This feature is important for allowing users to load extensions and used them as display formats. See #1198 and #1716.