Discussion: Fluent builder api

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

https://github.com/JSQLParser/JSqlParser/issues/1004

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.

@gitmotte

@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.

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.

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

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.

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?

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);
    }

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);

...

}

https://github.com/JSQLParser/JSqlParser/issues/1004

@AnEmortalKid

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?

JSQLParser#1004 add chaining - methods returning "this"

* add<Name> methods on collection-fields with varargs-parameter

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

@wumpz

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

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

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 ...

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.

* add with prefix for fluent setters.

* add with prefix for fluent setters. (revert to chaining setters, do

@wumpz

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

@gitmotte

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

@gitmotte

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

Fluent builder api #1004 (#1014)

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

Fluent builder api JSQLParser#1004 (JSQLParser#1014)

Validation visitor framework (#1045)

Closes JSQLParser#1004, removed superfluous constructor.

Navigation Menu

Repository navigation

Discussion: Fluent builder api #1004

16 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Discussion: Fluent builder api #1004

Description

Activity

gitmotte commented on Jun 27, 2020

AnEmortalKid commented on Jun 27, 2020

gitmotte commented on Jun 27, 2020

gitmotte commented on Jun 27, 2020

gitmotte commented on Jun 27, 2020

AnEmortalKid commented on Jun 27, 2020

gitmotte commented on Jun 27, 2020

gitmotte commented on Jun 29, 2020

gitmotte commented on Jun 29, 2020

AnEmortalKid commented on Jul 3, 2020

AnEmortalKid commented on Jul 3, 2020

16 remaining items

wumpz commented on Jul 4, 2020

gitmotte commented on Jul 4, 2020

wumpz commented on Jul 4, 2020

gitmotte commented on Jul 14, 2020

AnEmortalKid commented on Jul 14, 2020

wumpz commented on Jul 15, 2020

wumpz commented on Aug 25, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions