My company has a REST API, and one design issue that has come up a few times is that we use null to indicate "get everything".
For example /api/books?genre=romance
will get all romance books, but /api/books
will return all books from all genres.
The current technique used in the code is something like this
public ResponseEntity getBooks(@RequestParam(required = false) String genre) {
List<BookDto> bookDtos = new ArrayList<>();
List<Book> allBooks = getAllBooks();
for (Book book : allBooks) {
// This if condition is what I take issue with
if (genre == null || book.genre.equals(genre)) {
bookDtos.add(book.convertToDto());
}
}
return bookDtos;
}
I think it is bad practice to use null here to convey the meaning of get everything. Is this correct?
My current solution is to distinguish between these two cases at the very top level like so:
public ResponseEntity getBooks(@RequestParam(required = false) String genre) {
if (genre == null) {
return getAllBooks();
} else {
return getBooks(genre);
}
}
And then a few supporting functions that funnel all of the logic into the same place by massaging the inputs a little bit.
// This is the core code, that takes a list of genres and returns all books from all those genres.
public Collection<BookDto> getBooks(List<String> genre) {
// And here is the code that will actually get the books from the database.
// ...
// ...
}
// This is a supporting function, that finds all genres and funnels that into the above function.
public Collection<BookDto> getAllBooks() {
List<String> allGenres = getAllGenres()
return getBooks(allGenres);
}
// This is a supporting function that builds a list from a single genre, to again funnel into the 1st function.
public Collection<BookDto> getBooks(String genre) {
List<String> genres = new ArrayList<>();
genres.add(yearCode);
return getBooks(genres);
}
The idea here is to have a single place where the work actually takes place and the database is accessed, but to have multiple ways to access it. And more importantly to remove the idea of null == all
in the codebase, aside from the very top level where the execution starts after the user requests /api/books
.
It's always felt natural to me but I've not seen it used by anyone else (I've only worked at a few places, so this could just be inexperience on my part). Is the proposed design good? Is there a standard way of solving this problem?
3 Answers 3
I personally think it's fine, but it helps to turn your thinking around from a "selector" to a "filter". Then, the default is to return all books, and you selectively filter down the set you're going to eventually respond with.
(Note I haven't written Java in a long time, so grain of salt here,) I might specify a BookFilter
object which can incrementally have fields set from the parameters, and then have BookDTO.filterBooks
. Then, your request method becomes:
public ResponseEntity getBooks(@RequestParam(required = false) String genre) {
BookFilter filter = new BookFilter();
if (genre != null) {
filter.setGenre(genre);
}
return filterBooks(filter);
}
I know it's not directly applicable (your DTO probably doesn't work this way, based on what you described above), but I think what you're doing with the null check in your controller is pretty much the same thing with the tools you've got.
You might consider trying to refactor your DTO so it can work with filters however, because eventually you may want ?genre=scifi&published=1970+&publisher=penguin
etc. etc., at which point it'll be a lot easier to manage a single filter object than a bunch of explicit queries.
Instead of allowing the API user to retrieve all items, which could be a huge quantity of data to transmit, I would force paging, so that only the first n items were returned and the URL must be modified to get more.
e.g. /api/books?page=2
...or some other query
This API pattern -- null means everything -- is actually quite old. Think SQL:
select * from books
vs
select * from books where genre in <genre-list>
This also gives us a hint on implementation: the predicate, if present, is an additional filter on the list.
So this is really to support @Jon's answer above. But I do have an additional comment on the implementation. Where are the Books
stored? Database? In that case getAllBooks()
and then filtering can be needlessly expensive. Consider pushing the filter to the storage.
The same filtering pattern can be applied to constructing SQL
or whatever mechanism you are using for fetching the books from the system of records.
-
2\$\begingroup\$ I don't know,
genre
isn'tnull
in your first request, it's just not a specified criteria ;select * from books
!=select * from books where genre is null
\$\endgroup\$Aaron– Aaron2017年04月11日 12:31:11 +00:00Commented Apr 11, 2017 at 12:31 -
\$\begingroup\$ My analogy is to the actual API
/api/books
and/api/books/genre=something
. OP writes that genre is null in the former case but in the url it is a non-specified criteria \$\endgroup\$Miserable Variable– Miserable Variable2017年04月12日 02:22:26 +00:00Commented Apr 12, 2017 at 2:22 -
\$\begingroup\$ Yes, in the URI it's unspecified, which translates to a
null
value for theRequestParam
. I agree your analogy supports well the use of unspecified parameters in the URL, but I think OP's problem was with his servlet's code where he feels likenull
is a strange value to represent<genre-list>
. \$\endgroup\$Aaron– Aaron2017年04月12日 09:31:08 +00:00Commented Apr 12, 2017 at 9:31
/api/books
will return all books from all genres." I am pretty sure it's a common pattern. I see it in every book on rest api. C# exemple of this patern used in a tutorial \$\endgroup\$