2014-12-09 0:37 GMT+01:00 Alok Menghrajani <[email protected]>:
> On Mon, Dec 1, 2014 at 11:23 PM, Lukas Eder <[email protected]> wrote:
> > There is certainly room for improvement in that area. The two modes of
> > operation are very similar in the way they're used throughout the API as
> > both operations simply require String arguments, and String doesn't have
> > have a semantics per se.
>
> Correct. You could require the raw SQL statements to be wrapped in
> DSL.raw():
>
> DSL.select().from(DSL.raw("plain SQL table"))
>
"raw()" by itself would be insufficient, because there are currently four
different types of objects that can be constructed this way: Schema, Table,
Field, and Sequence. It would be inevitable to call those methods
rawSchema(), rawTable(), rawField(), rawSequence() ... or just ...
schema(), table(), field(), sequence() :-)
but see more on that below...
vs
>
> DSL.drop("tableByName")
>
> The advantage of doing this is that:
> * Forgetting to do something (i.e. forgetting to call DSL.raw) does
> not lead to potentially unsafe code.
>
That's a good point, although, you're inserting strings into the API. This
has an implicit feel of "ooh, am I doing this right?" I agree, though that
terms like "raw" would make users even more alert.
> * The code is more explicit. When you are reading the code, you know
> what is going on.
>
I personally think that explicitness is most useful with the "*ByName"
semantics where Strings must only contain schema/table/field/sequence
identifiers, which is a big feature reduction compared to raw/plain SQL
from a cognitive perspective. Many people currently get this wrong.
With plain SQL, you will always know what's going on when reading the code,
though, I suspect? E.g. these two:
DSL.using(configuration)
.select(count(VBL_DEFAULT.ID))
.from(table("vbl_default partition(p_04-Dec-14)"))
.where(...);
and
DSL.using(configuration)
.select(count(VBL_DEFAULT.ID))
.from("vbl_default partition(p_04-Dec-14)")
.where(...);
Specifically, the version where a String is passed to "from" seems
particularly readable to me.
* If DSL.raw is used consistently, it makes it easy to audit / setup a
> commit hook / highlight things at code review time / etc.
>
This could also be achieved with a new annotation, which could be checked
via JSR-308. I've had a very interesting discussion with spec lead Mike
Ernst recently, about the potential of JSR-308 validation of the current
@Support annotations to check that only API supported by the underlying
database is used. He was absolutely thrilled by the idea of doing this in
jOOQ, he almost forgot his dinner ;-)
In any case, I've registered an issue for adding @PlainSQL annotations:
https://github.com/jOOQ/jOOQ/issues/3851
No matter how we evolve this, it would be useful to annotated potentially
dangerous methods this way. We have already added the same SQL injection
warning in all Javadocs, but formal client-side validation might be even
better. We'll be doing some research and blog about JSR-308 pretty soon...
> One way to disambiguate this would be to deprecate the tableByName and
> > fieldByName methods (and all methods that accept strings with a "Name"
> > semantics), and offer alternative API methods accepting the
> org.jooq.Name
> > type:
> >
> > DSL.table(DSL.name("tableByName"));
> > DSL.field(DSL.name("fieldByName"), Integer.class);
>
> Would you keep the DSL.table(String), DSL.field(String) methods?
Yes, probably.
> If yes, I think it's safer to do it the other way around for the reasons
> I mentioned above.
>
Maybe, but it would be a heavy change and chances are that once you do need
some String version of an object (raw or just the name), that the name will
not be sufficient for very long and you will have to switch again to raw
SQL.
If we fusion your ideas with mine, the safest and most thorough thing would
be to create AST elements for everything and to gently remove the current
"dangerous" convenience methods (although I will think about this
thoroughly, as it is really a big change).
*Pseudo-API:*
class DSL {
// create names for use with any other AST element
static Name name(String... qualifiedName) { ... }
// create plain SQL for use with any other AST element
static SQL sql(String sql) { ... }
static SQL sql(String sql, Object... bindings) { ... }
static SQL sql(String sql, QueryPart... parts) { ... }
// Use Name or SQL in typed AST elements
static Field<Object> field(Name name) { ... }
static Field<Object> field(SQL sql) { ... }
static Table<?> table(Name name) { ... }
static Table<?> table(SQL sql) { ... }
static DropTableStep dropTable(Table<?> table) { ... }
static DropTableStep dropTable(Name name) { ... }
}
interface SelectWhereStep {
SelectConditionStep where(SQL sql) { ... }
}
interface Name extends QueryPart { ... }
interface SQL extends QueryPart { ... }
Marked in yellow: The only API elements that really take plain SQL
arguments - just like your "DSL.raw()".
The advantage of this new SQL type would be the fact that it can
immediately be embedded in other plain SQL templates, as in
QueryPart anyExpression = DSL.sql("abc + xyz");
DSL.field(DSL.sql("some_function({0})", anyExpression));
The same way that Name can already be embedded, or even keywords via
DSL.keyword()
> This would make using the jOOQ API a bit more verbose, but it might still
> be
> > acceptable.
>
> The API becomes a bit more verbose (an extra method call around a
> bunch of (hopefully) static strings). However don't forget the API
> becomes simpler (since you can deprecate/remove tableByName,
> fieldByName, etc.). Overall it's a reduction in the cognitive work for
> the library users.
>
Exactly. This confusion popped up too many times, which means that the
current status quo is more than insufficient from a "cognitive work"
perspective.
Even if maybe plain SQL should be restricted to only very few methods, all
Fields should be obtained via overloaded DSL.field(...) methods, Tables
should be obtained via overloaded DSL.table(...) methods.
--
You received this message because you are subscribed to the Google Groups "jOOQ
User Group" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
