Placeholder replacement #14

ukupat · 2016-11-02T16:04:27Z

I created two different placeholder token regexes (based on if placeholder has a name or not), but there is only one token type: PLACEHOLDER.

I haven't updated README.md yet, lets see what is your opinion on current solution.

Info about N1QL parameterized queries http://developer.couchbase.com/documentation/server/current/sdk/n1ql-query.html

I documented params cfg to be a object, but actually it can be either Array or Object.

See http://developer.couchbase.com/documentation/server/current/sdk/n1ql-query.html

coveralls · 2016-11-02T16:07:31Z

Coverage remained the same at 100.0% when pulling 861c22a on placeholder-replacement into acd77b6 on master.

nene · 2016-11-07T12:08:10Z

src/core/Tokenizer.js

-        this.QUOTED_VARIABLE_REGEX = this.createVariableRegex(cfg.variableTypes, this.createStringPattern(cfg.stringTypes));
+        this.INDEXED_PLACEHOLDER_REGEX = this.createPlaceholderRegex(cfg.indexedPlaceholderTypes);
+        this.PLAIN_NAMED_PLACEHOLDER_REGEX = this.createPlaceholderRegex(cfg.namedPlaceholderTypes, "[a-zA-Z0-9._$]+");
+        this.STRING_NAMED_PLACEHOLDER_REGEX = this.createPlaceholderRegex(


I don't really like the name PLAIN_NAMED. Perhaps better IDENT_NAMED to emphasize that the name is an identifier.

Speaking of identifiers though, the regex [a-zA-Z0-9._$]+ isn't fully covered by tests - we don't test for the special characters ., _ and $. I wonder, do we even really need to support . and $.

nene · 2016-11-07T12:10:09Z

src/core/Tokenizer.js

-            type: sqlTokenTypes.VARIABLE,
-            regex: this.QUOTED_VARIABLE_REGEX
+            type: sqlTokenTypes.PLACEHOLDER,
+            regex: this.INDEXED_PLACEHOLDER_REGEX
        });
    }


We could generalize these three methods - the only difference between them is the regex.

Not sure if we want to do it now when we have two different placeholder types. WDYT @nene ?

Added a different suggestion which might need a different generalization of the same thing.

nene · 2016-11-07T12:17:21Z

test/N1qlFormatterTest.js

+            "  third,\n" +
+            "  first;\n"
+        );
+    });


I think we should order other tests also similarly, so that all tests for @variables are grouped together like the above tests for $variables.

nene · 2016-11-07T12:18:59Z

test/StandardSqlFormatterTest.js

+                0: "first",
+                1: "second",
+                2: "third"
+            }


I think we should also have a test that uses plain array instead of object with numeric keys.

nene · 2016-11-07T12:33:32Z

src/core/Params.js

+        if (!this.params) {
+            return placeholder;
+        }
+        const matches = placeholder.match(/\w+(\s+\w+)*/);


I don't really understand why this regex is the way it is.

I understand that the goal is to detect whether it's a named placeholder. However it doesn't quite work - for example when I have a placeholder that does not consist of just spaces and word characters:

SELECT @'$^&*'

It might be simpler to differentiate between named and indexed placeholders inside tokenizer.

Another thing to consider is escape sequences inside strings:

SELECT @"hello \" world"

coveralls · 2016-11-08T12:23:20Z

Coverage remained the same at 100.0% when pulling b1992a0 on placeholder-replacement into acd77b6 on master.

# Conflicts: # src/core/Formatter.js

ukupat · 2016-11-08T14:04:06Z

@nene covered your requested changes.

coveralls · 2016-11-08T14:05:37Z

Coverage remained the same at 100.0% when pulling e606409 on placeholder-replacement into d60431f on master.

nene · 2016-11-10T13:48:01Z

src/core/Params.js

-        if (matches) {
-            return this.params[matches[0]];
+        if (key) {
+            return this.params[key];


We really have three placeholder token types: STRING, INDENT, INDEXED. Currently the INDEXED and INDENT are thrown together (although they're really quite different). By having three distinct types, we could express the three different choices in much clearer way:

switch(type) { case INDEXED: case STRING: case IDENT: }

Also, I think parsing the contents of the string and indent placeholders should really belong to Tokenizer. The Formatter should not care about the specific syntax that placeholders have. It's also common practice for Lexer to provide both a raw value and some computed value of a token like string. In that scenario we actually won't need 3 token types as STRING and INDENT can be represented with one.

Finally I still think that we should take at least some minimal care about escaping. As the Lexer allows escape sequences like \" in strings, the parameter replacement should also support them.

Also, we should add some additional tests for the quoted placeholders, something like testing:

SELECT :"^*& weird \" var "

Added one test @nene

nene · 2016-11-10T13:49:37Z

src/core/Tokenizer.js

-            type: sqlTokenTypes.VARIABLE,
-            regex: this.QUOTED_VARIABLE_REGEX
+            type: sqlTokenTypes.PLACEHOLDER,
+            regex: this.INDEXED_PLACEHOLDER_REGEX
        });
    }


Added a different suggestion which might need a different generalization of the same thing.

…placeholders

ukupat · 2016-11-11T12:00:39Z

I did my best to resolve @nene requested changes.

coveralls · 2016-11-11T12:00:39Z

Coverage remained the same at 100.0% when pulling 2d5c48f on placeholder-replacement into d60431f on master.

coveralls · 2016-11-11T12:11:34Z

Coverage remained the same at 100.0% when pulling 9ac4d75 on placeholder-replacement into d60431f on master.

coveralls · 2016-11-11T16:40:06Z

Coverage remained the same at 100.0% when pulling 2f76764 on placeholder-replacement into d60431f on master.

coveralls · 2016-11-14T11:32:28Z

Coverage remained the same at 100.0% when pulling 7b5f380 on placeholder-replacement into d60431f on master.

nene · 2016-11-14T11:41:58Z

src/core/Tokenizer.js

@@ -208,6 +208,10 @@ export default class Tokenizer {
        return token;
    }

+    getEscapedPlaceholderKey({key, stringChar}) {


I think a better name would be: quoteChar

nene · 2016-11-14T11:43:47Z

README.md

+sqlFormatter.format("SELECT ? FROM table1", {
+    params: ["bar"]
+}));
+```


We should use a more real-life example. One normally uses placeholders in conditions of WHERE clause like:

SELECT * FROM tbl WHERE foo = ?

coveralls · 2016-11-14T11:56:46Z

Coverage remained the same at 100.0% when pulling 96b92af on placeholder-replacement into d60431f on master.

Uku Pattak added 3 commits November 2, 2016 17:57

Rename variable token type to placeholder

697a101

Implement placeholders replacement with configured params collection

2ada26b

I documented params cfg to be a object, but actually it can be either Array or Object.

Add $ named placeholder type to N1QL

861c22a

See http://developer.couchbase.com/documentation/server/current/sdk/n1ql-query.html

ukupat assigned nene Nov 2, 2016

nene requested changes Nov 7, 2016

View reviewed changes

Rename plain placeholder to ident placeholder in Tokenizer

b1992a0

Uku Pattak added 7 commits November 8, 2016 14:32

Cover weird variable names with tests

87dc06a

Reorganise variables tests based on their type in Standard SQL tests

f0d0db4

Use array as params cfg in indexed placeholder tests

54fd52f

Make params replacing more robust

fb9aae6

Rename sqlTokenTypes to tokenTypes

2304448

Clean up editor config

a47d3d5

Merge branch 'master' into placeholder-replacement

e606409

# Conflicts: # src/core/Formatter.js

nene reviewed Nov 10, 2016

View reviewed changes

Move placeholder key parsing to tokenizer and use ? only for indexed …

2d5c48f

…placeholders

Update README.md

9ac4d75

Make string placeholder escaping correctly work

2f76764

Add some extra weird placeholder replacement test example

7b5f380

nene requested changes Nov 14, 2016

View reviewed changes

Uku Pattak added 2 commits November 14, 2016 13:53

Update placeholders README to have more real-life examples

211e148

Replace stringChar variable with quoteChar variable in Tokenizer

96b92af

ukupat added the feature label Nov 14, 2016

nene approved these changes Nov 14, 2016

View reviewed changes

nene merged commit b4b16e7 into master Nov 14, 2016

ukupat deleted the placeholder-replacement branch November 14, 2016 12:23

nene pushed a commit that referenced this pull request Apr 27, 2022

Merge pull request #14 from inferrinizzard/feature/alias-position

e95e2d2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Placeholder replacement #14

Placeholder replacement #14

ukupat commented Nov 2, 2016

coveralls commented Nov 2, 2016

nene Nov 7, 2016

nene Nov 7, 2016

ukupat Nov 8, 2016

nene Nov 10, 2016 •

edited

nene Nov 7, 2016

nene Nov 7, 2016

nene Nov 7, 2016

coveralls commented Nov 8, 2016

ukupat commented Nov 8, 2016

coveralls commented Nov 8, 2016

nene Nov 10, 2016

nene Nov 10, 2016

ukupat Nov 14, 2016

nene Nov 10, 2016 •

edited

ukupat commented Nov 11, 2016

coveralls commented Nov 11, 2016

coveralls commented Nov 11, 2016

coveralls commented Nov 11, 2016

coveralls commented Nov 14, 2016

nene Nov 14, 2016

nene Nov 14, 2016

coveralls commented Nov 14, 2016

Placeholder replacement #14

Placeholder replacement #14

Conversation

ukupat commented Nov 2, 2016

coveralls commented Nov 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nene Nov 10, 2016 • 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

coveralls commented Nov 8, 2016

ukupat commented Nov 8, 2016

coveralls commented Nov 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nene Nov 10, 2016 • edited

Choose a reason for hiding this comment

ukupat commented Nov 11, 2016

coveralls commented Nov 11, 2016

coveralls commented Nov 11, 2016

coveralls commented Nov 11, 2016

coveralls commented Nov 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 14, 2016

nene Nov 10, 2016 •

edited

nene Nov 10, 2016 •

edited