ebresie commented on a change in pull request #2820:
URL: https://github.com/apache/netbeans/pull/2820#discussion_r615002468
##########
File path:
ide/db.sql.editor/src/org/netbeans/modules/db/sql/analyzer/SQLStatementAnalyzer.java
##########
@@ -239,6 +239,8 @@ protected QualIdent parseIdentifier() {
}
protected String getUnquotedIdentifier() {
+ if (quoter == null)
+ return "";
Review comment:
Matthias, I appreciate the feedback. As a new commiter, I know I;m not
there yet but will get there with help.
The original scope of this ticket was...if no connection, allow some form of
auto completion without connection based details and avoid the connection
dialog from preventing futher autocompletion. As it is now I think it does
this.
The remaining concenrs is how getUnquotedIdentifier behaves when no
connection is available. Initial changes was to returned "", which I realize
is wrong, risking the loss of given seq.token.txt details. Next change was to
return the results without being "unquoted" with given "return
seq.token().text().toString();"; whether it needed or not was dependent on the
database in use which if not available is where problems may occur. When
quoter is not available or used avoids risk of a null pointers and return the
sequence of text in the given context without being concerned without being
"unquoted" independent of whether a "quoter" was available or not.
When quoter is available and calls the "unquote" method found in the
SQLIdentifer.Quoter abstrace class which is implemented/extended in the
DatabaseMetaDataQuoter implementation; if quoter was set correctly previously
(when DB is available) then instance of quoter should be available and all is
right with the world. However, when quoter is unavailable (not connection and
not setup earlier) then something different is needed.
So is the change suggetion to implement a new ConnectionLessQuoter or
BasicQuoter under the SqlIdentifer class and use this when connection is not
available earlier in the flow?
It seems many of the methods in the Quoter abstract class probably should be
static methods. So is the refactoring mentioned involve moving some of these
out into a "static" context in some way?
As implemented, "getUnquotedIdentifer" is used in placed like
"InsertStatementAnalyzer, SQLStatemenAnalyzer, and SelectStatementAnalyzer,
analyzeChosenColumns, etc. In other places, some form of interface is used
like "TablesClause" (assume this would be "connection based") used in places
such as "DeleteStatementAnalyzer", InsertStatementAnalyzer,
SelectStatementAnalyzer, and UpdateStatementAnalyzer". Or uses a call to
"parseIdentifier" which also use the getUnquotedIdentifer or parseTableIdent to
return table names "Identifiers". So are changes to standardize these across
context needed here?
For SqlStatementAnalyizer, the "connection" state is not know directly, but
is available previously such as in the SQLCompletionQuery context where it
established the quoter and passes forward to applicable as. So in a given
Analyzer context, the "quoter" was the way to determine if there was or wasn't
a connection without passing around the connections from other scope/context.
Do this type of aspect reflect the "connection vs non-connection" based
changes preferred in earlier comment?
Connection may be needed when determining what kind of identifiers to
include in the autocomplete options in applicable context of a given sql
statement (i.e. between select xxx from, insert .. INTO xxxx, etc.) but not
others (i.e. empty line so use initial SQL keywords). WHen available then the
identifier such as table and columns identifiers based on the connection are
possible which I bleive depneds on connected based DatabaseMetaDataQuoter and
related classed. This may help determine if should or shouldn't quote given
identifiers (database/schema/table/column) for given DB product. If connection
is not available, then maybe able to add "non connection based identifier
items" (i.e. "*", Count(*), DISTINCT, etc. - not sure it does this presently;
is this where a StandardSQLQuoter might come in to lay?).
In each call to "getUnquotedIdentifer()" it return a list Strings with
possible items. It iterates through each token in the current expression and
may key on before/after "dot" or if typing an identifier make another call. In
other cases it may use the above mentioned "createTable" type style so
Can't really create a DB quoter without a give connection; and no easy way
to determine what connection to use without user interaction (i.e. selecting DB
connection if available, creating a connection, etc.). Some sort of "default"
connection (probably set in the connection pulldown) could be persisted in some
way and reused after it was first established to reduce the chance it's not
setup, but that also seems like a different type of change (i.e. maybe a new
"store selected connection and reused next time file or SQL editor is opened"
type ticket).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
