Bug 115075

Currently, we quote *every* identifier, which is a usability issue, as SQL text is very ugly and confusing when all the
identifiers are quoted.  With this API in place, we can quote only those identifiers that are necessary.

Proposed stability: Friend

Impact: No impact, as this is new.  This API will be used by visualweb sql generation code, the sql editor and the
visual query editor to control quoting of identifiers.

=== Specification ====

/**
 * This class provides some utilities for working with SQL identifiers
 */
public final class SQLIdentifier {
    /**
     * Construct an instance of SQLIdentifier.  
     * 
     * @param dbmd The DatabaseMetaData to use when working with identifiers.
     *   The metadata object is used to determine when an identifier needs
     *   to be quoted and what the quote string should be.
     */
    public SQLIdentifier(DatabaseMetaData dbmd)
    
    /**
     * Quote an identifier to be used in a SQL command, if needed.
     * This is determined based on whether the characters in the
     * identifier string are within the set of characters that do
     * not need to be quoted.  Anyone generating SQL that will be
     * visible and/or editable by the user should use this method.
     * This helps to avoid unecessary quoting, which affects the
     * readability and clarity of the resulting SQL
     * 
     * @param identifier  The identifier to be quoted, if needed
     * 
     * @return The identifier, quoted if needed
     */
    public String quoteIfNeeded(String identifier)
}

AB01 I need to see the whole diff of the affected module.

AB02 Not sure which module this will go into. It can't be a friend API if it goes to the db module, since that already
exports a public API.

AB03 I don't see the reason for this class to be instantiable. Perhaps it would be visible from the diff (e.g., you need
to cache stuff from DMD, etc.). Better to expose a factory method instead of a public constructor. Best to expose no
constructor or factory method and have just a static quoteIfNeeded().

AB04 (nitpick) "@return The identifier" should be "@return the identifier".

> AB01 I need to see the whole diff of the affected module.

OK, I'll attach the diff.  The instructions on the NetBeans site said make the Javadoc available, not the whole diff.

> AB02 Not sure which module this will go into. It can't be a friend API if it goes to the db module, since that already
> exports a public API.

It seems to me it belongs in the db module.  It doesn't make sense to me to *not* put it there just because it's a new
method and a new class.  Your advice on how to best use the NB API process to do this would be most appreciated.

> AB03 I don't see the reason for this class to be instantiable. Perhaps it would be visible from the diff (e.g., you 
> need to cache stuff from DMD, etc.). Better to expose a factory method instead of a public constructor. 
> Best to expose no constructor or factory method and have just a static quoteIfNeeded().

I tried it as just a static method, but as you mentioned, I need to cache DMD information, otherwise I'll be
unnecessarily hitting the database every time quoteIfNeeded() is called, which can happen a lot.  I'll change it to a
factory method prior to posting the diff.

> AB04 (nitpick) "@return The identifier" should be "@return the identifier".

OK

Created attachment 48518 [details]
Diff for change, including test files

Forgot to fix AB04 in the attached diff, but I have changed it in my sandbox...

Y01 Test file is missing license header. Btw. I am glad the test is there.

Y02 From the name SQLIdentifier I would expect the class to represent an instance of some SQL identifier. This is not 
true - it contains an utility method to convert string to quoted string. There are two ways to fix this:
a) move the quiteIfNeeded method directly to DatabaseMetaData
b) rename the class. For example to DatabaseUtilities, etc.

Y03 Discoverability: There is no link from DatabaseMetaData to the utilities class, afaik. As such it is going to be 
pretty hard for people holding instance of DatabaseMetaData to find out that there is such utility class. If Y02b is 
implemented and not Y02a, would not it be better to have DatabaseMetaData.getDatabaseUtilities() method?

> Y02: ... SQLIdentifier (little confusing)...a) move the quiteIfNeeded method directly to DatabaseMetaData

My thoughts exactly. I mean, you already require DatabaseMetaData, and it is a method to work against the meta data, so
it logically should go there. I don't see why you need to have a factory method for this type method. It makes much more
sense to use the data you already have as it is logically connected. It is a simple action against the concepts and data
already in the class, and it is not a new concept you are adding to the API, so to me it doesn't warrant a new class.

AB05 Re Y02: DatabaseMetaData is a JDK class, so we can't modify it. But I don't like the SQLIdentifier name either. In
the future I would like to add a method like isSQL99Keyword() method to it (returning true if a given identifier is a
keyword according to the SQL-99 standard). But this method does not need a DMD instance to compute its result, so it can
be static. But then you have both static and instance methods in the class, which is ugly. I also don't like
DatabaseUtilities much, it is too generic. So I suggest

class SQLIdentifiers { // note the plural

    static Quoter createQuoter(DMD dmd);

    static boolean isSQL99Keyword(String identifier); // in a later API version

    static class Quoter {

        String quoteIfNeeded(String identifier);
    }
}

AB06 The method seems to quote an already quoted identifier (e.g., when passed "foo" it will return ""foo""). So it
seems to expect unquoted identifiers (not surrounded by quotes, to be precise). This needs to be mentioned in the Javadoc.

AB07 the current SQLIdentifier class is not thread-safe. This needs to be mentioned in the Javadoc. Or probably better
to make it thread-safe.

AB08 Please make the dbmd field final, regardless of AB07.

AB09 Perhaps should log warnings about failed DMD method calls with a WARNING level rather than INFO.

AB10 I would discourage allowing null as a valid parameter to quoteIfNeeded(). I would rather throw a
NullPointerException in that case. You can use org.openide.util.Parameters.notNull() for that. You should also make this
check as the very first thing in the method body (before computing nonQuoteChars).

AB11 The comments in the containsLowerCase() and containsUpperCase() methods do not make sense.

AB12 Method containsLowerCase() can be simplified to test if (ch >= 'a' || ch <= 'z') and you get can rid of the
UC_CHARS and LC_CHARS constants. Or you perhaps can refactor both methods to a single containsRange(char start, char
end, String str). 

AB13 The code does not seem to deal with getIdentifierQuoteString() returning a space, which its Javadoc says it can.

AB14 The value of the NONQUOTE_CHARS field is missing "7".

AB15 Is it really necessary to have all those escaped characters in testNonAscii()? A single character should suffice
for the test.

Whoops, the test in AB12 should use &&, not ||.

AB16 The note about not quoting new identifiers in the Javadoc of quoteIfNeeded() is irrelevant for this method.

Andrei, excellent, excellent review, as always.   You are making me a better Java programmer, I really appreciate it.

AB05 Re Y02: DatabaseMetaData is a JDK class, so we can't modify it. But I don't like the SQLIdentifier name either. In
the future I would like to add a method like isSQL99Keyword() method to it (returning true if a given identifier is a
keyword according to the SQL-99 standard). But this method does not need a DMD instance to compute its result, so it can
be static. But then you have both static and instance methods in the class, which is ugly. I also don't like
DatabaseUtilities much, it is too generic. So I suggest

class SQLIdentifiers { // note the plural

    static Quoter createQuoter(DMD dmd);

    static boolean isSQL99Keyword(String identifier); // in a later API version

    static class Quoter {

        String quoteIfNeeded(String identifier);
    }
}

David: I like this, and I made these changes (except I didn't add isSQL99Keyword).

AB06 The method seems to quote an already quoted identifier (e.g., when passed "foo" it will return ""foo""). So it
seems to expect unquoted identifiers (not surrounded by quotes, to be precise). This needs to be mentioned in the Javadoc.

David: I fixed it to ignore an identifier that is already quoted.

AB07 the current SQLIdentifier class is not thread-safe. This needs to be mentioned in the Javadoc. Or probably better
to make it thread-safe.

David: I fixed this by making it immutable (all fields final and initialized in the constructor)

AB08 Please make the dbmd field final, regardless of AB07.

David: Done

AB09 Perhaps should log warnings about failed DMD method calls with a WARNING level rather than INFO.

David: Done

AB10 I would discourage allowing null as a valid parameter to quoteIfNeeded(). I would rather throw a
NullPointerException in that case. You can use org.openide.util.Parameters.notNull() for that. You should also make this
check as the very first thing in the method body (before computing nonQuoteChars.

David: Done

AB11 The comments in the containsLowerCase() and containsUpperCase() methods do not make sense.

David: Sorry, cut-and-paste error

AB12 Method containsLowerCase() can be simplified to test if (ch >= 'a' || ch <= 'z') and you get can rid of the
UC_CHARS and LC_CHARS constants. Or you perhaps can refactor both methods to a single containsRange(char start, char
end, String str). 

David: Doh!  I *knew* there had to be a better way.  I wouldn't have passed a Google interview :).  I don't like
containsRange, it's basically as much writing as the other way, and actually obfuscates things a bit.  BTW, I think you
mean't '&&' and not '||'.

AB13 The code does not seem to deal with getIdentifierQuoteString() returning a space, which its Javadoc says it can.

David: Hm, I didn't see this.  At any rate, with the changes I made, I definitely don't see this.  Sorry if I'm missing
it, can you point this out?

Here's what I have now.  quoteString is initialized in the constructor now...: 

        public final String quoteIfNeeded(String identifier) {
            Parameters.notNull("identifier", identifier);
            
            if ( needToQuote(identifier) ) {
                return quoteString + identifier + quoteString;
            }

            return identifier;
        }


AB14 The value of the NONQUOTE_CHARS field is missing "7".

David: not relevant any more, as I have removed that field

AB15 Is it really necessary to have all those escaped characters in testNonAscii()? A single character should suffice
for the test.

David: Yes, I know.  This was just for fun, as it's the first message in the Japanese version of the Derby message file.
 If you don't mind I'd like to keep it - no real harm...

Re AB16: Removed this comment, no biggie

Created attachment 48607 [details]
Updated patch file based on feedback

One more comment: we haven't resolved where to put this API.  Andrei, are you OK with it going under the db module?  If
not, what do you suggest I do?

Thanks.

Yes, I would put this in the db module. Not sure about the package though. Initially I thought
o.n.api.db.explorer.support. But perhaps a package like o.n.api.db.sql.support would be better, since this class has
nothing do to with the DB Explorer.

Re AB13: disregard that, I missed the trim() call on the string returned by getExtraNameCharacters().

Thank you for addressing my comments. I have several more on the new code, again, in no particular order:

AB17 You need to update the class name in apichanges.xml and arch.xml.

AB18 Please add a private constructor to SQLIdentifiers to ensure that it can't be instantiated.

AB19 SQLIdentifiers.LOGGER looks unused.

AB20 I would use an assertion in needToQuote() instead of an exception. I think public API methods should not use
assertions to ensure valid parameters values, but they should throw exceptions. This ensures parameter values are
enforced even when assertions are turned off. OTOH, I think assertions serve private methods better. They are simpler to
write and they just need to assert that the validity of the parameter has been checked somewhere else, usually in the
public entry point. That is, turning them off should cause no harm. But this is just a personal opinion, feel free to
leave the code as it is if you don't agree.

AB21 I just realized some databases might not accept unquoted identifiers starting with a number, such as 1FOO. Derby
doesn't, but it accepts such identifiers if they are quoted, e.g., "1FOO" is fine. Perhaps we should be conservative and
quote such identifiers.

AB22 (nitpick) The private methods taking DMD as a parameter (getExtraNameChars(), etc.) can be made static too.

AB23 You can simplify testNullIdentifier():

try {
    quoter.quoteIfNeeded(null);
    fail();
} catch (NPE npe) {
    // expected
}

Hi, Andrei.  I'm working on your most recent set of comments, but one question I have is, how to work with the
architecture and API changes files when I change the package to o.n.a.db.sql.support.  When I generate the Javadoc it
doens't include this package, and the Javadoc/Arch Description is entitled "Database Explorer".

How do I go about fixing this?  I'm a little lost here.

Thanks,

David

Andrei: never mind, I think I figured it out :)

Andrei: Yes, I would put this in the db module. Not sure about the package though. Initially I thought
o.n.api.db.explorer.support. But perhaps a package like o.n.api.db.sql.support would be better, since this class has
nothing do to with the DB Explorer.

David: yes, that makes sense to me, done.

AB17 You need to update the class name in apichanges.xml and arch.xml.

David: done

AB18 Please add a private constructor to SQLIdentifiers to ensure that it can't be instantiated.

David: done

AB19 SQLIdentifiers.LOGGER looks unused.

David: removed

AB20 I would use an assertion in needToQuote() instead of an exception. I think public API methods should not use
assertions to ensure valid parameters values, but they should throw exceptions. This ensures parameter values are
enforced even when assertions are turned off. OTOH, I think assertions serve private methods better. They are simpler to
write and they just need to assert that the validity of the parameter has been checked somewhere else, usually in the
public entry point. That is, turning them off should cause no harm. But this is just a personal opinion, feel free to
leave the code as it is if you don't agree.

David: yes, this makes sense, and is what I normally do.  I just missed fixing this when I moved the NPE up to the
public API.  Changed.

AB21 I just realized some databases might not accept unquoted identifiers starting with a number, such as 1FOO. Derby
doesn't, but it accepts such identifiers if they are quoted, e.g., "1FOO" is fine. Perhaps we should be conservative and
quote such identifiers.

David: Man, you *are* good!  I wish I could think through all corner cases like that, you truly inspire me (seriously)!
 I think underbar ('_') is also iffy as a first char.  I am quoting any identifiers that start with a number or underbar
now.

AB22 (nitpick) The private methods taking DMD as a parameter (getExtraNameChars(), etc.) can be made static too.

David: yes, you're right, cool. Done.

AB23 You can simplify testNullIdentifier():

try {
    quoter.quoteIfNeeded(null);
    fail();
} catch (NPE npe) {
    // expected
}

David: I should listen to that uneasy feeling that there's a better way and try to find it.  Thanks.

Created attachment 48678 [details]
New revision based on latest input from Andrei

Thanks. Looks good now, just a final nitpick:

AB24: please keep the packages sorted in the public-packages element in project.xml. The api.db.sql.support package
should come before the spi.db.explorer package.

Fixed that, Andrei, but not bothering with a new patch.

Unless I hear any objections, I'll assume this API review is closed and will assign it to myself on Monday and check in
the changes.

Reassigning to myself, API review passes

cvs server: scheduling file `catalog.xml' for addition
cvs server: use 'cvs commit' to add this file permanently
Checking in test/unit/src/org/netbeans/modules/db/util/DBTestBase.java;
/cvs/db/test/unit/src/org/netbeans/modules/db/util/DBTestBase.java,v  <--  DBTestBase.java
new revision: 1.3; previous revision: 1.2
done
Checking in nbproject/project.xml;
/cvs/db/nbproject/project.xml,v  <--  project.xml
new revision: 1.23; previous revision: 1.22
done
RCS file: /cvs/db/catalog.xml,v
done
Checking in catalog.xml;
/cvs/db/catalog.xml,v  <--  catalog.xml
initial revision: 1.1
done
Checking in apichanges.xml;
/cvs/db/apichanges.xml,v  <--  apichanges.xml
new revision: 1.7; previous revision: 1.6
done
Checking in arch.xml;
/cvs/db/arch.xml,v  <--  arch.xml
new revision: 1.16; previous revision: 1.15
done
RCS file: /cvs/db/test/unit/src/org/netbeans/api/db/sql/support/QuoterTest.java,v
done
Checking in test/unit/src/org/netbeans/api/db/sql/support/QuoterTest.java;
/cvs/db/test/unit/src/org/netbeans/api/db/sql/support/QuoterTest.java,v  <--  QuoterTest.java
initial revision: 1.1
done
RCS file: /cvs/db/src/org/netbeans/api/db/sql/support/SQLIdentifiers.java,v
done
Checking in src/org/netbeans/api/db/sql/support/SQLIdentifiers.java;
/cvs/db/src/org/netbeans/api/db/sql/support/SQLIdentifiers.java,v  <--  SQLIdentifiers.java
initial revision: 1.1
done

Watch out, the CVS support was too smart and it commited the catalog.xml file, which I guess you didn't intend to commit.