Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 0 additions & 16 deletions .flake8

This file was deleted.

4 changes: 2 additions & 2 deletions .github/workflows/ci-build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ jobs:
version: "0.11.21"

- name: Run linting
run: uv run flake8 datagateway_api test util
run: uv run ruff check

formatting:
runs-on: ubuntu-24.04
Expand All @@ -157,7 +157,7 @@ jobs:
with:
version: "0.11.21"
- name: Install dependencies and check formatting
run: uv run black --check datagateway_api test util
run: uv run ruff format

generator-script-testing:
runs-on: ubuntu-24.04
Expand Down
6 changes: 3 additions & 3 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ repos:
- id: trailing-whitespace
- repo: local
hooks:
- id: black
name: black
entry: uv run black
- id: formatter
name: ruff
entry: uv run ruff format
language: system
types: [python]
11 changes: 5 additions & 6 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -50,24 +50,23 @@ uv add [PACKAGE-NAME]

When developing new features for the API, the following tools are used for code quality and testing:

- [Black](https://black.readthedocs.io/en/stable/) - Code formatting
- [flake8](https://flake8.pycqa.org/en/latest/) - Code linting (with additional plugins configured in `.flake8`)
- [Ruff](https://docs.astral.sh/ruff/) - Code formatting and linting
- [pytest](https://docs.pytest.org/en/stable/) - Testing framework

All these tools are included as development dependencies in uv and can be run via uv or directly.

### Running Development Tools

Format code with Black:
Format code with ruff:

```bash
uv run black datagateway_api test util
uv run ruff format
```

Lint code with flake8:
Lint code with ruff:

```bash
uv run flake8 datagateway_api test util
uv run ruff check
```

Run unit tests:
Expand Down
1 change: 0 additions & 1 deletion datagateway_api/auth/session_bearer.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import logging


from fastapi import Request
from fastapi.security import HTTPAuthorizationCredentials, HTTPBearer

Expand Down
2 changes: 1 addition & 1 deletion datagateway_api/common/base_query_filter_factory.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ class QueryFilterFactory(ABC):
@abstractstaticmethod
def get_query_filter(request_filter, entity_name=None): # noqa: B902, N805
"""
Given a filter, return a matching Query filter object
Given a filter, return a matching Query filter object.

:param request_filter: The filter to create the QueryFilter for
:type request_filter: :class:`dict`
Expand Down
33 changes: 16 additions & 17 deletions datagateway_api/common/config.py
Original file line number Diff line number Diff line change
@@ -1,23 +1,22 @@
from functools import cached_property
import logging
from pathlib import Path
import sys
from typing import Annotated, Optional, Self
from functools import cached_property
from pathlib import Path
from typing import Annotated, Self

import yaml
from pydantic import (
AfterValidator,
BaseModel,
computed_field,
Field,
model_validator,
SecretStr,
StrictBool,
StrictInt,
StrictStr,
ValidationError,
computed_field,
model_validator,
)
import yaml


log = logging.getLogger()

Expand Down Expand Up @@ -73,7 +72,7 @@ class DataGatewayAPI(BaseModel):
extension: DataGatewayAPIExtension
icat_check_cert: StrictBool
icat_url: StrictStr
use_reader_for_performance: Optional[UseReaderForPerformance] = None
use_reader_for_performance: UseReaderForPerformance | None = None

def __getitem__(self, item):
return getattr(self, item)
Expand Down Expand Up @@ -131,14 +130,14 @@ class APIConfig(BaseModel):
API startup so any missing options will be caught quickly.
"""

datagateway_api: Optional[DataGatewayAPI] = None
reload: Optional[StrictBool] = None
host: Optional[StrictStr] = None
port: Optional[StrictInt] = None
search_api: Optional[SearchAPI] = None
test_mechanism: Optional[StrictStr] = None
datagateway_api: DataGatewayAPI | None = None
reload: StrictBool | None = None
host: StrictStr | None = None
port: StrictInt | None = None
search_api: SearchAPI | None = None
test_mechanism: StrictStr | None = None
url_prefix: DataGatewayAPIExtension
test_user_credentials: Optional[TestUserCredentials] = None
test_user_credentials: TestUserCredentials | None = None

def __getitem__(self, item):
return getattr(self, item)
Expand Down Expand Up @@ -172,7 +171,7 @@ def load(cls, path=None):
)

return cls(**data)
except (IOError, ValidationError) as error:
except (OSError, ValidationError) as error:
sys.exit(f"An error occurred while trying to load the config data: {error}")

@staticmethod
Expand Down Expand Up @@ -204,6 +203,6 @@ def _validate_api_extensions(self) -> Self:


class Config:
"""Class containing config as a class variable so it can mocked during testing"""
"""Class containing config as a class variable so it can mocked during testing."""

config = APIConfig.load()
4 changes: 1 addition & 3 deletions datagateway_api/common/date_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ def is_str_a_date(potential_date):
:type potential_date: :class:`str`
:return: Boolean to signify whether `potential_date` is a date or not
"""

text = potential_date.strip()

# Reject if the string is just digits (like "5" or "20200101")
Expand Down Expand Up @@ -58,7 +57,6 @@ def str_to_datetime_object(data):
:return: Date converted into a :class:`datetime` object
:raises BadRequestError: If there is an issue with the date format
"""

try:
datetime_obj = helper.parse_attr_string(data, "Date")
except ValueError as e:
Expand All @@ -69,7 +67,7 @@ def str_to_datetime_object(data):
@staticmethod
def datetime_object_to_str(datetime_obj):
"""
Convert a datetime object to a string so it can be outputted in JSON
Convert a datetime object to a string so it can be outputted in JSON.

:param datetime_obj: Datetime object from data from an ICAT entity
:type datetime_obj: :class:`datetime.datetime`
Expand Down
15 changes: 5 additions & 10 deletions datagateway_api/common/filter_order_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
log = logging.getLogger()


class FilterOrderHandler(object):
class FilterOrderHandler:
"""
The FilterOrderHandler takes in filters, sorts them according to the order of
operations, then applies them.
Expand All @@ -36,9 +36,7 @@ def remove_filter(self, query_filter):
self.filters.remove(query_filter)

def sort_filters(self):
"""
Sorts the filters according to the order of operations
"""
"""Sorts the filters according to the order of operations."""
self.filters.sort(key=lambda x: x.precedence)

def apply_filters(self, query):
Expand Down Expand Up @@ -84,7 +82,6 @@ def add_icat_relations_for_non_related_fields_of_panosc_related_entities(
:param panosc_entity_name: A PaNOSC entity name e.g. "Dataset"
:type panosc_entity_name: :class:`str`
"""

python_icat_include_filter = None
icat_relations = []
for filter_ in self.filters:
Expand Down Expand Up @@ -120,12 +117,11 @@ def add_icat_relations_for_panosc_non_related_fields(
):
"""
Retrieve ICAT relations and create a `PythonICATIncludeFilter` for these ICAT
relations
relations.

:param panosc_entity_name: A PaNOSC entity name e.g. "Dataset"
:type panosc_entity_name: :class:`str`
"""

icat_relations = mappings.get_icat_relations_for_panosc_non_related_fields(
panosc_entity_name,
)
Expand All @@ -138,7 +134,7 @@ def add_icat_relations_for_panosc_non_related_fields(
def merge_python_icat_limit_skip_filters(self):
"""
When there are both limit and skip filters in a request, merge them into the
limit filter and remove the skip filter from the instance
limit filter and remove the skip filter from the instance.
"""
log.info("Merging a PythonICATSkipFilter and PythonICATLimitFilter together")
skip_filter = None
Expand Down Expand Up @@ -177,14 +173,13 @@ def manage_icat_filters(self, filters, query):
Utility function to call other functions in this class, used to manage filters
when using the Python ICAT. These steps are the same with the different
types of requests that utilise filters, therefore this function helps to reduce
code duplication
code duplication.

:param filters: The list of filters that will be applied to the query
:type filters: List of specific implementations :class:`QueryFilter`
:param query: ICAT query which will fetch the data at a later stage
:type query: :class:`icat.query.Query`
"""

self.add_filters(filters)
self.merge_python_icat_limit_skip_filters()
self.clear_python_icat_order_filters()
Expand Down
6 changes: 3 additions & 3 deletions datagateway_api/common/filters.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
from abc import ABC, abstractmethod
import logging
from abc import ABC, abstractmethod

from datagateway_api.common.exceptions import BadRequestError, FilterError

Expand All @@ -20,7 +20,7 @@ def apply_filter(self, query):
class WhereFilter(QueryFilter):
precedence = 1

def __init__(self, field, value, operation):
def __init__(self, _field, value, operation):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure how I feel about this. I assume since the first argument is unused, it wants it to be marked with an underscore? But now the arguments for WhereFilter are different from its subclasses PythonICATWhereFilter and SearchAPIWhereFilter. And we never directly instantiate WhereFilter as far as I can see (one place we import PythonICATWhereFilter as WhereFilter just to be extra confusing. And SearchAPIWhereFilter is a subclass of PythonICATWhereFilter not WhereFilter. So why does WhereFilter even exist?

Similarly, the other contents of ral-facilities/datagateway-api/datagateway_api/search_api/filters.py all import from ral-facilities/datagateway-api/datagateway_api/datagateway_api/icat/filters.py not ral-facilities/datagateway-api/datagateway_api/common/filters.py. Can we just remove this entire file and migrate the shared functionality to ral-facilities/datagateway-api/datagateway_api/datagateway_api/icat/filters.py classes? Appreciate that's scope creep so should be in another PR. Maybe I'm missing something subtle too.

In the case of this line I'd leave it and just ignore the QA rather than changing it if it's going to get nuked later, but either way it's not a big deal. The important bit to me is: why does this file exist?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This might be a remnant of the database backend potentially

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll create an issue for this

# The field is set to None as a precaution but this should be set
# when initialising Python ICAT
self.field = None
Expand All @@ -33,7 +33,7 @@ def __init__(self, field, value, operation):
f"When using the {self.operation} operation for a WHERE filter, the"
f" values must be in a list format e.g. [1, 2]",
)
if self.operation == "between" and len(self.value) != 2:
if self.operation == "between" and len(self.value) != 2: # noqa: PLR2004
raise BadRequestError(
"When using the 'between' operation for a WHERE filter, the list"
"must contain two values e.g. [1, 2]",
Expand Down
29 changes: 14 additions & 15 deletions datagateway_api/common/helpers.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
from datetime import datetime
from functools import wraps
import json
import logging
from datetime import datetime
from functools import wraps

import requests
from fastapi import Request
from pydantic import ValidationError
import requests


from datagateway_api.common.date_handler import DateHandler
from datagateway_api.common.exceptions import (
Expand All @@ -25,7 +24,7 @@ def queries_records(method):
Decorator for endpoint resources that search for a record in a table
:param method: The method for the endpoint
:return: Will return a 404, "No such record" if a MissingRecordError is caught
:return: Will return a 400, "Error message" if other expected errors are caught
:return: Will return a 400, "Error message" if other expected errors are caught.
"""

@wraps(method)
Expand All @@ -52,7 +51,7 @@ def get_session_id_from_auth_header(request: Request):
"""
Gets the sessionID from the Authorization header of a request
:request Request: FastAPI Request object containing the incoming headers
:return: String: SessionID
:return: String: SessionID.
"""
log.info("Getting session Id from auth header")

Expand All @@ -63,7 +62,7 @@ def get_session_id_from_auth_header(request: Request):

auth_header = auth_header_value.split(" ")

if len(auth_header) != 2 or auth_header[0] != "Bearer":
if len(auth_header) != 2 or auth_header[0] != "Bearer": # noqa: PLR2004
raise AuthenticationError(
f"Could not authenticate consumer with auth header {auth_header}",
)
Expand All @@ -75,7 +74,7 @@ def is_valid_json(string):
"""
Determines if a string is valid JSON
:param string: The string to be tested
:return: boolean representing if the string is valid JSON
:return: boolean representing if the string is valid JSON.
"""
try:
json.loads(string)
Expand All @@ -89,7 +88,7 @@ def is_valid_json(string):
def get_filters_from_query_string(request: Request, api_type, entity_name=None):
"""
Gets a list of filters from the query_strings arg,value pairs, and returns a list of
QueryFilter Objects
QueryFilter Objects.

:request Request: FastAPI Request object containing the incoming headers
:param api_type: Type of API this function is being used for i.e. DataGateway API or
Expand All @@ -102,11 +101,11 @@ def get_filters_from_query_string(request: Request, api_type, entity_name=None):
:return: The list of filters
"""
if api_type == "search_api":
from datagateway_api.search_api.query_filter_factory import (
from datagateway_api.search_api.query_filter_factory import ( # noqa: PLC0415
SearchAPIQueryFilterFactory as QueryFilterFactory,
)
elif api_type == "datagateway_api":
from datagateway_api.datagateway_api.query_filter_factory import (
from datagateway_api.datagateway_api.query_filter_factory import ( # noqa: PLC0415
Comment on lines 103 to +108

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't like ignoring these, as I don't like doing conditional imports mid-function. I also don't like the fact that a common helper has to import from two of our api specific modules at all.

Given we pass api_type to distinguish between the two cases, would it be acceptable (and to my mind simpler) to just pass either SearchAPIQueryFilterFactory or DataGatewayAPIQueryFilterFactory as an argument of type class, and call it directly below?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll create an issue for this

DataGatewayAPIQueryFilterFactory as QueryFilterFactory,
)
else:
Expand Down Expand Up @@ -135,7 +134,7 @@ def get_icat_properties(icat_url, icat_check_cert):
"""
ICAT properties can be retrieved using Python ICAT's client object, however this
requires the client object to be authenticated which may not always be the case
when requesting these properties, hence a HTTP request is sent as an alternative
when requesting these properties, hence a HTTP request is sent as an alternative.
"""
properties_url = f"{icat_url}/icat/properties"
r = requests.request("GET", properties_url, verify=icat_check_cert)
Expand All @@ -147,7 +146,7 @@ def get_icat_properties(icat_url, icat_check_cert):
def map_distinct_attributes_to_results(distinct_attributes, query_result):
"""
Maps the attribute names from a distinct filter onto the results given by the result
of a query
of a query.

When selecting multiple (but not all) attributes in a database query, the results
are returned in a list and not mapped to an entity object. This means the 'normal'
Expand All @@ -169,7 +168,7 @@ def map_distinct_attributes_to_results(distinct_attributes, query_result):
split_attr_name = attr_name.split(".")

if isinstance(data, datetime):
data = DateHandler.datetime_object_to_str(data)
data = DateHandler.datetime_object_to_str(data) # noqa: PLW2901

# Attribute name is from the 'origin' entity (i.e. not a related entity)
if len(split_attr_name) == 1:
Expand All @@ -184,7 +183,7 @@ def map_distinct_attributes_to_results(distinct_attributes, query_result):
def map_nested_attrs(nested_dict, split_attr_name, query_data):
"""
A function that can be called recursively to map attributes from related
entities to the associated data
entities to the associated data.

:param nested_dict: Dictionary to insert data into
:type nested_dict: :class:`dict`
Expand Down
Loading
Loading