Repository: mlflow/mlflow
Pull request: [pagination] followups to clean up search registered models
## What changes are proposed in this pull request?

This PR cleans up some issues with testing and edge cases with search registered models:

1. It adds tests to verify that lowercase and uppercase ordering keys work.
2. It changes search `order_by` parsing in `search_runs` and `search_registered_models` to throw if an invalid token is passed in

per discussion with @mparkhe and @sueann 

## How is this patch tested?

Unit tests and manual

## Release Notes

### Is this a user-facing change?

- [x] No. You can skip the rest of this section.
- [ ] Yes. Give a description of this change to be included in the release notes for MLflow users.

(Details in 1-2 sentences. You can just refer to another PR with a description if this PR is part of a larger change.)

### What component(s), interfaces, languages, and integrations does this PR affect?
Components 
- [ ] `area/artifacts`: Artifact stores and artifact logging
- [ ] `area/build`: Build and test infrastructure for MLflow
- [ ] `area/docs`: MLflow documentation pages
- [ ] `area/examples`: Example code
- [x] `area/model-registry`: Model Registry service, APIs, and the fluent client calls for
Model Registry
- [ ] `area/models`: MLmodel format, model serialization/deserialization, flavors
- [ ] `area/projects`: MLproject format, project running backends
- [ ] `area/scoring`: Local serving, model deployment tools, spark UDFs
- [ ] `area/tracking`: Tracking Service, tracking client APIs, autologging

Interface 
- [ ] `area/uiux`: Front-end, user experience, JavaScript, plotting
- [ ] `area/docker`: Docker use across MLflow's components, such as MLflow Projects and MLflow Models
- [ ] `area/sqlalchemy`: Use of SQLAlchemy in the Tracking Service or Model Registry
- [ ] `area/windows`: Windows support

Language 
- [ ] `language/r`: R APIs and clients
- [ ] `language/java`: Java APIs and clients

Integrations
- [ ] `integrations/azure`: Azure and Azure ML integrations
- [ ] `integrations/sagemaker`: SageMaker integrations

### How should the PR be classified in the release notes? Choose one:

- [ ] `rn/breaking-change` - The PR will be mentioned in the "Breaking Changes" section
- [x] `rn/none` - No description will be included. The PR will be mentioned only by the PR number in the "Small Bugfixes and Documentation Updates" section
- [ ] `rn/feature` - A new user-facing feature worth mentioning in the release notes
- [ ] `rn/bug-fix` - A user-facing bug fix worth mentioning in the release notes
- [ ] `rn/documentation` - A user-facing documentation change worth mentioning in the release notes


Problem statement:
[BUG] `mlflow.search_runs` does not raise error or warning when passed invalid `order_by` expression
`mlflow.search_runs` does not raise error or warning when passed invalid `order_by` expression. I think this behavior is dangerous. Let's consider a scenario where a user wants to select the best model from a bunch of runs and writes the code below:

```python
order_by = ["metrics.accuracy DECS"]  # "DECS" is a typo, should be "DESC"
sorted_runs = mlflow.search_runs(..., order_by=order_by)
best_run = sorted_runs.iloc[0]  # select the frist row
best_model = load_model_from_run(best_run)
```

What this code does is:
- The `order_by` expression `"metrics.accuracy DECS"` doesn't end with ` desc` or ` asc` so it's parsed as a metric named `accuracy DECS`.
- `search_runs` sorts runs by the metric `accuracy DECS` which doesn't exist and returns unsorted runs.
- `sorted_runs.iloc[0]` is obviously not the best run, **but no error or warning is raised**.

Hints from issue discussion:
I think this is because we attempt to parse ASC or DESC from the oder by clause, but we default to `is_ascending=True` if we can't parse something valid from it

I can take this one

Commit message: DRY

Patch to review:
diff --git a/mlflow/utils/search_utils.py b/mlflow/utils/search_utils.py
index 2b8712a034b37..66aa6bcbba521 100644
--- a/mlflow/utils/search_utils.py
+++ b/mlflow/utils/search_utils.py
@@ -19,6 +19,9 @@
 class SearchUtils(object):
     LIKE_OPERATOR = "LIKE"
     ILIKE_OPERATOR = "ILIKE"
+    ASC_OPERATOR = "asc"
+    DESC_OPERATOR = "desc"
+    VALID_ORDER_BY_TAGS = [ASC_OPERATOR, DESC_OPERATOR]
     VALID_METRIC_COMPARATORS = set(['>', '>=', '!=', '=', '<', '<='])
     VALID_PARAM_COMPARATORS = set(['!=', '=', LIKE_OPERATOR, ILIKE_OPERATOR])
     VALID_TAG_COMPARATORS = set(['!=', '=', LIKE_OPERATOR, ILIKE_OPERATOR])
@@ -363,11 +366,20 @@ def _validate_order_by_and_generate_token(cls, order_by):
     def _parse_order_by_string(cls, order_by):
         token_value = cls._validate_order_by_and_generate_token(order_by)
         is_ascending = True
-        if token_value.lower().endswith(" desc"):
-            is_ascending = False
-            token_value = token_value[0:-len(" desc")]
-        elif token_value.lower().endswith(" asc"):
-            token_value = token_value[0:-len(" asc")]
+        tokens = token_value.split()
+        if len(tokens) > 2:
+            raise MlflowException(f"Invalid order_by clause '{order_by}'. Could not be parsed.",
+                                  error_code=INVALID_PARAMETER_VALUE)
+        elif len(tokens) == 2:
+            order_token = tokens[1].lower()
+            if order_token not in cls.VALID_ORDER_BY_TAGS:
+                raise MlflowException(f"Invalid ordering key in order_by clause '{order_by}'.",
+                                      error_code=INVALID_PARAMETER_VALUE)
+            if order_token == cls.DESC_OPERATOR:
+                is_ascending = False
+                token_value = tokens[0]
+            elif order_token == cls.ASC_OPERATOR:
+                token_value = tokens[0]
         return token_value, is_ascending
 
     @classmethod
diff --git a/tests/store/model_registry/test_sqlalchemy_store.py b/tests/store/model_registry/test_sqlalchemy_store.py
index 821d5edef81ef..0d98e8ad20691 100644
--- a/tests/store/model_registry/test_sqlalchemy_store.py
+++ b/tests/store/model_registry/test_sqlalchemy_store.py
@@ -515,6 +515,11 @@ def test_search_registered_models(self):
         rms, _ = self._search_registered_models(f"name iLike '%blah%'")
         self.assertEqual(rms, [])
 
+        # confirm that ILIKE works for empty query
+        rms, _ = self._search_registered_models(f"name iLike '%%'")
+        # raise Exception()
+        self.assertEqual(rms, names)
+
         rms, _ = self._search_registered_models(f"name ilike '%RM4a'")
         self.assertEqual(rms, names[4:])
 
@@ -715,6 +720,29 @@ def test_search_registered_model_order_by(self):
                                                    order_by=['timestamp  ASC'],
                                                    max_results=100)
         self.assertEqual([rm1, rm2, rm3, rm4], result)
+        # validate order by key is case-insensitive
+        result, _ = self._search_registered_models(query,
+                                                   page_token=None,
+                                                   order_by=['timestamp  asc'],
+                                                   max_results=100)
+        self.assertEqual([rm1, rm2, rm3, rm4], result)
+        result, _ = self._search_registered_models(query,
+                                                   page_token=None,
+                                                   order_by=['timestamp  aSC'],
+                                                   max_results=100)
+        self.assertEqual([rm1, rm2, rm3, rm4], result)
+        result, _ = self._search_registered_models(query,
+                                                   page_token=None,
+                                                   order_by=['timestamp  desc',
+                                                             'name desc'],
+                                                   max_results=100)
+        self.assertEqual([rm4, rm3, rm2, rm1], result)
+        result, _ = self._search_registered_models(query,
+                                                   page_token=None,
+                                                   order_by=['timestamp  deSc',
+                                                             'name deSc'],
+                                                   max_results=100)
+        self.assertEqual([rm4, rm3, rm2, rm1], result)
 
     def test_search_registered_model_order_by_errors(self):
         query = "name LIKE 'RM%'"
@@ -763,3 +791,19 @@ def test_search_registered_model_order_by_errors(self):
                                            order_by=['timestamp somerandomstuff'],
                                            max_results=5)
         assert exception_context.exception.error_code == ErrorCode.Name(INVALID_PARAMETER_VALUE)
+
+        # test that timestamp with random strings is invalid
+        with self.assertRaises(MlflowException) as exception_context:
+            self._search_registered_models(query,
+                                           page_token=None,
+                                           order_by=['timestamp decs'],
+                                           max_results=5)
+        assert exception_context.exception.error_code == ErrorCode.Name(INVALID_PARAMETER_VALUE)
+
+        # test that timestamp with random strings is invalid
+        with self.assertRaises(MlflowException) as exception_context:
+            self._search_registered_models(query,
+                                           page_token=None,
+                                           order_by=['timestamp ACS'],
+                                           max_results=5)
+        assert exception_context.exception.error_code == ErrorCode.Name(INVALID_PARAMETER_VALUE)
diff --git a/tests/store/tracking/test_sqlalchemy_store.py b/tests/store/tracking/test_sqlalchemy_store.py
index d2b4e2ddd54df..ab45a0dffa151 100644
--- a/tests/store/tracking/test_sqlalchemy_store.py
+++ b/tests/store/tracking/test_sqlalchemy_store.py
@@ -1001,6 +1001,12 @@ def create_run(start_time, end):
                              self.get_ordered_runs(["attribute.start_time asc",
                                                     "attribute.end_time desc"], experiment_id))
 
+        # invalid asc/desc throw errors
+        with self.assertRaises(MlflowException) as exception_context:
+            self.get_ordered_runs(["attribute.start_time aCS",
+                                   "attribute.end_time desc"], experiment_id)
+        assert exception_context.exception.error_code == ErrorCode.Name(INVALID_PARAMETER_VALUE)
+
     def test_search_vanilla(self):
         exp = self._experiment_factory('search_vanilla')
         runs = [self._run_factory(self._get_run_configs(exp)).info.run_id


Code review comments:
diff_hunk: @@ -363,11 +366,20 @@ def _validate_order_by_and_generate_token(cls, order_by):
     def _parse_order_by_string(cls, order_by):
         token_value = cls._validate_order_by_and_generate_token(order_by)
         is_ascending = True
-        if token_value.lower().endswith(" desc"):
-            is_ascending = False
-            token_value = token_value[0:-len(" desc")]
-        elif token_value.lower().endswith(" asc"):
-            token_value = token_value[0:-len(" asc")]
+        tokens = token_value.split()
+        if len(tokens) > 2:
+            raise MlflowException(f"Invalid order_by clause '{order_by}'. Could not be parsed.",
+                                  error_code=INVALID_PARAMETER_VALUE)
+        elif len(tokens) == 2:
+            order_token = tokens[1].lower()
+            if order_token not in cls.VALID_ORDER_BY_TAGS:
+                raise MlflowException(f"Invalid ordering key in order_by clause '{order_by}'.",
+                                      error_code=INVALID_PARAMETER_VALUE)
+            if order_token == cls.DESC_OPERATOR:
+                is_ascending = False
+                token_value = tokens[0]
+            elif order_token == cls.ASC_OPERATOR:
+                token_value = tokens[0]
path: mlflow/utils/search_utils.py
text: @user1:
nit: could do
```
is_ascending  = (order_token == cls.ASC_OPERATOR)
token_value = tokens[0]
```