Skip to content

fix: filters and limits in GET/jobs to get consistent results #1866

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

Merged
merged 3 commits into from
May 20, 2025

Conversation

despadam
Copy link
Member

@despadam despadam commented Apr 29, 2025

Description

GET/jobs and fullQuery/jobs need to apply CASL rules after retrieving the jobs because these rules depend on each job configuration. This was causing inconsistencies in the results of endpoints because the filters and limits of the search where applied first, and the CASL rules afterwards. These rules would exclude jobs that the user does not have access to, causing discrepancies if the results where being paginated.

For example, when applying limits like so: { "where": { "type": "archive" }, "limits": { "skip": 0, "limit": 10 } } one would expect the first page to contain 10 jobs, and similarly the rest of the pages, until we reach the end of the list. However, since the execution order was 1. where + limits, 2. CASL rules, there were cases where a page would contain less than 10 jobs even if the remaining number of jobs was more than that.

Motivation

Return consistent number of jobs per page, when pagination is applied.

Changes:

Instead of using findAll and findquery from jobs.service, introduce findByFilters and applyFilterLimits, so that they can be applied separarately and the execution order can change to 1. where, 2. CASL rules, 3. limits. This way the pagination will be done after all filtering has been applied. Now GET/jobs, fullQuery/jobs as well as fullFacet/jobs have been updated to always return consistent results.

Tests included

  • Included for each change/fix?
  • Passing?

Documentation

  • swagger documentation updated (required for API changes)
  • official documentation updated

official documentation info

@despadam despadam self-assigned this Apr 29, 2025
@despadam despadam marked this pull request as ready for review May 5, 2025 10:12
@despadam despadam requested review from sbliven, nitrosx and sofyalaski May 5, 2025 10:16
Copy link
Member

@sofyalaski sofyalaski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! The only thing maybe is that we should also provide an example for SwaggerUI similar as we do in all other methods?

@despadam despadam merged commit df94798 into master May 20, 2025
14 checks passed
@despadam despadam deleted the fix-jobs-get-endpoints branch May 20, 2025 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants