Discussion: Fluent builder api

[gitmotte]

Is it desirable that the model also supports the builder pattern?

If we change setters and add* methods returning "this" building sql will be very easy ... i.e.

@Test
    public void testParseAndBuild() throws JSQLParserException {
        String sql = "SELECT * FROM tab1 t1 "
                + "JOIN tab2 t2 ON t1.ref = t2.id WHERE t1.col1 = ? OR t1.col2 = ?";
        Statement stmt = CCJSqlParserUtil.parse(sql);
        StringBuilder buffer = new StringBuilder();
        stmt.accept(new StatementDeParser(buffer));
        assertEquals(sql, buffer.toString());

        Table t1 = new Table("tab1").setAlias(new Alias("t1", false));
        Table t2 = new Table("tab2").setAlias(new Alias("t2", false));

        Select select = new Select().setSelectBody(
                new PlainSelect().setFromItem(t1)
                        .setSelectItems(Arrays.asList(new AllColumns()))
                        .setJoins(
                                Arrays.asList(new Join().setRightItem(t2).setOnExpression(
                                        new EqualsTo(new Column(t1, "ref"), new Column(t2, "id")))))
                        .setWhere(new OrExpression(new EqualsTo(new Column(t1, "col1"), new JdbcParameter()),
                                new EqualsTo(new Column(t1, "col2"), new JdbcParameter()))));
        buffer = new StringBuilder();
        select.accept(new StatementDeParser(buffer));
        assertEquals(sql, buffer.toString());
    }

I did a fork starting the change to get a picture of it.

See https://github.com/gitmotte/JSqlParser

[gitmotte]

Downside:

• none-void setter-methods are not valid to the javabean specification. An alternative is to add methods named without "set" prefix at all or use the prefix "with", using the regular setter inside and return "self()".
• adds a type parameter declaring the type of "this", which is needed to return the real object-type, not it's possibly abstract parent. Therefore users will get a compiler warning on using those interfaces an abstract classes without type-declaration.

[AnEmortalKid]

@gitmotte how do you feel about:

Select select = Select.builder().selectBody(
                            PlainSelect.builder().fromItem(1).selectItems(Arrays.asList(new AllColumns())
                             .joins(Arrays.asList(Join.builder().rightItem(t2).onExpression( new EqualsTo(new Column(t1, "ref"), new Column(t2, "id"))))
                              .where(....etc)
.build())
.build();

Where we make the builder accessible through a static method, it can have not the 'set' methods , maybe we get rid of build() with the caveat that it's not returning an immutable object. But that builder lets us not deal with the restriction of void methods.

[gitmotte]

The effect would be using a static construction method "builder" instead of "new". Static constructors are ok for me, but i would prefer a common util class (i.e. JSQL.select() { return new Select() ;} ) providing static construction methods for this.

i.e:

Select select = JSQL.select().selectBody( //
                JSQL.plainSelect() //
                        .selectItems(JSQL.allColumns()) //
                        .fromItem(t1) //
                        .addJoins(JSQL.join().rightItem(t2)
                                .onExpression(JSQL.equalsTo(JSQL.column(t1, "ref"), JSQL.column(t2, "id"))))

But i'm ok with the new operator too! The utility class may contain more complex shortcut-methods for building sql.

[gitmotte]

I'm ok with methods without prefix "set" and "with"-methods are good option too, not breaking javabean spec. What is your preference?

[gitmotte]

The fluent api should operate directly on a mutable object. If you think of the usecase of manipulate a query after parsing, a real builder does not fit the needs.

[AnEmortalKid]

This sort of feels like jooq to me, particularly the JSQL.select() portions.

not breaking javabean spec

Isn't anything outside set for mutators, get/is for accesors not part of the bean spec?

[gitmotte]

This sort of feels like jooq to me, particularly the JSQL.select() portions.

I see nothing wrong with that.
But if you like the method within the object and change the name of "builder()" into "create()" it would be more clear that you will create a mutable object without the need to warn about.

Select select = Select.create().selectBody(
                            PlainSelect.create().fromItem(1).selectItems(AllColumns.create())
                             .joins(Join.create().rightItem(t2).onExpression(  
EqualsTo.create( Column.create(t1, "ref"),  Column.create(t2, "id")))
                              .where(....etc)

But leave the empty constructor public because of the javabean-spec !

Isn't anything outside set for mutators, get/is for accesors not part of the bean spec?

Yes, get/is are part of the javabean-spec too, but we won't change them.

For all Collection - fields i'd like to add elements without the need to create an empty list.
i.e.

public PlainSelect addJoins(Join... joins) {
        Collections.addAll(ensureNotNull(getJoins()), joins);
        return this;
    }

    public PlainSelect joins(Join... joins) {
        return joins(Arrays.asList(joins));
    }

    public PlainSelect joins(List<Join> joins) {
        setJoins(joins);
        return this;
    }

    public void setJoins(List<Join> list) {
        joins = list;
    }
    
    private <T> List<T> ensureNotNull(List<T> list) {
        return Optional.ofNullable(list).orElseGet(ArrayList::new);
    }

[gitmotte]

adds a type parameter declaring the type of "this", which is needed to return the real object-type, not it's possibly abstract parent. Therefore users will get a compiler warning on using those interfaces an abstract classes without type-declaration.

I reverted this change, because it makes the api incompatible with users of older versions. As an alternative we are able to overwrite fluent setter methods within the concrete type returning the specific concrete type casting the super-result.

i.e.

public class Table implements FromItem {

...

    @Override
    public Table alias(Alias alias) {
        return (Table) FromItem.super.alias(alias);
    }

...

}

and

public interface FromItem {

...

     default FromItem alias(Alias alias) {
        setAlias(alias);
        return this;
    }

    void setAlias(Alias alias);

...

}

[gitmotte]

Check out new branch master.fluent to get a picture of all api addons i'd like to add:

• chaining - methods returning "this"
• overwrite chaining - methods of abstract parents/interfaces for
  returning concrete type
• add methods on collection-fields with varargs-parameter
• add public T get(Class) - casting and returning an inner
  interface-type
• (optional) create(...) methods

@AnEmortalKid @wumpz how do you feel about the proposed changes? Whould you merge them into your lib?

[AnEmortalKid]

Comparison link: master...gitmotte:master.fluent

[AnEmortalKid]

• chaining - methods returning "this" ✅
• overwrite chaining - methods of abstract parents/interfaces for returning concrete type ✅
• add methods on collection-fields with varargs-parameter ✅
• add public T get(Class) - casting and returning an inner interface-type ✅
• (optional) create(...) methods ✅

Those seem fine to me, it's @wumpz 's call.

Is the expectation here that future enhancements also include chained methods like these?

I think if you were to PR this, you'd probably have to do it in chunks just cause 100+ files is harder to parse through mentally

[gitmotte]

Is the expectation here that future enhancements also include chained methods like these?

Yes, i'd expect this, but it's not that critical if it is missed in some cases.

I think if you were to PR this, you'd probably have to do it in chunks just cause 100+ files is harder to parse through mentally

Well, i'm committing currently chunks. Those committed changes are generated with javaparser, manually reviewed and reverted format-changes, that there are only adds on top of the current api.

[wumpz]

Sorry. Was out of Jsqlparser business for some days now. I will look into the proposed changes.
One first comment or info:
This change would go into a new main version. So API changes should not be a problem, if this would make it easier implementing this.

Without looking into the last changes here another comment. Since this would drive JSqlParser more in a SQL builder library the change should somehow try to make it hard to construct invalid SQLs. E.g. a lot of setters accept Expression instances because this is the common interface of the instances that should go in there. But in fact you could put in any Expression implementing instance into it. Using the library as a parser this restriction is maintained by the grammar. Using it as a builder it's not restricted.
I know this is a problem even without this change.

[gitmotte]

A pragmatic approach would be using the deparser to build sql and validate it by parsing it again.

Enforcing this by intruducing more subinterfaces to expression would be a good step, but it may be a second or parallel step to clean those things.

Btw. sometimes the generator does not find a setter-/getter-method because of naming-issues. i.e. the field is named using a abbreviation.
Those changes - renaming the private field to match a javabean-style setter are possible without an api break.

Personally, i would prefer to add @deprecated annotations to methods which will removed at next Release

[wumpz]

Sure a very pragmatic approach. Regarding performance this would be the last try I think.
I am not quite sure if this grouping of classes via interfaces is a good idea. In the worst case we should build a different interface for each method, because of different groupings for different methods. Right?

What about some kind of annotation stuff like

@Allow( Column.class, Function.class, ...)
public void setMyExpr(Expression expr)

Writing this I surely know, that this is a step in your validation stuff. But using this we would be able to constrain it while building it.

[gitmotte]

No, i don't think that annotations are the best approach for this, but on other side the standard bean-validation api in java using this pattern. The difference is, that it validates the content, but does not check the type itself. (i.e. javax.validation.NotNull, org.hibernate.validator.constraints.ISBN)

Why can't we split Column into Column and ColumnExpression ?

[wumpz]

I looked into your changes:

• chaining - methods returning "this" ✅ But leave the original ones intact. In fact setters should not return anything, so I would prefer the with way.
• overwrite chaining - methods of abstract parents/interfaces for returning concrete type ✅
• add methods on collection-fields with varargs-parameter ✅
• add public T get(Class) - casting and returning an inner interface-type

In fact I am no fan of this. I see no value in doing

MyExpression myexpr = myclass.getExpression(MyExpression.class);

instead of

MyExpression myexpr = (MyExpression)myclass.getExpression();

• (optional) create(...) methods ✅
  • I think this one will be valuable, since it combines the needed parameters in the right way. There are a lot of classes within JSqlParser that has grown fairly complex (e.g. AnalyticExpression). So you need to know what you are doing. The create methods could take at least a bit of complexity from the user.

[wumpz]

@gitmotte
What do you mean by "Why can't we split Column into Column and ColumnExpression ?"?

.. and sorry if I got you wrong. :)

We want to allow only specific extensions of Expression in a method. What should this ColumnExpression be for? In fact, we need an ExpressionType interface that somehow combines all possible classes / implementation for this method. So looking at my annotation example this grouping interface has to combine Column and Function, so we got a new grouping interface ColumnFunctionGroup. Since the groups are getting more and more complex I had constructed this worst-case scenario above.

IMHO using the interface stuff in this way it is a no go.

[wumpz]

@gitmotte
Since this is all about validation, maybe we should indeed try not to mix up this building stuff with the validation stuff.

So this one could be checked through some kind of validation visitor, like you proposed in your other issue.

[gitmotte]

* add public T get(Class) - casting and returning an inner interface-type

In fact I am no fan of this. I see no value in doing

MyExpression myexpr = myclass.getExpression(MyExpression.class);

instead of

MyExpression myexpr = (MyExpression)myclass.getExpression();

the advantage is, that you save a lot of brackets, if you want to chain the getter-methods.
ie.

ExpressionList list = select.getSelectBody(PlainSelect.class).getWhere(AndExpression.class)
                .getRightExpression(InExpression.class).getRightItemsList(ExpressionList.class);

* (optional) create(...) methods white_check_mark
  
  * I think this one will be valuable, since it combines the needed parameters in the right way. There are a lot of classes within JSqlParser that has grown fairly complex (e.g. AnalyticExpression). So you need to know what you are doing. The create methods could take at least a bit of complexity from the user.

but I'm not sure I'm convinced of that. we now have nice methods for chaining - there are no constructors needed at all.
If you think of java-beans there should be always an empty constructor. And you can provide additional constructors for parameter combinations

new AndExpression(left, right)

instead of

AndExpression.create(left, right)

[gitmotte]

Since this is all about validation, maybe we should indeed try not to mix up this building stuff with the validation stuff.

So this one could be checked through some kind of validation visitor, like you proposed in your other issue.

yes, that's a different subject, which fits more to validation.
I think i misunderstood you by thinking a Column is an expression only in exceptional cases.

[gitmotte]

except for the create methods all changes are committed.

• chaining - methods returning "this" white_check_mark But leave the original ones intact. In fact setters should not return anything, so I would prefer the with way.
• overwrite chaining - methods of abstract parents/interfaces for returning concrete type white_check_mark
• add methods on collection-fields with varargs-parameter white_check_mark
• add public T get(Class) - casting and returning an inner interface-type

Now the coverage decreased from 83.6% to 80.1% - if we use the chaining api within the parser, we can push it up again.

[wumpz]

the advantage is, that you save a lot of brackets, if you want to chain the getter-methods.
ie.

Agreed. The fluent usage is much more concise. You are creating a second way of addressing parse tree objects. Since the visitor stuff is sometimes daunting. But this would be a massive change / addition, right?

void setExpr(Expression expr);
Expression getExpr();
T get(Class<T>);
MyClass withExpr(Expression expr);

So the last both have to be implemented for all methods, to be complete.

[wumpz]

So I assume you somehow limit the scope of where the changes should happen.

[gitmotte]

the advantage is, that you save a lot of brackets, if you want to chain the getter-methods.
ie.

Agreed. The fluent usage is much more concise. You are creating a second way of addressing parse tree objects. Since the visitor stuff is sometimes daunting. But this would be a massive change / addition, right?

void setExpr(Expression expr);
Expression getExpr();
T get(Class<T>);
MyClass withExpr(Expression expr);

So the last both have to be implemented for all methods, to be complete.

The casting getter is implemented for all methods returning interfaces or abstract types, the last method (chaining setter) is implemented for all setters (but i'ts named without prefix "with" at the moment)

void setExpr(Expression expr);
Expression getExpr();
<T extends Expression> T getExpr(Class<T>);
MyClass expr(Expression expr);

The "with" prefix i would add if you prefer it, but next week ...

[wumpz]

The get instead of getExpr was simply a typo. :)

If I didn't overlooked something the changes should be backward compatible. The performance shouldn't be touched. So we don't even make a new major version.

So this will be a huge improvement due to the useability of this tool. Nice.

I would indeed prefer the with prefix.

[gitmotte]

@wumpz @AnEmortalKid i put a merge request for this issue some days ago, have you seen it?

[AnEmortalKid]

negative @gitmotte , lost track of it since your other PR was merged :) thx for the reminder, i'll take a look tonight!

[wumpz]

@gitmotte thx for reminding me. I got into another issue.

[wumpz]

Since the corresponding pull request was merged, I close this issue.