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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql-formatter crash on SQL casts, tilde, square brackets, $$ delimiter #35

Closed
karlhorky opened this issue Nov 26, 2023 · 10 comments 路 Fixed by #39
Closed

sql-formatter crash on SQL casts, tilde, square brackets, $$ delimiter #35

karlhorky opened this issue Nov 26, 2023 · 10 comments 路 Fixed by #39

Comments

@karlhorky
Copy link
Contributor

karlhorky commented Nov 26, 2023

Hi @Sec-ant , hope you're well! 馃憢

(very similar to #34, maybe the same issue)

sql-formatter crashes with prettier-plugin-embed, embedded SQL in JavaScript/TypeScript template string and prettier-plugin-sql, when a cast is used:

a.sql

SELECT '[]'::jsonb;

a.js

function up(sql) {
  await sql`SELECT '[]'::jsonb;`;
}
$ pnpm prettier a.sql --write
a.sql 58ms

$ pnpm prettier a.js --write
a.js
Error: Parse error: Unexpected "::jsonb;" at line 1 column 12
    at TokenizerEngine.createParseError (file:///Users/k/p/project/node_modules/sql-formatter/lib/lexer/TokenizerEngine.js:53:12)
    at TokenizerEngine.tokenize (file:///Users/k/p/project/node_modules/sql-formatter/lib/lexer/TokenizerEngine.js:35:22)
    at Tokenizer.tokenize (file:///Users/k/p/project/node_modules/sql-formatter/lib/lexer/Tokenizer.js:16:47)
    at LexerAdapter.tokenize (file:///Users/k/p/project/node_modules/sql-formatter/lib/parser/createParser.js:16:76)
    at LexerAdapter.reset (file:///Users/k/p/project/node_modules/sql-formatter/lib/parser/LexerAdapter.js:17:24)
    at Parser.feed (/Users/k/p/project/node_modules/nearley/lib/nearley.js:281:15)
    at Object.parse (file:///Users/k/p/project/node_modules/sql-formatter/lib/parser/createParser.js:26:18)
    at Formatter.parse (file:///Users/k/p/project/node_modules/sql-formatter/lib/formatter/Formatter.js:32:49)
    at Formatter.format (file:///Users/k/p/project/node_modules/sql-formatter/lib/formatter/Formatter.js:25:22)
    at formatDialect (file:///Users/k/p/project/node_modules/sql-formatter/lib/sqlFormatter.js:78:57)
a.js 40ms (unchanged)

Other casts that also fail:

SELECT '[]'::jsonb->0;
SELECT ARRAY['5', '6']::int[];

Tildes

This also crashes similarly on the ~ (tilde) operator:

SELECT 'ABC' ~ '^[A-Z]{3}$';

Error:

Error: Parse error: Unexpected "~ '^ABC$';" at line 1 column 14

Square brackets for PostgreSQL exact length arrays

Another failing pattern:

CREATE TABLE x(
        y integer[5]
  )
Error: Parse error: Unexpected "[5]
  )" at line 2 column 18

$$ delimiter


At first, from the error messages, I thought it was the lack of support for casts or tilde operators or square brackets in sql-formatter, but they are all supported with prettier-plugin-sql when used directly with .sql files.

My versions and config, maybe I've configured something incorrectly? 馃

package.json

    "prettier": "3.1.0",
    "prettier-plugin-embed": "0.2.5",
    "prettier-plugin-sql": "0.16.0",

prettier.config.mjs

/** @type {import('prettier').Config} */
const prettierConfig = {
  plugins: [
    'prettier-plugin-tailwindcss',
    'prettier-plugin-embed',
    'prettier-plugin-sql',
  ],
  // Avoid excessive diffs on changes to MDX files
  proseWrap: 'never',
  singleQuote: true,
  trailingComma: 'all',
};

/** @type {import('prettier-plugin-embed').PrettierPluginEmbedOptions} */
const prettierPluginEmbedConfig = {
  embeddedSqlIdentifiers: ['sql'],
};

/** @type {import('prettier-plugin-sql').SqlBaseOptions} */
const prettierPluginSqlConfig = {
  language: 'postgresql',
  keywordCase: 'upper',
  // - Wrap all parenthesized expressions to new lines (eg. `INSERT` columns)
  // - Do not wrap foreign keys (eg. `REFERENCES table_name (id)`)
  // - Do not wrap column type expressions (eg. `VARCHAR(255)`)
  expressionWidth: 8,
};

const config = {
  ...prettierConfig,
  ...prettierPluginEmbedConfig,
  ...prettierPluginSqlConfig,
};

export default config;

cc @JounQin

@karlhorky karlhorky changed the title sql-formatter crash on ::jsonb SQL cast sql-formatter crash on SQL casts Nov 26, 2023
@karlhorky karlhorky changed the title sql-formatter crash on SQL casts sql-formatter crash on SQL casts and tilde Nov 26, 2023
@karlhorky karlhorky changed the title sql-formatter crash on SQL casts and tilde sql-formatter crash on SQL casts, tilde, square brackets Nov 26, 2023
@karlhorky
Copy link
Contributor Author

Oh interesting, I just upgraded to sql-formatter@14.0.0, which offers better error messages, and it hinted that the dialect in my config above is not being passed through prettier-plugin-sql to sql-formatter:

This likely happens because you're using the default "sql" dialect.
If possible, please select a more specific dialect (like sqlite, postgresql, etc).

@karlhorky
Copy link
Contributor Author

Patching sql-formatter lib/sqlFormatter.js file to hard-code the 'postgresql' dialect / language as below seems to work

-const canonicalDialectName = dialectNameMap[cfg.language || 'sql'];
+const canonicalDialectName = dialectNameMap['postgresql'];

Maybe the option is being recognized / passed incorrectly through prettier-plugin-sql when prettier-plugin-embed is used?

@karlhorky karlhorky changed the title sql-formatter crash on SQL casts, tilde, square brackets sql-formatter crash on SQL casts, tilde, square brackets, $$ delimiter Nov 26, 2023
@Sec-ant
Copy link
Owner

Sec-ant commented Nov 27, 2023

sql is a dialect name, so when it is used as an identifier, its following template is mapped to sql.

Check #26 and the introduction of embeddedOverrides in README to properly map sql to postgresql.

A bit of explanation: I want to make this plugin be able to format different languages (dialects) in a single file. If we're to support language overriding directly, we need to detect whether the user has explicitly set the language option. However this option has a default value in prettier-plugin-sql so we cannot decide if it is explicitly set, and it leads us here.

@JounQin
Copy link

JounQin commented Nov 27, 2023

Why not embedLanguage then? I'm not familiar with prettier-plugin-embed, just saying.

@Sec-ant
Copy link
Owner

Sec-ant commented Nov 27, 2023

Why not embedLanguage then? I'm not familiar with prettier-plugin-embed, just saying.

Definitely a choice if we add embeddedSqlLanguage or sth like that. But prettier-plugin-sql is currently the only plugin that supports different languages in just one plugin. So 1), I'd like to avoid adding additional options to prettier-plugin-embed just for a specific plugin as much as possible, it can make this plugin less maintainable. 2) language and database are 2 options already provided by prettier-plugin-sql, adding another option embeddedSqlLanguage may confuse users.

Given the rationale in #26 and this issue. I'm thinking maybe we should disable the identifier -> dialect mapping by default and respect the language or database setting of prettier-plugin-sql for all identifiers. And of course we'll then need another option to enable auto mapping.

@karlhorky
Copy link
Contributor Author

Given the rationale in #26 and this issue. I'm thinking maybe we should disable the identifier -> dialect mapping by default and respect the language or database setting of prettier-plugin-sql for all identifiers

Without knowing too many details, this seems like a good approach.

It would apply 'postgresql' for the sql identifier for the following configuration, which would seem to make sense to me:

const config = {
  plugins: ['prettier-plugin-embed', 'prettier-plugin-sql'],
  embeddedSqlIdentifiers: ['sql'],
  language: 'postgresql',
};

And of course we'll then need another option to enable auto mapping.

What about instead discarding the auto mapping altogether and requiring the user to set language or database if they want something other than the 'sql' default?

@Sec-ant
Copy link
Owner

Sec-ant commented Nov 27, 2023

What about instead discarding the auto mapping altogether and requiring the user to set language or database if they want something other than the 'sql' default?

The reason why I keep auto mapping is that I want this plugin to be able to format various dialects based on the identifiers in a single file. language and database will take effect for the whole file at the minimal, which means you cannot configure which identifier maps to which dialect for each identifier.

I will add a new option to control the auto mapping behavior and disable it by default. This is a breaking change but it seems more reasonable and intuitive given the confusion caused. I plan to release it in the official v0.3.0.

Edit:

which means you cannot configure which identifier maps to which dialect for each identifier.

Oh wait, maybe I'm wrong 馃ゲ, we can use embeddedOverrides to achieve that already. In that case, I can discard automapping altogether.

Sec-ant added a commit that referenced this issue Nov 27, 2023
fixes: #35, #34, #26

BREAKING CHANGE: sql identifier -> dialect auto mapping discarded
@Sec-ant
Copy link
Owner

Sec-ant commented Nov 27, 2023

@karlhorky I made a PR #39. You can test that branch with the codesandebox-ci built package: https://ci.codesandbox.io/status/Sec-ant/prettier-plugin-embed/pr/39

@karlhorky
Copy link
Contributor Author

It's working for me 馃檶 for the ::jsonb->0 test case above

package.json

"prettier-plugin-embed": "https://pkg.csb.dev/Sec-ant/prettier-plugin-embed/commit/dc1eb180/prettier-plugin-embed"

Sec-ant added a commit that referenced this issue Nov 27, 2023
fixes: #35, #34, #26

BREAKING CHANGE: sql identifier -> dialect auto mapping discarded
@karlhorky
Copy link
Contributor Author

Thanks for #39 and the merge + publish!

I can confirm this is working in prettier-plugin-embed@0.3.0

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 a pull request may close this issue.

3 participants