Suggestions for limt() and offset() improvements to SelectDSL

[mailingfeng]

Version: 1.5.2

Question

There are paging or non-paging query scenarios.

In API design, offset[Long] and limit[Long] are generally used. When a non-paginated scenario is required, the arguments offset=null and limit=null are passed in.

However, the current arguments to limit() and offset() are long, which requires a non-null check on RequestBean's limit and offset, which is not elegant enough.

    public LimitFinisher limit(long limit) {
        this.limit = limit;
        return new LimitFinisher();
    }

    public OffsetFirstFinisher offset(long offset) {
        this.offset = offset;
        return new OffsetFirstFinisher();
    }

Suggestion

The arguments to limit() and offset() are changed to Long, which is not change the current code logic.

ps: same to other xxxDSL

Looking forward to your reply~!

    public LimitFinisher limit(Long limit) {
        this.limit = limit;
        return new LimitFinisher();
    }

    public OffsetFirstFinisher offset(Long offset) {
        this.offset = offset;
        return new OffsetFirstFinisher();
    }

[jeffgbutler]

Well it's not as simple as that. If you passed a null to one of those methods, it would render something like limit(#{parameters.p1}) where the value of p1 is null. That seems like a poor outcome.

So what would you have the library do if you passed a null into one of these methods? I assume you would like the library to drop those phrases out of the rendered SQL. In that case, then I think it would be more in keeping with the rest of the library to make methods like limitIfPresent and offsetIfPresent.

[mailingfeng]

Thank you for your reply.

In existing code, SelectDSL & PagingModel member variables limit and offset are now of type Long.

PagingModel already handles cases when limit or offset is null, so no other changes should be required ?

public class PagingModel {

    private final Long limit;
    private final Long offset;
    private final Long fetchFirstRows;

    private PagingModel(Builder builder) {
        super();
        limit = builder.limit;
        offset = builder.offset;
        fetchFirstRows = builder.fetchFirstRows;
    }

    public Optional<Long> limit() {
        return Optional.ofNullable(limit);
    }

    public Optional<Long> offset() {
        return Optional.ofNullable(offset);
    }

    public Optional<Long> fetchFirstRows() {
        return Optional.ofNullable(fetchFirstRows);
    }

    public static class Builder {
        private Long limit;
        private Long offset;
        private Long fetchFirstRows;

        public Builder withLimit(Long limit) {
            this.limit = limit;
            return this;
        }

        public Builder withOffset(Long offset) {
            this.offset = offset;
            return this;
        }

        public Builder withFetchFirstRows(Long fetchFirstRows) {
            this.fetchFirstRows = fetchFirstRows;
            return this;
        }

        public Optional<PagingModel> build() {
            if (limit == null && offset == null && fetchFirstRows == null) {
                return Optional.empty();
            }

            return Optional.of(new PagingModel(this));
        }
    }
}

Of course I think your idea is better: limitIfPresent and offsetIfPresent

[mailingfeng]

Add a code snippet that illustrates my question:

requires a non-null check on limit and offset, which is not elegant enough.

    private final CorpMapper mapper;

    public List<Corp> select(CorpQuery query) {

       if (query.getPageSize() != null && query.getPageSize() > 0 && query.getOffset() != null && query.getOffset() >= 0) {
            return mapper.select(c -> c.where(area, SqlBuilder.isEqualTo(query.getArea()))
                    .limit(query.getPageSize())
                    .offset(query.getOffset())
            );

       } else {
            return mapper.select(c -> c.where(area, SqlBuilder.isEqualTo(query.getArea())));
       }

    }

  @Data
  public class CorpQuery {
      private String area;
      private Long offset;
      private Long pageSize;
  }

[jeffgbutler]

Yeah I get it - but if I change every method to Long it breaks dozens of tests and makes it impossible to use int literals - which is a hassle. So I will probably keep the long methods and add whenPresent methods for your use case. Then it will be consistent with other whenPresent methods in the library.

[mailingfeng]

Thanks and looking forward to new version~!

[jeffgbutler]

I've added this to the backlog for V2 (#827)

[mailingfeng]

I've added this to the backlog for V2 (#827)

We have a lot of projects based on Java8, and that this issue has not changed much, can it be fixed in 1.5.x?

need us to submit a PR?

[jeffgbutler]

We're not doing any more Java 8 releases. The entire MyBatis ecosystem system is moving to Java 17. The code has already been updated such that it won't compile in Java 8 anymore.

[mailingfeng]

OK，We will consider upgrading Java21.

What is the release time of v2?