TECH-408: Add pagination to GET /report endpoint (BE) #18
Conversation
…created endpoint for products count
* Fix directory structure for server * Fix type in error message * Fix that yarn/npm installs run after source files are copied
| const limit = FiltersValidation.validateFilter(this.req.query.limit, Infinity); | ||
| const offset = FiltersValidation.validateFilter(this.req.query.offset, 0); |
There was a problem hiding this comment.
What is behavior when offset exceeds number of entries in database?
Should we pass count and nextOffset as a part of the payload or is it redundant?
There was a problem hiding this comment.
From what I tested doesn't show results
There was a problem hiding this comment.
We agreed with Karolina that she will check if there is more records to get, so I think it's not necessary. What do you think @dborowiecki ?
| static validateFilter(filter, defaultValue) { | ||
| return filter === undefined || !filter.match(/^\d+$/) | ||
| ? defaultValue | ||
| : filter; | ||
| } |
There was a problem hiding this comment.
I don't think we should allow Infinity on regular calls.
Can you add check for max limit (either configurable or fixed on 1k) just for safety measures?
There was a problem hiding this comment.
Fixed, please confirm if the solution meets your expectations @dborowiecki
* TECH-453: Added compatibility and overallCompatibility fields to the product object * TECH-453: Added check in divide to not divide by 0
…created endpoint for products count
dborowiecki
left a comment
dborowiecki
left a comment
There was a problem hiding this comment.
I don't think we should execute "generic" pagination inside of requests directly. I'd suggest to implement middleware that can be attached to many requests instead.
|
|
||
| static maxLimitCheck(limit) { | ||
| if (limit > this.MAX_REPORTS_IN_REQUEST) { | ||
| console.log(`Provided limit parameter: ${limit} exceeded maximum limit: ${this.MAX_REPORTS_IN_REQUEST} and has been set to: ${this.MAX_REPORTS_IN_REQUEST}`); |
There was a problem hiding this comment.
Please change to warning
There was a problem hiding this comment.
changed
|
|
||
| let validatedLimit; | ||
| validatedLimit = FiltersValidation.isFilterValid(limit) ? limit : null; | ||
| validatedLimit = FiltersValidation.maxLimitCheck(validatedLimit); |
There was a problem hiding this comment.
What is behavior in case of validatedLimit===null?
Also I think we can refactor this validation. I think there's no need to have single function for offset and limit. In addition FiltersValidation can be renamed to be more descriptive (those filters are specific to pagination). Also with that much code I don't like putting this lines inside of request handler directly. It can be tricky to manage with more handlers. Can we create separate middleware function that would be attached to routing for endpoints with pagination? This way we can ensure consistency.
There was a problem hiding this comment.
I added middleware to handle pagination filters, please let me know if that is what you meant @dborowiecki
| ReportModel.aggregate(getLatestReportPipeline(filters)).exec(callback); | ||
| const aggregation = ReportModel.aggregate(getLatestReportPipeline(filters)); | ||
|
|
||
| if (filters.validatedOffset !== null) { |
There was a problem hiding this comment.
Can we change attribute names to be consistent with db naming conventions? In the "repository" we don't have information if offset is or is not valid, also it can be used in different context depending on business logic. I think skip and limit could be better names
There was a problem hiding this comment.
changed to offset and limit
| @@ -0,0 +1,16 @@ | |||
| /* eslint-disable no-console */ | |||
| module.exports = class FiltersValidation { | |||
There was a problem hiding this comment.
Please add information regarding pagination to class name. If we decide to use middleware then function should also provide name
Created handling for offset and limit parameters.
Added endpoint to get products count
Description
Acceptance criteria:
TICKET: https://govstack-global.atlassian.net/browse/TECH-408