Implement Limit and Skip filters

[VKTB]

This PR will close #262
This PR will close #263

Description

The Limit (SearchAPILimitFilter) and Skip (SearchAPISkipFilter) filters are already implemented. To reduce duplication, they inherit from PythonICATLimitFilter and PythonICATSkipFilter. The PythonICATSkipFilter class connects to ICAT to get the maxEntities value applying the Skip filter to the ICAT query so this PR refactors the PythonICATSkipFilter class to take a client_use value in order to allow for client configuration for both Search API and DataGateway API.

It also adds tests which test the SearchAPILimitFilter and SearchAPISkipFilter classes.

Testing Instructions

• Review code
• Check GitHub Actions build
• If icatdb Generator Script Consistency Test CI job fails, is this because of a deliberate change made to the script to change generated data (which isn't actually a problem) or is here an underlying issue with the changes made?
• Review changes to test coverage
• Does this change mean a new patch, minor or major version should be made? If so, does one of the commit messages feature fix:, feat: or BREAKING CHANGE: so a release is automatically made via GitHub Actions upon merge?

Agile Board Tracking

Connect to #262
Connect to #263

[VKTB]

The tests inside the TestSearchAPILimitFilter and TestSearchAPISkipFilter classes are based off the test that are already in TestICATLimitFilter and TestICATSkipFilter. However, I noticed that the test_valid_limit_value and test_valid_skip_value tests are integration rather than unit tests. This is because they connect to ICAT and as a result take ~705ms and ~751ms respectively to complete. We can keep these as integration tests and separate them together with other integration tests for when we plan to do this in future. But if we want to unit test the SearchAPILimitFilter and SearchAPISkipFilter classes, this is how we should do it (they take ~3ms to complete):

TestSearchAPILimitFilter:

@pytest.mark.parametrize(
    "limit_value",
    [
        pytest.param(10, id="typical"),
        pytest.param(0, id="low boundary"),
        pytest.param(9999, id="high boundary"),
    ],
)
@patch.object(Query, "setLimit")
def test_valid_limit_value(self, query_mock, limit_value):
    test_filter = SearchAPILimitFilter(limit_value)
    test_filter.apply_filter(query_mock)

    query_mock.setLimit.assert_called_once_with((0, limit_value))

TestSearchAPISkipFilter:

@pytest.mark.parametrize(
    "skip_value", [pytest.param(10, id="typical"), pytest.param(0, id="boundary")],
)
@patch("datagateway_api.src.datagateway_api.icat.filters.get_icat_properties", return_value={"maxEntities":5000})
@patch.object(Query, "setLimit")
def test_valid_skip_value(self, query_mock, get_icat_properties_mock, skip_value):
    test_filter = PythonICATSkipFilter(skip_value)
    test_filter.apply_filter(query_mock)
    limit_value = get_icat_properties_mock()["maxEntities"]

    query_mock.setLimit.assert_called_once_with((skip_value, limit_value))

[codecov]

Codecov Report

Merging #295 (7310707) into master (b3b797c) will increase coverage by 0.65%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #295      +/-   ##
==========================================
+ Coverage   86.23%   86.89%   +0.65%     
==========================================
  Files          37       37              
  Lines        2667     2670       +3     
  Branches      216      217       +1     
==========================================
+ Hits         2300     2320      +20     
+ Misses        341      324      -17     
  Partials       26       26              

Impacted Files	Coverage Δ
...atagateway_api/src/datagateway_api/icat/filters.py	99.37% <100.00%> (+0.01%)	⬆️
datagateway_api/src/search_api/filters.py	80.95% <100.00%> (+80.95%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3b797c...7310707. Read the comment docs.

[MRichards99]

Implementation looks good, I think it might be benefical to change client_use to a name more focused on filters.

As for the tests, I think let's keep them as they are now (i.e. lets focus on the search API deadline) and refactor/group the tests when there is time. I think I blur the line between unit and integration tests quite a lot (mainly for speed to write the tests) but this might not be the optimal way to test. I would like this repo to have a set of unit tests which can be run without a working ICAT instance attached to it but I think the higher priority is delivering a solution for the search API. I think the mixture of tests that are in this repo do give good coverage though, and are ultimately better than the small amount of testing that used to be present in this repo. TL;DR - the testing serves its purpose to know we're not breaking stuff, but could do with a refactor to make adhere to 'proper' developer practices.

[VKTB]

Sounds good about the tests :)

[MRichards99]

Approved because I know you can fix merge conflicts :)

[MRichards99]

I forgot that adding a commit will dismiss my review, so maybe second time lucky