DBManager: getAll now accepts DBFilter<T>... #290
Conversation
| return this.getAll(offset, limit, orders, null); | ||
| } | ||
|
|
There was a problem hiding this comment.
Nice PR @jrfinc, do we need this overload? And the corresponding null check in the other getAll?
Without this overload a call to getAll(offset, limit, orders) will just invoke the other with an empty array.
There was a problem hiding this comment.
You are right and we don't need it, but if removed it could break other DBManager implementations (in jPOS-EE RevisionManager for example, but could have overrides in other projects). It's an easy fix on the managers but still I thought it didn't hurt leaving it.
Regarding the null check, we need it for: Arrays.stream(filters) (I can either check for null like I did, or use Optionals. etc).
There was a problem hiding this comment.
I understand, I see there are two null checks, one for protecting against the nullity of the filters argument, and one for each element on it. I wonder if that is because of this call or for protection against other calls.
I was thinking that this call could be with an empty array, in case the inner null check is because of this null, but one can't be too cautious anyway right?
There was a problem hiding this comment.
Yes. I am thinking mainly on use cases of optional filters for a big list. It could happen that nulls get there.
I guess the devs using this call could (should?) sanitize the array and remove nulls beforehand, but it was easy to do here too.
No description provided.