In fact... another possibility to get confused between String (as in *ByName), String (as in Plain SQL) is the String (as in bind value) type. With all the convenience methods that allow for String arguments, the ordinary String binding to a Field's <T> type can be yet again, confusing.
I guess that in the long run, the ideal solution is really to accept the fact that the existing convenience is misleading and inconsistent, and that AST elements simply always have to be wrapped in AST construction methods, while the String type is reserved to bind values for Field<String>. 2014-12-09 9:45 GMT+01:00 Lukas Eder <[email protected]>: > > > 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.
