ebean icon indicating copy to clipboard operation
ebean copied to clipboard

Add also() for query beans

Open artemik opened this issue 1 year ago • 1 comments

Feature Request

Please add also() function (unconditional version of alsoIf()) into query beans. Example usage would be:

new QOrder()
    ...
    .also(isNotDeleted());

Current Workaround

Hardcode true for conditional alsoIf():

new QOrder()
    ...
    .alsoIf(() -> true, isNotDeleted());

Why

Better readability. Code reuse. It would be very convenient for applying group of conditions that form a single meaning, especially when such logic could be shared between multiple query bean creations.

Possible examples are:

.also(isNotDeleted()) // some custom soft deletion logic
.also(withinDateRange(start, end)) // some date range check
.also(withPaging(pageParams)) // setFirstRow(...), setMaxRows(...) cursor paging

artemik avatar Aug 26 '24 20:08 artemik

I've created a PR that uses also(Consumer<QueryBuilder<?,?>> apply). So this uses QueryBuilder rather than SELF as my thinking is that this is for general functions to apply query features like paging.

An alternative would be to use the more specific Consumer<SELF> instead and then adapt that noting that SELF is specific to each query bean type. This could be a touch more cumbersome when we would look to apply generic logic.

The Consumer<QueryBuilder<?,?>> use looks like:

  @Test
  void also() {
    var myPager = new MyPager(10);
    var q = new QCustomer()
      .select(name)
      .also(myPager)
      .query();

    q.findList();
    assertThat(q.getGeneratedSql()).contains("select /* QueryAlsoIfTest.also */ t0.id, t0.name from be_customer t0 limit 10");
  }

  static class MyPager implements Consumer<QueryBuilder<?,?>> {

    final int maxRows;

    MyPager(int maxRows) {
      this.maxRows = maxRows;
    }

    @Override
    public void accept(QueryBuilder<?, ?> queryBuilder) {
      queryBuilder.setMaxRows(maxRows);
    }
  }

rbygrave avatar Aug 27 '24 11:08 rbygrave

I'm afraid (just like you explained) Consumer<QueryBuilder<?,?>> apply, won't allow to set something specific to a given query bean like .also(withinDateRange(start, end)) // some date range check which I think is very valuable.

Another concern is that such signature would differ from alsoIf().

But I also see now that in this case it's vice versa with SELF instead of Consumer<QueryBuilder<?,?>> it won't be possible to have an also() accepting a generalized logic (to any query bean) which is valuable as well.

Is there some solution to support both? Some kind of method overloading? Or two methods (like "also()" and "alsoApply()", i.e. stupid naming but on the other hand would do the job)?

artemik avatar Aug 30 '24 20:08 artemik

Is there some solution to support both?

I'm thinking going with also(Consumer<SELF> apply) and match the alsoIf(...) signature. This is the specific type but ultimately query beans extend QueryBuilder so code can consume the specific type and then internally use the generic QueryBuilder.

This might not be perfect vs adding some sort of generic alsoApply(Consumer<QueryBuilder<?,?>>) ... but I think we should think about this generic case more. I'm comfortable that adding the also(Consumer<SELF> apply) to match alsoIf() is good, so lets do that and see if the generic cases come up that are not quite satisfactory.

rbygrave avatar Sep 09 '24 10:09 rbygrave

Makes sense, thanks.

artemik avatar Sep 09 '24 13:09 artemik