You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@netbeans.apache.org by Eric Bresie <eb...@gmail.com> on 2021/05/15 16:59:55 UTC
SQL Autocompletion - Quoter Refactoring breaks quoter check
As part of NETBEANS-189 / https://github.com/apache/netbeans/pull/2820,
The basic PR I believe addresses the main concern of the ticket to allow
autocomplete when no connection is set yet. These changes are limited to
the SQL Editor project.
However, requested PR review change suggests refactoring and better
handling "connected" vs "unconnected" cases a little better rather than in
the lower level handling when connections aren't available.
So I'm trying to refactor the SqlIdentifier to account for connected and
nonconnected DB sessions for quoting purposes. This expands the scope of
the changes to include the "SQL Explorer" project. This seems to make it a
little more complicated than needed. Should this refactoring be captured
as a separate bug?
While trying to work something out for the refactoring...
I'm moving some methods out from DatabaseMetaDataQuoter into the
"SQLIdentifier.Quoter" to be shareable where applicable for use in
"non-connected" cases. And also brought in one of the unit test classes
"NonASCIIQuoter" in as wellwith hopes some of this is usable with/without a
connection context.
However, when running the "SQL99-Quoting check"
"testQuoteIdentifierContainingQuotingChar()", it fails on
assertEquals(quoted, quoter.quoteAlways(unquoted)); due to "quoted" = ""test
*""*xx"" vs quoteAlways(unquoted) = ""test*"*xx"" which don't match.
I think the root of this is the "quoteAlways" calling to alreadyQuoted()
which seems to check ends and no quotes within.
In this case there may be a "<table>.<column>" type scenario. Assume there
is a full identifier which in some context are broken up into
"sub-identifiers [tokens]" each of which may need to handled separately
(i.e. each token checked as opposed to the whole identifier being check)
but now it's handled as the "whole" identifier.
This test worked prior to all the refactoring so assume it's related to
some of the changes but I'm just wondering if this highlights a new bug
that maybe was not accounted for previously.
Should it account for "internal quotes" as well?
Hope this makes sense
Eric Bresie
ebresie@gmail.com
[NETBEANS-189] Re: SQL Autocompletion - Quoter Refactoring breaks
quoter check
Posted by Eric Bresie <eb...@gmail.com>.
I’ll try to summarize again…
Trying to fix NETBEANS-189 with PR 2820 which I believe currently fixes the
problem. I’ve tried to capture details from the discussion in the ticket.
Change in present fix involves adding checks for null when a connection is
not available and allows to progress without a connection, allowing basic
sql auto completion without a connection.
Review change request (which I tried to implement unsuccessfully) leads me
to believe this to be a potentially larger architectural change out of
scope of the original issue and should be addressed as separate ticket(s)
and not hold up inclusion of the fix.
If new issues are needed then I (or someone with more understanding) can
raise them.
I am looking to close out the review (with acceptance where applicable as
I’m newer “commuter”) and submit for next release.
Eric
On Sun, Jun 20, 2021 at 11:33 AM Geertjan Wielenga
<ge...@googlemail.com.invalid> wrote:
> Just judging from the long mails you’ve been sending on this topic, there
> are probably very few if any who are keeping track of this or understand
> what it’s about.
>
> My gut feeling is: if it is this complex and convoluted to introduce
> something as narrow and specific as code completion for SQL, there must be
> something fundamentally wrong somewhere.
>
> Maybe a way forward is to create one Wiki document where you describe step
> by step from scratch what it is you’re trying to do and the exact pieces of
> code you propose to introduce into the codebase for this, i.e., really
> clearly and coherently, where you’re able to explain everything that needs
> to be added for this feature, the broader picture of this can be clarified
> for more people so we can all know what is going on here.
>
> Your effort and hard work is appreciated, however, I predict another long
> convoluted mail again in response to this request, instead of simply: what
> is this really about, why is it so overcomplicated, can you put it into a
> Wiki where you explain everything, concisely, please?
>
> Gj
>
--
Eric Bresie
ebresie@gmail.com
Re: SQL Autocompletion - Quoter Refactoring breaks quoter check
Posted by Geertjan Wielenga <ge...@googlemail.com.INVALID>.
Just judging from the long mails you’ve been sending on this topic, there
are probably very few if any who are keeping track of this or understand
what it’s about.
My gut feeling is: if it is this complex and convoluted to introduce
something as narrow and specific as code completion for SQL, there must be
something fundamentally wrong somewhere.
Maybe a way forward is to create one Wiki document where you describe step
by step from scratch what it is you’re trying to do and the exact pieces of
code you propose to introduce into the codebase for this, i.e., really
clearly and coherently, where you’re able to explain everything that needs
to be added for this feature, the broader picture of this can be clarified
for more people so we can all know what is going on here.
Your effort and hard work is appreciated, however, I predict another long
convoluted mail again in response to this request, instead of simply: what
is this really about, why is it so overcomplicated, can you put it into a
Wiki where you explain everything, concisely, please?
Gj
On Sun, 20 Jun 2021 at 18:11, Eric Bresie <eb...@gmail.com> wrote:
> Regarding https://github.com/apache/netbeans/pull/2820
>
> Adding currently identified reviewers to "To"
>
> I realize everyone has other priorities and that the comments/discussions
> on the PR may be a little chaotic and confusing so in summary, I have:
> (1) Resolved issues adding logic to account for cases when connection is
> not available and allow sql autocomplete to work even without an active
> connection
> (2) This no longer shows the dialog asking for connection which prevented
> autocompletion previously, but still shows when attempting to "execute" a
> query where a connection is really needed (similar to how other sql editor
> type applications may do things [i.e. wait until such time as absolutely
> needed]
> (3) I removed any unnecessary changes (i.e. hints - created ticket
> NETBEANS-5474
> Add SQL Hints to address this),
> (4) I have squashed [to the best of my abilities] and reduced the changed
> files to only applicable 3 files in the Sql Editor project, and
> (5) I have adjusted the quoter logic to no longer return an empty string
> and have a minimum form of quoter/unquoter which still works in the unit
> tests for the SQL projects
> (6) I have not re-architected to introduce any kind of "Standard SQL
> Quoter" class at this time as I believe this is likely a larger effort
> which I propose raising a new ticket to create a Standard SQL Quoter class
> and retrofit the code as part of that work.
>
> *Assume this would involve developing a new class for such and
> handle standardized quoting of identifiers similar to what is discussed (
> https://www.w3resource.com/sql/sql-syntax.php#IDENTIF
> <https://www.w3resource.com/sql/sql-syntax.php#IDENTIF> ). *
> *For this I (or whoever takes this on) would need to better understand what
> the expected input/outputs are for this and what kind of tests would be
> needed to confirm this does as expected beyond the existing sql tests. *
>
> (7) There may also be another SQL improvements ticket to raise as mentioned
> in this comment in the discussion
>
> *Looking at one of the w3 resources SQL syntax page in identifier section
> <https://www.w3resource.com/sql/sql-syntax.php#IDENTIF> it does look more
> complicated.*
>
> *The w3 reference mentions after "SELECT" there can be optionally "DISTINCT
> | ALL" and either a wildcard ("*") or select List (i.e. identifiers).
> Unless I've missed it, I'm not sure these cases are handled. Is a new issue
> for adding that worth wild?*
>
>
> Does this seem acceptable? If not, what (besides maybe raising above
> tickets) is still needed?
>
> Eric Bresie
> ebresie@gmail.com
>
>
> On Fri, Jun 18, 2021 at 4:46 PM Eric Bresie <eb...@gmail.com> wrote:
>
> > Can anyone respond to this?
> >
> > As is, the PR from my perspective fixes the problem.
> >
> > The last change requested in the PR was made multiple times in multiple
> > ways with the request was against an “out of date” version of files so
> not
> > sure if the PR discussion is ready or able to be closed down.
> >
> > I believe re-architecting sql autocomplete functionality for connection
> vs
> > connection less session to address quoting and/or sql standard quotes
> issue
> > is probably out of scope of this ticket. If that is really preferred
> then
> > maybe a new ticket to address that with more specific would be better for
> > that.
> >
> > What do I need to do to get this accepted for inclusion?
> >
> > Eric
> >
> > On Sun, Jun 6, 2021 at 11:00 AM Eric Bresie <eb...@gmail.com> wrote:
> >
> >> Okay...I've concluded my connectionless refactoring is breaking more
> unit
> >> tests than preferred and is not a good viable option.
> >>
> >> I've reverted back to the state as of the last commit for the PR and
> >> would like some guidance from someone more well versed than I on how to
> >> handle this.
> >>
> >> Assume can either
> >> (1) take it as is in the PR and maybe write a new ticket on the refactor
> >> aspect (Refactor SQL quoter to handle connection or connection less
> cases)
> >> or
> >> (2) if someone more well verses in the sql code can provide suggestions
> >> for a new approach I would be welcome to.
> >>
> >> Some help/suggestion/mentoring is appreciated.
> >>
> >> Eric Bresie
> >> ebresie@gmail.com
> >>
> >>
> >> On Sat, May 15, 2021 at 11:59 AM Eric Bresie <eb...@gmail.com> wrote:
> >>
> >>> As part of NETBEANS-189 / https://github.com/apache/netbeans/pull/2820
> ,
> >>>
> >>> The basic PR I believe addresses the main concern of the ticket to
> allow
> >>> autocomplete when no connection is set yet. These changes are limited
> to
> >>> the SQL Editor project.
> >>>
> >>> However, requested PR review change suggests refactoring and better
> >>> handling "connected" vs "unconnected" cases a little better rather
> than in
> >>> the lower level handling when connections aren't available.
> >>>
> >>> So I'm trying to refactor the SqlIdentifier to account for connected
> and
> >>> nonconnected DB sessions for quoting purposes. This expands the scope
> of
> >>> the changes to include the "SQL Explorer" project. This seems to make
> it a
> >>> little more complicated than needed. Should this refactoring be
> captured
> >>> as a separate bug?
> >>>
> >>> While trying to work something out for the refactoring...
> >>>
> >>> I'm moving some methods out from DatabaseMetaDataQuoter into the
> >>> "SQLIdentifier.Quoter" to be shareable where applicable for use in
> >>> "non-connected" cases. And also brought in one of the unit test
> classes
> >>> "NonASCIIQuoter" in as wellwith hopes some of this is usable
> with/without a
> >>> connection context.
> >>>
> >>> However, when running the "SQL99-Quoting check"
> >>> "testQuoteIdentifierContainingQuotingChar()", it fails on
> >>> assertEquals(quoted, quoter.quoteAlways(unquoted)); due to "quoted" =
> ""test
> >>> *""*xx"" vs quoteAlways(unquoted) = ""test*"*xx"" which don't match.
> >>>
> >>> I think the root of this is the "quoteAlways" calling to
> alreadyQuoted()
> >>> which seems to check ends and no quotes within.
> >>>
> >>> In this case there may be a "<table>.<column>" type scenario. Assume
> >>> there is a full identifier which in some context are broken up into
> >>> "sub-identifiers [tokens]" each of which may need to handled separately
> >>> (i.e. each token checked as opposed to the whole identifier being
> check)
> >>> but now it's handled as the "whole" identifier.
> >>>
> >>> This test worked prior to all the refactoring so assume it's related to
> >>> some of the changes but I'm just wondering if this highlights a new bug
> >>> that maybe was not accounted for previously.
> >>>
> >>> Should it account for "internal quotes" as well?
> >>>
> >>> Hope this makes sense
> >>>
> >>> Eric Bresie
> >>> ebresie@gmail.com
> >>>
> >> --
> > Eric Bresie
> > ebresie@gmail.com
> >
>
Re: SQL Autocompletion - Quoter Refactoring breaks quoter check
Posted by Eric Bresie <eb...@gmail.com>.
Regarding https://github.com/apache/netbeans/pull/2820
Adding currently identified reviewers to "To"
I realize everyone has other priorities and that the comments/discussions
on the PR may be a little chaotic and confusing so in summary, I have:
(1) Resolved issues adding logic to account for cases when connection is
not available and allow sql autocomplete to work even without an active
connection
(2) This no longer shows the dialog asking for connection which prevented
autocompletion previously, but still shows when attempting to "execute" a
query where a connection is really needed (similar to how other sql editor
type applications may do things [i.e. wait until such time as absolutely
needed]
(3) I removed any unnecessary changes (i.e. hints - created ticket
NETBEANS-5474
Add SQL Hints to address this),
(4) I have squashed [to the best of my abilities] and reduced the changed
files to only applicable 3 files in the Sql Editor project, and
(5) I have adjusted the quoter logic to no longer return an empty string
and have a minimum form of quoter/unquoter which still works in the unit
tests for the SQL projects
(6) I have not re-architected to introduce any kind of "Standard SQL
Quoter" class at this time as I believe this is likely a larger effort
which I propose raising a new ticket to create a Standard SQL Quoter class
and retrofit the code as part of that work.
*Assume this would involve developing a new class for such and
handle standardized quoting of identifiers similar to what is discussed (
https://www.w3resource.com/sql/sql-syntax.php#IDENTIF
<https://www.w3resource.com/sql/sql-syntax.php#IDENTIF> ). *
*For this I (or whoever takes this on) would need to better understand what
the expected input/outputs are for this and what kind of tests would be
needed to confirm this does as expected beyond the existing sql tests. *
(7) There may also be another SQL improvements ticket to raise as mentioned
in this comment in the discussion
*Looking at one of the w3 resources SQL syntax page in identifier section
<https://www.w3resource.com/sql/sql-syntax.php#IDENTIF> it does look more
complicated.*
*The w3 reference mentions after "SELECT" there can be optionally "DISTINCT
| ALL" and either a wildcard ("*") or select List (i.e. identifiers).
Unless I've missed it, I'm not sure these cases are handled. Is a new issue
for adding that worth wild?*
Does this seem acceptable? If not, what (besides maybe raising above
tickets) is still needed?
Eric Bresie
ebresie@gmail.com
On Fri, Jun 18, 2021 at 4:46 PM Eric Bresie <eb...@gmail.com> wrote:
> Can anyone respond to this?
>
> As is, the PR from my perspective fixes the problem.
>
> The last change requested in the PR was made multiple times in multiple
> ways with the request was against an “out of date” version of files so not
> sure if the PR discussion is ready or able to be closed down.
>
> I believe re-architecting sql autocomplete functionality for connection vs
> connection less session to address quoting and/or sql standard quotes issue
> is probably out of scope of this ticket. If that is really preferred then
> maybe a new ticket to address that with more specific would be better for
> that.
>
> What do I need to do to get this accepted for inclusion?
>
> Eric
>
> On Sun, Jun 6, 2021 at 11:00 AM Eric Bresie <eb...@gmail.com> wrote:
>
>> Okay...I've concluded my connectionless refactoring is breaking more unit
>> tests than preferred and is not a good viable option.
>>
>> I've reverted back to the state as of the last commit for the PR and
>> would like some guidance from someone more well versed than I on how to
>> handle this.
>>
>> Assume can either
>> (1) take it as is in the PR and maybe write a new ticket on the refactor
>> aspect (Refactor SQL quoter to handle connection or connection less cases)
>> or
>> (2) if someone more well verses in the sql code can provide suggestions
>> for a new approach I would be welcome to.
>>
>> Some help/suggestion/mentoring is appreciated.
>>
>> Eric Bresie
>> ebresie@gmail.com
>>
>>
>> On Sat, May 15, 2021 at 11:59 AM Eric Bresie <eb...@gmail.com> wrote:
>>
>>> As part of NETBEANS-189 / https://github.com/apache/netbeans/pull/2820,
>>>
>>> The basic PR I believe addresses the main concern of the ticket to allow
>>> autocomplete when no connection is set yet. These changes are limited to
>>> the SQL Editor project.
>>>
>>> However, requested PR review change suggests refactoring and better
>>> handling "connected" vs "unconnected" cases a little better rather than in
>>> the lower level handling when connections aren't available.
>>>
>>> So I'm trying to refactor the SqlIdentifier to account for connected and
>>> nonconnected DB sessions for quoting purposes. This expands the scope of
>>> the changes to include the "SQL Explorer" project. This seems to make it a
>>> little more complicated than needed. Should this refactoring be captured
>>> as a separate bug?
>>>
>>> While trying to work something out for the refactoring...
>>>
>>> I'm moving some methods out from DatabaseMetaDataQuoter into the
>>> "SQLIdentifier.Quoter" to be shareable where applicable for use in
>>> "non-connected" cases. And also brought in one of the unit test classes
>>> "NonASCIIQuoter" in as wellwith hopes some of this is usable with/without a
>>> connection context.
>>>
>>> However, when running the "SQL99-Quoting check"
>>> "testQuoteIdentifierContainingQuotingChar()", it fails on
>>> assertEquals(quoted, quoter.quoteAlways(unquoted)); due to "quoted" = ""test
>>> *""*xx"" vs quoteAlways(unquoted) = ""test*"*xx"" which don't match.
>>>
>>> I think the root of this is the "quoteAlways" calling to alreadyQuoted()
>>> which seems to check ends and no quotes within.
>>>
>>> In this case there may be a "<table>.<column>" type scenario. Assume
>>> there is a full identifier which in some context are broken up into
>>> "sub-identifiers [tokens]" each of which may need to handled separately
>>> (i.e. each token checked as opposed to the whole identifier being check)
>>> but now it's handled as the "whole" identifier.
>>>
>>> This test worked prior to all the refactoring so assume it's related to
>>> some of the changes but I'm just wondering if this highlights a new bug
>>> that maybe was not accounted for previously.
>>>
>>> Should it account for "internal quotes" as well?
>>>
>>> Hope this makes sense
>>>
>>> Eric Bresie
>>> ebresie@gmail.com
>>>
>> --
> Eric Bresie
> ebresie@gmail.com
>
Re: SQL Autocompletion - Quoter Refactoring breaks quoter check
Posted by Eric Bresie <eb...@gmail.com>.
Can anyone respond to this?
As is, the PR from my perspective fixes the problem.
The last change requested in the PR was made multiple times in multiple
ways with the request was against an “out of date” version of files so not
sure if the PR discussion is ready or able to be closed down.
I believe re-architecting sql autocomplete functionality for connection vs
connection less session to address quoting and/or sql standard quotes issue
is probably out of scope of this ticket. If that is really preferred then
maybe a new ticket to address that with more specific would be better for
that.
What do I need to do to get this accepted for inclusion?
Eric
On Sun, Jun 6, 2021 at 11:00 AM Eric Bresie <eb...@gmail.com> wrote:
> Okay...I've concluded my connectionless refactoring is breaking more unit
> tests than preferred and is not a good viable option.
>
> I've reverted back to the state as of the last commit for the PR and would
> like some guidance from someone more well versed than I on how to handle
> this.
>
> Assume can either
> (1) take it as is in the PR and maybe write a new ticket on the refactor
> aspect (Refactor SQL quoter to handle connection or connection less cases)
> or
> (2) if someone more well verses in the sql code can provide suggestions
> for a new approach I would be welcome to.
>
> Some help/suggestion/mentoring is appreciated.
>
> Eric Bresie
> ebresie@gmail.com
>
>
> On Sat, May 15, 2021 at 11:59 AM Eric Bresie <eb...@gmail.com> wrote:
>
>> As part of NETBEANS-189 / https://github.com/apache/netbeans/pull/2820,
>>
>> The basic PR I believe addresses the main concern of the ticket to allow
>> autocomplete when no connection is set yet. These changes are limited to
>> the SQL Editor project.
>>
>> However, requested PR review change suggests refactoring and better
>> handling "connected" vs "unconnected" cases a little better rather than in
>> the lower level handling when connections aren't available.
>>
>> So I'm trying to refactor the SqlIdentifier to account for connected and
>> nonconnected DB sessions for quoting purposes. This expands the scope of
>> the changes to include the "SQL Explorer" project. This seems to make it a
>> little more complicated than needed. Should this refactoring be captured
>> as a separate bug?
>>
>> While trying to work something out for the refactoring...
>>
>> I'm moving some methods out from DatabaseMetaDataQuoter into the
>> "SQLIdentifier.Quoter" to be shareable where applicable for use in
>> "non-connected" cases. And also brought in one of the unit test classes
>> "NonASCIIQuoter" in as wellwith hopes some of this is usable with/without a
>> connection context.
>>
>> However, when running the "SQL99-Quoting check"
>> "testQuoteIdentifierContainingQuotingChar()", it fails on
>> assertEquals(quoted, quoter.quoteAlways(unquoted)); due to "quoted" = ""test
>> *""*xx"" vs quoteAlways(unquoted) = ""test*"*xx"" which don't match.
>>
>> I think the root of this is the "quoteAlways" calling to alreadyQuoted()
>> which seems to check ends and no quotes within.
>>
>> In this case there may be a "<table>.<column>" type scenario. Assume
>> there is a full identifier which in some context are broken up into
>> "sub-identifiers [tokens]" each of which may need to handled separately
>> (i.e. each token checked as opposed to the whole identifier being check)
>> but now it's handled as the "whole" identifier.
>>
>> This test worked prior to all the refactoring so assume it's related to
>> some of the changes but I'm just wondering if this highlights a new bug
>> that maybe was not accounted for previously.
>>
>> Should it account for "internal quotes" as well?
>>
>> Hope this makes sense
>>
>> Eric Bresie
>> ebresie@gmail.com
>>
> --
Eric Bresie
ebresie@gmail.com
Re: SQL Autocompletion - Quoter Refactoring breaks quoter check
Posted by Eric Bresie <eb...@gmail.com>.
Okay...I've concluded my connectionless refactoring is breaking more unit
tests than preferred and is not a good viable option.
I've reverted back to the state as of the last commit for the PR and would
like some guidance from someone more well versed than I on how to handle
this.
Assume can either
(1) take it as is in the PR and maybe write a new ticket on the refactor
aspect (Refactor SQL quoter to handle connection or connection less cases)
or
(2) if someone more well verses in the sql code can provide suggestions for
a new approach I would be welcome to.
Some help/suggestion/mentoring is appreciated.
Eric Bresie
ebresie@gmail.com
On Sat, May 15, 2021 at 11:59 AM Eric Bresie <eb...@gmail.com> wrote:
> As part of NETBEANS-189 / https://github.com/apache/netbeans/pull/2820,
>
> The basic PR I believe addresses the main concern of the ticket to allow
> autocomplete when no connection is set yet. These changes are limited to
> the SQL Editor project.
>
> However, requested PR review change suggests refactoring and better
> handling "connected" vs "unconnected" cases a little better rather than in
> the lower level handling when connections aren't available.
>
> So I'm trying to refactor the SqlIdentifier to account for connected and
> nonconnected DB sessions for quoting purposes. This expands the scope of
> the changes to include the "SQL Explorer" project. This seems to make it a
> little more complicated than needed. Should this refactoring be captured
> as a separate bug?
>
> While trying to work something out for the refactoring...
>
> I'm moving some methods out from DatabaseMetaDataQuoter into the
> "SQLIdentifier.Quoter" to be shareable where applicable for use in
> "non-connected" cases. And also brought in one of the unit test classes
> "NonASCIIQuoter" in as wellwith hopes some of this is usable with/without a
> connection context.
>
> However, when running the "SQL99-Quoting check"
> "testQuoteIdentifierContainingQuotingChar()", it fails on
> assertEquals(quoted, quoter.quoteAlways(unquoted)); due to "quoted" = ""test
> *""*xx"" vs quoteAlways(unquoted) = ""test*"*xx"" which don't match.
>
> I think the root of this is the "quoteAlways" calling to alreadyQuoted()
> which seems to check ends and no quotes within.
>
> In this case there may be a "<table>.<column>" type scenario. Assume
> there is a full identifier which in some context are broken up into
> "sub-identifiers [tokens]" each of which may need to handled separately
> (i.e. each token checked as opposed to the whole identifier being check)
> but now it's handled as the "whole" identifier.
>
> This test worked prior to all the refactoring so assume it's related to
> some of the changes but I'm just wondering if this highlights a new bug
> that maybe was not accounted for previously.
>
> Should it account for "internal quotes" as well?
>
> Hope this makes sense
>
> Eric Bresie
> ebresie@gmail.com
>