Skip to content
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

Input validation to handle 502 Errors #87

Merged
Merged
Show file tree
Hide file tree
Changes from 14 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
1 change: 0 additions & 1 deletion imap_data_access/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@
"l1cb",
"l1d",
"l2",
"l2pre",
"l3",
"l3a",
"l3b",
Expand Down
32 changes: 32 additions & 0 deletions imap_data_access/file_validation.py
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,38 @@ def is_valid_date(input_date: str) -> bool:
except ValueError:
return False

@staticmethod
def is_valid_version(input_version: str) -> bool:
"""Check input version string is in valid format 'vXXX' or 'latest'.

Parameters
----------
input_version : str
Version to be checked.

Returns
-------
bool
Whether input version is valid or not.
"""
return input_version == "latest" or re.fullmatch(r"v\d{3}", input_version)

@staticmethod
def is_valid_repointing(input_repointing: str) -> bool:
"""Check input repointing string is in valid format 'repointingXXXXX'.

Parameters
----------
input_repointing : str
Repointing to be checked.

Returns
-------
bool
Whether input repointing is valid or not.
"""
return re.fullmatch(r"repoint\d{5}", str(input_repointing))

def construct_path(self) -> Path:
"""Construct valid path from class variables and data_dir.

Expand Down
51 changes: 51 additions & 0 deletions imap_data_access/io.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
from urllib.parse import urlencode

import imap_data_access
from imap_data_access import file_validation

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -99,6 +100,8 @@ def download(file_path: Union[Path, str]) -> Path:
return destination


# Too many branches error
# ruff: noqa: PLR0912
def query(
*,
instrument: Optional[str] = None,
Expand Down Expand Up @@ -159,6 +162,54 @@ def query(
"At least one query parameter must be provided. "
"Run 'query -h' for more information."
)
# Check instrument name
if instrument is not None and instrument not in imap_data_access.VALID_INSTRUMENTS:
raise ValueError(
"Not a valid instrument, please choose from "
+ ", ".join(imap_data_access.VALID_INSTRUMENTS)
)

# Check data-level
# do an if statement that checks that data_level was passed in,
# then check it against all options, l0, l1a, l1b, l2, l3 etc.
if data_level is not None and data_level not in imap_data_access.VALID_DATALEVELS:
raise ValueError(
"Not a valid data level, choose from "
+ ", ".join(imap_data_access.VALID_DATALEVELS)
)

# Check start-date
if start_date is not None and not file_validation.ScienceFilePath.is_valid_date(
start_date
):
raise ValueError("Not a valid start date, use format 'YYYYMMDD'.")

# Check end-date
if end_date is not None and not file_validation.ScienceFilePath.is_valid_date(
end_date
):
raise ValueError("Not a valid end date, use format 'YYYYMMDD'.")

# Check version make sure to include 'latest'
if version is not None and not file_validation.ScienceFilePath.is_valid_version(
version
):
raise ValueError("Not a valid version, use format 'vXXX'.")

# check repointing follows 'repoint00000' format
if (
repointing is not None
and not file_validation.ScienceFilePath.is_valid_repointing(repointing)
):
raise ValueError(
"Not a valid repointing, use format repointing<num>,"
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the ing and make it explicit how many numbers there has to be. (Update the actual code as well, I just noticed this in the test here)

Suggested change
"Not a valid repointing, use format repointing<num>,"
"Not a valid repointing, use format repointXXXXX,"

" where <num> is a 5 digit integer."
)

# check extension
if extension is not None and extension not in imap_data_access.VALID_FILE_EXTENSION:
raise ValueError("Not a valid extension, choose from ('pkts', 'cdf').")

url = f"{imap_data_access.config['DATA_ACCESS_URL']}"
url += f"/query?{urlencode(query_params)}"

Expand Down
58 changes: 56 additions & 2 deletions tests/test_io.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import json
import os
import re
import unittest
from io import BytesIO
from pathlib import Path
Expand Down Expand Up @@ -158,8 +159,8 @@ def test_download_already_exists(mock_urlopen: unittest.mock.MagicMock):
"descriptor": "test-description",
"start_date": "20100101",
"end_date": "20100102",
"repointing": "00001",
"version": "000",
"repointing": "repoint00001",
"version": "v000",
"extension": "pkts",
},
# Make sure not all query params are sent if they are missing
Expand Down Expand Up @@ -218,6 +219,59 @@ def test_query_bad_params(mock_urlopen: unittest.mock.MagicMock):
assert mock_urlopen.call_count == 0


@pytest.mark.parametrize(
("query_flag", "query_input", "expected_output"),
[
# All parameters should not send query
(
"instrument",
"badInput",
"Not a valid instrument, please choose from "
+ ", ".join(imap_data_access.VALID_INSTRUMENTS),
daralynnrhode marked this conversation as resolved.
Show resolved Hide resolved
),
(
"data_level",
"badInput",
"Not a valid data level, choose from "
+ ", ".join(imap_data_access.VALID_DATALEVELS),
),
("start_date", "badInput", "Not a valid start date, use format 'YYYYMMDD'."),
("end_date", "badInput", "Not a valid end date, use format 'YYYYMMDD'."),
(
"repointing",
"badInput",
"Not a valid repointing, use format repointing<num>, "
"where <num> is a 5 digit integer.",
),
("version", "badInput", "Not a valid version, use format 'vXXX'."),
(
"extension",
"badInput",
re.escape("Not a valid extension, choose from ('pkts', 'cdf')."),
),
],
)
def test_bad_query_input(query_flag, query_input, expected_output):
daralynnrhode marked this conversation as resolved.
Show resolved Hide resolved
"""Test a function call to query with correct params but bad values.

Ensures correct error message is returned.

Parameters
----------
query_flag : str
correct query flag.
query_input : str
incorrect query input.
expected_output : str
Output error expected to be given.
"""
kwargs = {query_flag: query_input}

# Check if the ValueError is raised and contains the correct message
with pytest.raises(ValueError, match=expected_output):
imap_data_access.query(**kwargs)


def test_upload_no_file(mock_urlopen: unittest.mock.MagicMock):
"""Test a call to the upload API that has no filename supplied.

Expand Down
Loading