-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[#59391] rework file links to paginated collection #17230
[#59391] rework file links to paginated collection #17230
Conversation
aebba26
to
b13c7d0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes look good to me. I am assuming the pagination change is considered a non breaking API change and thus can be included without any issues to consumers.
🎉
@brunopagno This is an important point. My take on it would be, that usage of it is in the most cases the same. If you do not define a pagesize, you get a standard back. only if you have more then 30 file links in one collection, you suddenly won't get all back. This could be considered breaking, but the solution for it is quite simple and needs only adding a |
- https://community.openproject.org/wp/59391 - files tab counter now only calls for file links of wp with page size 0 - file links endpoint returns paginated collections - frontend requests file links with page size -1 (INFINITE) - file sync is still executed for ALL file links of a work package, not only the requested page - this is practically no change for the product, as we do not fetch single pages - but fetching only the total numbers with page size 0 now does not trigger a sync
- avoid sql injection - file link collection is returned ordered by id asc
9ca4a05
to
e86d5bd
Compare
modules/storages/lib/api/v3/file_links/join_origin_status_to_file_links_relation.rb
Show resolved
Hide resolved
modules/storages/lib/api/v3/file_links/join_origin_status_to_file_links_relation.rb
Outdated
Show resolved
Hide resolved
@Kharonus the behaviour change could be considered breaking. Do we have any previous events similar to this to take into account? Would be nice if the decision was made for us 😅 I personally do not feel the change is a big deal, but if any consumer depends on this behaviour they could be annoyed, even if the fix on the client side is simple. |
Well, TBH in the past we did not really bothered with breaking changes to the API. We renamed properties, changed JSON representations, and still released them without a deprecation round. For this thing here a deprecation round would be adding this behavior behind some kind of feature flag. It is possible, but it would raise of course again the question, if the change is breaking enough. It should not happen, that all custom clients suddenly break. And for those, where big collections are an issue, the fix is rather simple. |
@Kharonus sounds good enough to me. Thanks for the context there 🙇 |
Ticket
OP#59391
What are you trying to accomplish?
What approach did you choose and why?