`CURRENT_USER`, 'USER `and` SESSION_USER` system functions #5807

andreidan · 2017-06-26T12:36:03Z

No description provided.

m-kharbat · 2017-06-26T12:46:37Z

sql-parser/src/main/antlr/SqlBase.g4

@@ -232,6 +232,9 @@ primaryExpression
    | name=CURRENT_TIME ('(' precision=integerLiteral')')?                           #specialDateTimeFunction
    | name=CURRENT_TIMESTAMP ('(' precision=integerLiteral')')?                      #specialDateTimeFunction
    | CURRENT_SCHEMA ('(' ')')?                                                      #currentSchema
+    | CURRENT_USER                                                                   #currentUser
+    | USER                                                                           #user


if USER is always returning CURRENT_USER they can be merged in the same statement | (CURRENT_USER | USER) #currentUser and visitUser won't be needed

seut · 2017-06-26T13:40:32Z

blackbox/docs/sql/scalar.txt

+``USER``
+--------
+
+Equivalent to ``CURRENT_USER``.


pls make CURRENT_USER a reference(link) (use `CURRENT_USER`_)

seut · 2017-06-26T13:40:46Z

blackbox/docs/sql/scalar.txt

+
+``SESSION_USER``
+----------------
+The ``SESSION_USER`` system information function returns the name of the


missing newline

why is the description here explicit (but a copy of CURRENT_USER) instead of the same description of USER?

Because SESSION_USER is not the same as CURRENT_USER. See the difference in result column name.

seut · 2017-06-26T13:42:19Z

blackbox/docs/sql/scalar.txt

+
+.. note::
+
+    The ``CURRENT_USER``, ``SESSION_USER`` and ``USER`` functions have a  


pls make all 3 links

seut · 2017-06-26T13:42:38Z

blackbox/docs/sql/scalar.txt

@@ -1647,4 +1719,7 @@ Example::
 .. _Java Regular Expressions: http://docs.oracle.com/javase/8/docs/api/java/util/regex/Pattern.html
 .. _`MySQL date_format`: http://dev.mysql.com/doc/refman/5.6/en/date-and-time-functions.html#function_date-format
 .. _`Haversine formula`: https://en.wikipedia.org/wiki/Haversine_formula
+


pls remove all these new lines

seut · 2017-06-26T13:44:16Z

sql-parser/src/main/antlr/SqlBase.g4

@@ -232,6 +232,8 @@ primaryExpression
    | name=CURRENT_TIME ('(' precision=integerLiteral')')?                           #specialDateTimeFunction
    | name=CURRENT_TIMESTAMP ('(' precision=integerLiteral')')?                      #specialDateTimeFunction
    | CURRENT_SCHEMA ('(' ')')?                                                      #currentSchema
+    | (CURRENT_USER | USER)                                                          #currentUser
+    | SESSION_USER ('(' ')')?                                                        #sessionUser


the documentation does not mention optional parenthesis

seut · 2017-06-26T13:51:46Z

sql/src/main/java/io/crate/operation/scalar/systeminformation/SessionUserFunction.java

+import javax.annotation.Nullable;
+
+
+public class SessionUserFunction extends Scalar<BytesRef, Object> implements FunctionFormatSpec {


this class is a complete copy of CurrentUserFunction. pls solve by 1 implementation which is registered under 2 different names

seut · 2017-06-26T13:52:47Z

sql/src/main/java/io/crate/operation/scalar/systeminformation/UserFunctionsNormalizer.java

+    }
+
+    static Symbol normalizeUsernameFor(String functionName, @Nullable TransactionContext transactionContext) {
+        if (transactionContext == null) {


move inside your function after you have refactored it to 1 implementation

seut · 2017-06-26T13:53:29Z

sql/src/test/java/io/crate/analyze/CreateAlterTableStatementAnalyzerTest.java

@@ -350,12 +350,12 @@ public void testCreateTableWithAnalyzerParameter() throws Exception {
    public void textCreateTableWithCustomAnalyzerInNestedColumn() throws Exception {
        CreateTableAnalyzedStatement analysis = e.analyze(
            "create table ft_search (" +
-            "user object (strict) as (" +
+            "auser object (strict) as (" +


why not just quote the user? would make it much more readable

I thought " "user" object" is actually less readable than "auser object"

seut · 2017-06-26T13:54:45Z

sql/src/test/java/io/crate/integrationtests/ShowIntegrationTest.java

@@ -215,24 +215,24 @@ public void testShowCreateTableWithGeneratedColumn() throws Exception {
                " col1 AS ts + 1," +
                " col2 string GENERATED ALWAYS AS ts + 1," +
                " col3 string GENERATED ALWAYS AS (ts + 1)," +
-                " name AS concat(user['name'], 'foo')," +


just quote the user and nothing else needs to be changed...

seut · 2017-06-26T13:58:02Z

sql/src/test/java/io/crate/operation/scalar/systeminformation/UserFunctionsNormalizerTest.java

+import static io.crate.testing.SymbolMatchers.isLiteral;
+import static org.hamcrest.core.Is.is;
+
+public class UserFunctionsNormalizerTest extends CrateUnitTest {


pls move all tests to UserFunctionsTest after your refactoring

seut

lgtm otherwise

seut · 2017-06-28T10:36:29Z

sql/src/main/java/io/crate/operation/scalar/systeminformation/UserFunction.java

+        this.functionInfo = new FunctionInfo(new FunctionIdent(name, ImmutableList.of()), DataTypes.STRING);
+    }
+
+    public static void register(ScalarFunctionModule scalarFunctionModule) {


pls define before all instance vars and methods

mfussenegger · 2017-06-28T11:47:12Z

sql/src/test/java/io/crate/operation/scalar/systeminformation/UserFunctionsTest.java

+    public void testNormalizeUser() {
+        assertNormalize("user", isLiteral("testUser"), false);
+    }
+}


please also test the case where the user isn't set on the session

mfussenegger

please also test the case where the user isn't set on the session

seut · 2017-06-28T13:59:13Z

sql/src/main/java/io/crate/operation/scalar/systeminformation/UserFunction.java

-            username = user.name();
+        if (user == null) {
+            throw new UnsupportedFeatureException(
+                String.format(Locale.ENGLISH, "%s function is only supported in enterprise version", name)


hm but it also can be null if the user management is disabled...

codecov-io · 2017-06-28T14:06:04Z

Codecov Report

❗ No coverage uploaded for pull request base (master@31fba78). Click here to learn what that means.
The diff coverage is 82.35%.

@@           Coverage Diff            @@
##             master   #5807   +/-   ##
========================================
  Coverage          ?   83.1%           
  Complexity        ?   12201           
========================================
  Files             ?    1428           
  Lines             ?   46642           
  Branches          ?    5754           
========================================
  Hits              ?   38764           
  Misses            ?    5209           
  Partials          ?    2669

Impacted Files	Coverage Δ	Complexity Δ
...o/crate/operation/scalar/ScalarFunctionModule.java	`100% <ø> (ø)`	`6 <0> (?)`
.../src/main/java/io/crate/sql/parser/AstBuilder.java	`94.11% <100%> (ø)`	`243 <2> (?)`
...ava/io/crate/scalar/UsersScalarFunctionModule.java	`100% <100%> (ø)`	`2 <2> (?)`
...ers/src/main/java/io/crate/plugin/UsersPlugin.java	`100% <100%> (ø)`	`7 <3> (?)`
...o/crate/scalar/systeminformation/UserFunction.java	`68.42% <68.42%> (ø)`	`8 <8> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 31fba78...09f6240. Read the comment docs.

mfussenegger

Can't we change it to always just return null if the user isn't available in the session and get rid of these enterprise checks?

andreidan · 2017-06-28T17:04:49Z

@mfussenegger Discussed with @dobe to throw UnsupportedFeatureException as it's an enterprise feature (to be consistent with CREATE/DROP USER)

andreidan force-pushed the andrei/userFunctions branch from b90bd01 to 43d1e4b Compare June 26, 2017 12:37

m-kharbat reviewed Jun 26, 2017

View reviewed changes

andreidan force-pushed the andrei/userFunctions branch 2 times, most recently from 6edf4e7 to abb059a Compare June 26, 2017 13:15

andreidan requested a review from seut June 26, 2017 13:18

seut requested changes Jun 26, 2017

View reviewed changes

seut approved these changes Jun 28, 2017

View reviewed changes

andreidan force-pushed the andrei/userFunctions branch 2 times, most recently from 6591525 to d70c201 Compare June 28, 2017 11:38

mfussenegger reviewed Jun 28, 2017

View reviewed changes

mfussenegger requested changes Jun 28, 2017

View reviewed changes

mfussenegger approved these changes Jun 28, 2017

View reviewed changes

andreidan force-pushed the andrei/userFunctions branch from 888b433 to 1a87de3 Compare June 28, 2017 11:55

seut requested changes Jun 28, 2017

View reviewed changes

mfussenegger requested changes Jun 28, 2017

View reviewed changes

seut approved these changes Jun 29, 2017

View reviewed changes

mfussenegger approved these changes Jun 29, 2017

View reviewed changes

andreidan force-pushed the andrei/userFunctions branch 4 times, most recently from f8de716 to 3ff4b32 Compare July 5, 2017 11:32

mfussenegger approved these changes Jul 5, 2017

View reviewed changes

seut approved these changes Jul 5, 2017

View reviewed changes

andreidan force-pushed the andrei/userFunctions branch from 8da56c9 to 3c77f49 Compare July 5, 2017 12:12

Added CURRENT_USER, 'USER and SESSION_USER` system functions.

09f6240

andreidan force-pushed the andrei/userFunctions branch from 3c77f49 to 09f6240 Compare July 5, 2017 12:29

andreidan merged commit 09f6240 into master Jul 5, 2017

andreidan deleted the andrei/userFunctions branch July 5, 2017 12:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`CURRENT_USER`, 'USER `and` SESSION_USER` system functions #5807

`CURRENT_USER`, 'USER `and` SESSION_USER` system functions #5807

andreidan commented Jun 26, 2017

m-kharbat Jun 26, 2017

seut Jun 26, 2017

seut Jun 26, 2017

seut Jun 26, 2017

andreidan Jun 27, 2017

seut Jun 26, 2017

seut Jun 26, 2017

seut Jun 26, 2017

seut Jun 26, 2017

seut Jun 26, 2017

seut Jun 26, 2017

andreidan Jun 27, 2017 •

edited

seut Jun 26, 2017

seut Jun 26, 2017

seut left a comment

seut Jun 28, 2017

mfussenegger Jun 28, 2017

mfussenegger left a comment

seut Jun 28, 2017

codecov-io commented Jun 28, 2017 •

edited

mfussenegger left a comment

andreidan commented Jun 28, 2017


		.. note::

		The ``CURRENT_USER``, ``SESSION_USER`` and ``USER`` functions have a

		import javax.annotation.Nullable;


		public class SessionUserFunction extends Scalar<BytesRef, Object> implements FunctionFormatSpec {

CURRENT_USER, 'USER and SESSION_USER` system functions #5807

CURRENT_USER, 'USER and SESSION_USER` system functions #5807

Conversation

andreidan commented Jun 26, 2017

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

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

Choose a reason for hiding this comment

andreidan Jun 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seut left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfussenegger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jun 28, 2017 • edited

Codecov Report

mfussenegger left a comment

Choose a reason for hiding this comment

andreidan commented Jun 28, 2017

`CURRENT_USER`, 'USER `and` SESSION_USER` system functions #5807

`CURRENT_USER`, 'USER `and` SESSION_USER` system functions #5807

andreidan Jun 27, 2017 •

edited

codecov-io commented Jun 28, 2017 •

edited