Skip to content
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

[3.x] Remove problematic unused code when formatting SQL #924

Merged
merged 1 commit into from
Jul 28, 2020
Merged

[3.x] Remove problematic unused code when formatting SQL #924

merged 1 commit into from
Jul 28, 2020

Conversation

derekmd
Copy link
Contributor

@derekmd derekmd commented Jul 27, 2020

Fixes: #919
Replaces pull request #923 as per Dries' request

Formatted SQL preview:

before

which should be:

after

Telescope's QueryWatcher always stores an empty array for query parameter bindings.

Telescope::recordQuery(IncomingEntry::make([
'connection' => $event->connectionName,
'bindings' => [],

This causes an issue in NPM package sql-formatter when the query has MySQL session variables referenced with an '@' character. Using the generic SQL:2011 dialect, it's treated like a named placeholder (e.g., for MS SQL Server.)

sqlFormatter.format('select @user_id := id as id from users', {params: []});
// select
//   undefined: = id as id
// from
//   users

This expects a named options parameter as if @user_id is referenced within a WHERE clause, e.g., {params: {user_id: 1}} Missing a named binding value, JavaScript undefined is instead filled in.

Excluding the params option will stop the package from attempting to fill the placeholder.

sqlFormatter.format('select @user_id := id as id from users');
// select
//   @user_id: = id as id
// from
//   users

<query-preview> cleanup

  1. stop passing the bindings empty array to the .format() method since the argument is never used.
  2. Instead of referencing the generic wrapper sqlFormatter.format() public API, pull in the exact StandardSqlFormatter implementation. ES6 classes Db2Formatter, N1qlFormatter, & PlSqlFormatter are also being unnecessarily bundled into Telescope's app.js. This shrinks the production file 813kb to 796kb. There's definitely more room for perf improvement here. e.g.,
    • remove /**! NPM package comments
    • remove bootstrap.js components Telescope doesn't use like Carousel
  3. remove unused Vue component props entry & batch since query data is accessed via a slotted scope.

Telescope's QueryWatcher always stores
an empty array for query parameter
bindings. This causes issues in NPM
package "sql-formatter" when the query
has MySQL session variables referenced
with an '@' character.

sql-formatter's generic SQL syntax handling
will treat the '@' character the same as a
'?' placeholder. 'SELECT @foo := id ...'
without bindings will cause @foo to be
replaced with JavaScript 'undefined' rather
than being left as-is.

1. stop passing the bindings empty array
   to the .format() method since the
   argument is never used.
2. instead of referencing the generic
   'sqlFormatter.format()' public API,
   pull in the exact implementation of
   StandardSqlFormatter. Three other
   SQL dialect implementations were being
   unnecessarily bundled into Telescope's
   app.js distribution. This will shrink
   app.js' file size.
3. remove unused <query-preview> Vue
   component props 'entry' and 'batch'
   since query data is being injected
   through the the slot scope.
@driesvints
Copy link
Member

Thanks @derekmd

@driesvints driesvints requested a review from themsaid July 27, 2020 19:24
@taylorotwell taylorotwell merged commit ac99169 into laravel:3.x Jul 28, 2020
@derekmd derekmd deleted the fix-queries-dump-with-mysql-variable branch July 28, 2020 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Query detail page shows mysql variables as "undefined"
4 participants