Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add also() for query beans #3461

Closed
artemik opened this issue Aug 26, 2024 · 4 comments · Fixed by #3472 · May be fixed by #3465
Closed

Add also() for query beans #3461

artemik opened this issue Aug 26, 2024 · 4 comments · Fixed by #3472 · May be fixed by #3465
Assignees
Milestone

Comments

@artemik
Copy link

artemik commented Aug 26, 2024

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
@rbygrave
Copy link
Member

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

@artemik
Copy link
Author

artemik commented Aug 30, 2024

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

@rbygrave
Copy link
Member

rbygrave commented Sep 9, 2024

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 added a commit that referenced this issue Sep 9, 2024
@rbygrave rbygrave linked a pull request Sep 9, 2024 that will close this issue
rbygrave added a commit that referenced this issue Sep 9, 2024
@rbygrave rbygrave self-assigned this Sep 9, 2024
@rbygrave rbygrave added this to the 14.5.2 milestone Sep 9, 2024
@artemik
Copy link
Author

artemik commented Sep 9, 2024

Makes sense, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
@rbygrave @artemik and others