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

Review the internal data structures in class Query #158

Open
wants to merge 27 commits into
base: develop
Choose a base branch
from
Open

Conversation

RKrahl
Copy link
Member

@RKrahl RKrahl commented Aug 24, 2024

Review the internal data structures to represent the items in the ORDER BY and the WHERE clause in class Query. Implement dedicated helper classes to represent these items. As a result of #89, these data structures used to be a mapping of attribute names to format strings to build the corresponding item or condition in the ORDER BY or WHERE clause respectively. Now, we use lists of corresponding helper class objects instead. Retaining all the information used to build the item (attribute name, JPQL function if used, right hand side of the condition, direction in the order) in separate attributes of the helper class objects and building the format string on demand as needed makes the code more clear and consistent.

As a byproduct, we fix the formal string representation operator of class Query and close #94. Furthermore, we drop the restriction that any attribute may only appear once in the ORDER BY clause, as there is not really a justification for that restriction. E.g., the following is now allowed:

>>> query = Query(client, 'User', order=[('LENGTH(fullName)', 'DESC'), 'fullName'])
>>> str(query)
'SELECT o FROM User o ORDER BY LENGTH(o.fullName) DESC, o.fullName'

The changes are on the internal data sructures, there are only few changes in the documented API. Most prominently: the conditions argument to Query.addConditions() now accepts a list of tuples, with a pair of an attribute name and a condition on that attribute, respectively. The previous format, a mapping of attribute names to a (lists of) conditions, is still supported, but deprecated. E.g. you'd need to change

>>> # Still supported, but deprecated
>>> query = Query(client, "Datafile", conditions={
...     "dataset.name": "= 'e208945'",
...     "datafileCreateTime": [">= '2012-01-01'", "< '2013-01-01'" ],
... })
<stdin>:1: DeprecationWarning: Passing a mapping in the conditions argument is deprecated and will be removed in python-icat 3.0.
>>> str(query)
"SELECT o FROM Datafile o JOIN o.dataset AS ds WHERE o.datafileCreateTime >= '2012-01-01' AND o.datafileCreateTime < '2013-01-01' AND ds.name = 'e208945'"

to

>>> query = Query(client, "Datafile", conditions=[
...     ("dataset.name", "= 'e208945'"),
...     ("datafileCreateTime", ">= '2012-01-01'"),
...     ("datafileCreateTime", "< '2013-01-01'" ),
... ])
>>> str(query)
"SELECT o FROM Datafile o JOIN o.dataset AS ds WHERE o.datafileCreateTime >= '2012-01-01' AND o.datafileCreateTime < '2013-01-01' AND ds.name = 'e208945'"

While we are at it, also eliminate a some occurences of the old legacy ICAT query syntax in the code.

Draft status. Still TODO:

  • update all source code creating queries to use the new format of the conditions argument,
  • emit a deprecation warning in the case of passing a mapping in the conditions argument,
  • update the test_06_query.py module: update the conditions argument as in the producion code, add tests to verify that evaluating the formal string representation operator of Query may indeed be used to create an equivalent query, add tests for the legacy use of a mapping and verify that the deprecation warning is indeed being raised,
  • update all other tests creating queries to use the new format of the conditions argument,
  • update the documentation and example scripts regarding the new format of the conditions argument, in particular, update the tutorial.

@RKrahl RKrahl added the enhancement New feature or request label Aug 24, 2024
@RKrahl RKrahl added this to the 2.0.0 milestone Aug 24, 2024
@RKrahl
Copy link
Member Author

RKrahl commented Aug 24, 2024

It may be subject to discussion whether this is a breaking change that need to wait for a major release. One could argue that the documented API has only been augmented, the legacy use is still supported. But if any third party code was relying on the (intentionally not documented) internal data structures, for instance in subclasses of Query, that code will definitely be broken. Anyway, the Query class is rather central in python-icat and relevant changes to it merit a major release. Furthermore, this change is not urgent and we likely want to make a major release anyway, as soon as we add support for the schema changes expected in icat.server 7.0. So, I decided to target this for version 2.0.

@RKrahl
Copy link
Member Author

RKrahl commented Aug 24, 2024

conditions argument.  Not yet covered: tests, documentation examples.
argument when creating queries.

Not yet done: verify that evaluating the formal string representation
of Query works as expected, add tests for the legacy use of a mapping
and verify that the deprecation warning is indeed being raised.
  when creating queries
- While we are at it: eliminate a few more occurences of the old
  legacy ICAT query syntax in the tests
operator of Query may indeed be used to create an equivalent query to
the tests in test_06_query.py where it makes sense
argument in class Query.  Also verify that a DeprecationWarning is
raised.
argument when creating queries.  Eliminate a few more occurrences of
the old legacy ICAT query syntax on the way.
to use the new format of the conditions argument when creating
queries.
IDS" to use the new format of the conditions argument when creating
queries.
order of the keyword arguments and the order of calling the methods
during initialization.  This should essentially only have an impact on
the presentation of the module reference page in the documentation.
@RKrahl RKrahl marked this pull request as ready for review August 30, 2024 13:50
of the conditions argument is done in test_06_query.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalid return value from the formal string representation operator Query.__repr__()
2 participants