adapter.http: implement the pagination #35
Conversation
jkhsjdhjs
left a comment
jkhsjdhjs
left a comment
There was a problem hiding this comment.
Looks pretty good so far, but I still have some comments on this one. Also, slicing isn't currently implemented for AAS, can you add this?
jkhsjdhjs
left a comment
jkhsjdhjs
left a comment
There was a problem hiding this comment.
Some things I just noticed while testing the implementation:
- In case less than
cursoritems are returned, thecursoris returned as 10 anyway. We should discuss whether a cursor should be returned in these cases as all, as there should be a way to indicate to the client that there aren't any further objects it can request. Maybe the cursor should be omitted if there aren't any further items to request, or set to -1 or something else? - Invalid
cursor/limitvalues (e.g. non-numeric values) are currently silently ignored. We should return a 400 instead to make the client aware of its errors. - Negative numbers for
cursororlimitcurrently cause an internal server error. We should instead also return 400 for negative values, as they are equally invalid. - XML
resultresponses are wrapped in aresponseobject, which they shouldn't IMO.
|
I'm not sure if I agree with your first point. While it isn't optimal I think it is quite obvious to the user when there are no more objects to request. But it would still be nice to know where you ended with your request, maybe you want to POST an element between the requests and then continue with the cursor of the old request. I'm not sure if cursor information is completly useless even if it lies outside the existing number of elements. But I'm definitely open to changing it, I'm just not sure how. The rest are alle good points I'll implement them, thanks 😄 |
basyx/aas/adapter/http.py
Outdated
| limit_str= request.args.get('limit', type=str, default="10") | ||
| cursor_str= request.args.get('cursor', type=str, default="0") | ||
| if not limit_str.isdigit() or not cursor_str.isdigit(): | ||
| raise BadRequest("Cursor and limit must be positive integers!") |
There was a problem hiding this comment.
While the code is probably the shortest possible, I don't like that limit and cursor are retrieved twice, and also have the default values specified in two separate lines. What about the following instead:
limit = request.args.get('limit', default="10")
cursor = request.args.get('cursor', default="0")
try:
limit, cursor = int(limit), int(cursor)
if limit < 0 or cursor < 0:
raise ValueError
except ValueError:
raise BadRequest("Cursor and limit must be positive integers!")
Adds pagination via a cursor and limit functionality.