Skip to content

[#600] prevent case-sensitive testing with BETWEEN where infeasible #818

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.

Sign up for GitHub

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

Open
d-w-moore wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[#600] prevent case-sensitive testing with BETWEEN where infeasible #818

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
74 changes: 52 additions & 22 deletions irods/test/query_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,16 +44,49 @@


class TestQuery(unittest.TestCase):

_csq = None

Check failure on line 48 in irods/test/query_test.py

View workflow job for this annotation

GitHub Actions / ruff-lint / ruff-format

Ruff format 

Improper formatting

Iterate_to_exhaust_statement_table = range(IRODS_STATEMENT_TABLE_SIZE + 1)

More_than_one_batch = 2 * MAX_SQL_ROWS # may need to increase if PRC default page
# size is increased beyond 500

register_resc = ""

@property
def case_sensitive_testing_feasible(self):
"""
Return result of the probe into whether lexical order of query columns has been found compatible with
case sensitive operators such as >, <, between. (MySQL can fail in the default DB configuration,
whereas PostgreSQL does not).
"""

Check failure on line 63 in irods/test/query_test.py

View workflow job for this annotation

GitHub Actions / ruff-lint / ruff-check

Ruff D205 

D205: 1 blank line required between summary line and description [pydocstyle:missing-blank-line-after-summary]
return self._csq in (True,)

Check failure on line 64 in irods/test/query_test.py

View workflow job for this annotation

GitHub Actions / ruff-lint / ruff-check

Ruff FURB171 

FURB171: Membership test against single-item container [refurb:single-item-membership-test]

@staticmethod
def _query_columns_case_sensitive_by_default(ses):
home = helpers.home_collection(ses)
data_objs = []
try:
for x in ('a', 'b', 'A', 'B'):
data_objs.append(ses.data_objects.create(f'{home}/{x}'))

Check failure on line 72 in irods/test/query_test.py

View workflow job for this annotation

GitHub Actions / ruff-lint / ruff-check

Ruff PERF401 

PERF401: Use `list.extend` to create a transformed list [Perflint:manual-list-comprehension]
lower = { r[DataObject.name] for r in ses.query(DataObject.name).filter(DataObject.name >= 'a', DataObject.name <= 'b') }

Check failure on line 73 in irods/test/query_test.py

View workflow job for this annotation

GitHub Actions / ruff-lint / ruff-check

Ruff E501 

E501: Line too long (133 > 120) [pycodestyle:line-too-long]
if {'A','B'} & lower:
return False
upper = { r[DataObject.name] for r in ses.query(DataObject.name).filter(DataObject.name >= 'A', DataObject.name <= 'B') }

Check failure on line 76 in irods/test/query_test.py

View workflow job for this annotation

GitHub Actions / ruff-lint / ruff-check

Ruff E501 

E501: Line too long (133 > 120) [pycodestyle:line-too-long]
if {'a','b'} & upper:
return False
return True

Check failure on line 79 in irods/test/query_test.py

View workflow job for this annotation

GitHub Actions / ruff-lint / ruff-check

Ruff SIM103 

SIM103: Return the condition `not {'a', 'b'} & upper` directly [flake8-simplify:needless-bool]
finally:
for d in data_objs:
d.unlink(force=True)

Check failure on line 82 in irods/test/query_test.py

View workflow job for this annotation

GitHub Actions / ruff-lint / ruff-format

Ruff format 

Improper formatting

@classmethod
def setUpClass(cls):
with helpers.make_session() as sess:
if cls._csq is None:
cls._csq = cls._query_columns_case_sensitive_by_default(sess)
Copy link
Copy Markdown
Contributor

@korydraughn korydraughn Apr 27, 2026

Choose a reason for hiding this comment

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

Why does case_sensitive_testing_feasible use a tuple to check cls._csq on line 64?

_query_columns_case_sensitive_by_default returns a boolean which removes the need for a tuple. Am I misunderstanding?

Copy link
Copy Markdown
Collaborator Author

@d-w-moore d-w-moore Apr 28, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

oh. The in (True,) thing? I could have just said == True, if that's any better. Lots of folk are allergic to that, but in Python it's meaningful because of the False != None gotcha.

I expect people to object, it's part of software developer psychology to hate it when people write Python that contains things like is True or == True. I don't mind arguing with an actual human because you can convince them usually to see the logic.

But what I also expect is that codacy and ruff will heartily object and I'll be wasting a good half hour, in aggregate, dealing with the fallout. But when I write that type of code, I mean it exactly the way I wrote it... Lol

Copy link
Copy Markdown
Contributor

@korydraughn korydraughn Apr 28, 2026

Choose a reason for hiding this comment

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

Yep. What you typed felt very intentional. Just trying to understand the python-isms.

Is the in (True,) style a common pattern in python?

Copy link
Copy Markdown
Collaborator Author

@d-w-moore d-w-moore Apr 28, 2026

Choose a reason for hiding this comment

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

no, actually. Just my own perverse variation : ) . It kind of screams, "please review. and change as appropriate."

Copy link
Copy Markdown
Collaborator Author

@d-w-moore d-w-moore Apr 28, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

In the early review process, I like having humans look at things. Then, and only afterward, let codacy and ruff have their turn. Saves a lot of useless machinations.


resource_name = helpers.get_register_resource(sess)
if resource_name:
cls.register_resc = resource_name
Expand Down Expand Up @@ -214,28 +247,25 @@
self.assertEqual(len(result12), 0)

# BETWEEN tests

# TODO(#600): Uncomment these lines and/or make a new test when database flavor can be detected.
# The resultset for BETWEEN queries can differ from database to database.

# result13 = self.sess.query(DataObject.name).filter(
# Collection.name == self.coll_path).filter(
# Between(DataObject.name, [self.case_sensitive_obj_name1,
# self.case_sensitive_obj_name1 + "_"])).all()
# self.assertTrue(result13.has_value(self.case_sensitive_obj_name1))
# self.assertEqual(len(result13), 1)

# result14 = self.sess.query(DataObject.name).filter(
# Collection.name == self.coll_path).filter(
# Between(DataObject.name, [str.lower(self.case_sensitive_obj_name1),
# str.lower(self.case_sensitive_obj_name1) + "_"])).all()
# self.assertEqual(len(result14), 0)

# result15 = self.sess.query(DataObject.name).filter(
# Collection.name == self.coll_path).filter(
# Between(DataObject.name, [str.upper(self.case_sensitive_obj_name1),
# str.upper(self.case_sensitive_obj_name1) + "_"])).all()
# self.assertEqual(len(result15), 0)
if self.case_sensitive_testing_feasible:
result13 = self.sess.query(DataObject.name).filter(
Collection.name == self.coll_path).filter(
Between(DataObject.name, [self.case_sensitive_obj_name1,
self.case_sensitive_obj_name1 + "_"])).all()
self.assertTrue(result13.has_value(self.case_sensitive_obj_name1))
self.assertEqual(len(result13), 1)

result14 = self.sess.query(DataObject.name).filter(
Collection.name == self.coll_path).filter(
Between(DataObject.name, [str.lower(self.case_sensitive_obj_name1),
str.lower(self.case_sensitive_obj_name1) + "_"])).all()
self.assertEqual(len(result14), 0)

result15 = self.sess.query(DataObject.name).filter(
Collection.name == self.coll_path).filter(
Between(DataObject.name, [str.upper(self.case_sensitive_obj_name1),
str.upper(self.case_sensitive_obj_name1) + "_"])).all()

Check failure on line 267 in irods/test/query_test.py

View workflow job for this annotation

GitHub Actions / ruff-lint / ruff-format

Ruff format 

Improper formatting
self.assertEqual(len(result15), 0)

def test_files_query_case_insensitive(self):
# This tests that GenQueries are case-insensitive when the case_sensitive
Expand Down
Loading