Address review feedback

Author: aclark4life

README.md

@@ -67,7 +67,7 @@ DEBUG_TOOLBAR_PANELS = [
 ```python
 # Maximum number of documents to return when re-executing select
 # queries (default is 100).
-DJDT_MQL_MAX_SELECT_RESULTS = 25
+DJDT_MQL_MAX_QUERY_RESULTS = 25
 
 # Queries slower than this threshold (in milliseconds) are highlighted
 # in the debug toolbar (default is 500 ms).

django_mongodb_extensions/mql_panel/__init__.py

@@ -1,3 +1,3 @@
 from django_mongodb_extensions.mql_panel.panel import MQLPanel
 
-__all__ = ["MQLPanel"]
+__all__ = [MQLPanel.panel_id]

django_mongodb_extensions/mql_panel/forms.py

@@ -10,15 +10,16 @@
 from pymongo import errors as pymongo_errors
 
 from django_mongodb_extensions.mql_panel.utils import (
-    MQL_PANEL_ID,
     QueryParts,
-    get_max_select_results,
+    get_max_query_results,
     parse_query_args,
 )
 
 
 class MQLBaseForm(SQLSelectForm):
     def clean(self):
+        from .panel import MQLPanel
+
         # Call forms.Form.clean() to bypass SQLSelectForm.clean() which has
         # SQL-specific validation
         cleaned_data = forms.Form.clean(self)
@@ -28,10 +29,10 @@ def clean(self):
         djdt_query_id = cleaned_data.get("djdt_query_id")
         if not djdt_query_id:
             raise ValidationError(_("Missing query ID."))
-        toolbar = DebugToolbar.fetch(request_id, panel_id=MQL_PANEL_ID)
+        toolbar = DebugToolbar.fetch(request_id, panel_id=MQLPanel.panel_id)
         if toolbar is None:
             raise ValidationError(_("Data for this panel isn't available anymore."))
-        panel = toolbar.get_panel_by_id(MQL_PANEL_ID)
+        panel = toolbar.get_panel_by_id(MQLPanel.panel_id)
         stats = panel.get_stats()
         if not stats or "queries" not in stats:
             raise ValidationError(_("Query data is not available."))
@@ -107,7 +108,7 @@ def _handle_operation_error(self, error, mql_string, operation_type="operation")
             if isinstance(error, ValueError):
                 header = "Query Parsing Error"
                 messages = [f"Query parsing error: {error}"]
-                if operation_type == "select":
+                if operation_type == "query":
                     messages += [
                         "The MQL panel can only re-execute read operations.",
                         "Write operations (insert, update, delete) cannot be re-executed.",
@@ -177,23 +178,23 @@ class MQLQueryForm(MQLBaseForm):
     def _execute_aggregate(self, collection, args_list):
         pipeline = args_list[0] if args_list else []
         result_docs = []
-        max_results = get_max_select_results()
+        max_results = get_max_query_results()
         with collection.aggregate(pipeline) as cursor:
             for i, doc in enumerate(cursor):
                 if i >= max_results:
                     break
                 result_docs.append(doc)
         return result_docs
 
-    def _execute_select(self, db, collection, collection_name, operation, args_list):
+    def _execute_query(self, db, collection, collection_name, operation, args_list):
         if operation == "aggregate":
             result_docs = self._execute_aggregate(collection, args_list)
         else:
             raise ValueError(f"Unsupported read operation: {operation}")
         return self.convert_documents_to_table(result_docs)
 
-    def select(self):
-        return self._execute_operation("select", self._execute_select)
+    def query(self):
+        return self._execute_operation("query", self._execute_query)
 
     def _format_cell_value(self, value):
         """Format a single cell value for table display."""

django_mongodb_extensions/mql_panel/panel.py

@@ -16,8 +16,6 @@
 
 from django_mongodb_extensions.mql_panel import views
 from django_mongodb_extensions.mql_panel.utils import (
-    MQL_PANEL_ID,
-    MQL_READ_OPERATIONS,
     get_mql_warning_threshold,
     patch_get_collection,
     patch_new_connection,
@@ -30,7 +28,8 @@
 
 
 class MQLPanel(SQLPanel):
-    panel_id = MQL_PANEL_ID
+    nav_title = _("MQL")
+    template = "mql_panel/mql.html"
 
     def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)
@@ -52,10 +51,6 @@ def record(self, **kwargs):
             self._databases[alias]["num_queries"] += 1
         self._mql_time += kwargs["duration"]
 
-    # Implement Panel API
-    nav_title = _("MQL")
-    template = "mql_panel/mql.html"
-
     @classmethod
     def get_urls(cls):
         return [
@@ -68,8 +63,8 @@ def nav_subtitle(self):
         stats = self.get_stats()
         query_count = len(stats.get("queries", []))
         return ngettext(
-            "%(query_count)d query in %(mql_time).2fms",
-            "%(query_count)d queries in %(mql_time).2fms",
+            "%(query_count)d query in %(mql_time).3f ms",
+            "%(query_count)d queries in %(mql_time).3f ms",
             query_count,
         ) % {
             "query_count": query_count,
@@ -120,7 +115,7 @@ def _hex_to_rgb(hex_color):
 
     @staticmethod
     def _is_read_operation(operation):
-        return operation in MQL_READ_OPERATIONS
+        return operation in {"aggregate"}
 
     def generate_stats(self, request, response):
         duplicate_query_groups = defaultdict(list)
@@ -137,8 +132,9 @@ def generate_stats(self, request, response):
                 duplicate_query_groups[(alias, dup_key)].append(query)
                 query["is_slow"] = query["duration"] > mql_warning_threshold
                 operation = query.get("mql_operation", "")
-                # Only show Sel/Expl buttons if it's a read operation AND
-                # the args were successfully serialized (mql_args_json is not None).
+                # Only show Query/Explain buttons if it's a read operation and
+                # the args were successfully serialized (mql_args_json is not
+                # None).
                 args_json = query.get("mql_args_json")
                 query["is_query"] = (
                     self._is_read_operation(operation) and args_json is not None

django_mongodb_extensions/mql_panel/utils.py

@@ -6,9 +6,7 @@
 from django.conf import settings
 from django_mongodb_backend.utils import OperationDebugWrapper
 
-MQL_PANEL_ID = "MQLPanel"
-MQL_READ_OPERATIONS = {"aggregate"}  # noqa
-DEFAULT_MAX_SELECT_RESULTS = 100
+DEFAULT_MAX_QUERY_RESULTS = 100
 DEFAULT_MQL_WARNING_THRESHOLD = 500
 _patched_connections = weakref.WeakSet()
 
@@ -50,10 +48,7 @@ def __init__(self, db, collection, logger):
     def log(self, op, duration, args, kwargs=None):
         args_str = ", ".join(repr(arg) for arg in args)
         operation = f"db.{self.collection_name}{op}({args_str})"
-        try:
-            args_json = json_util.dumps(list(args))
-        except (TypeError, ValueError):
-            args_json = None
+        args_json = json_util.dumps(list(args))
         if self.logger:
             self.logger.record(
                 alias=self.db.alias,
@@ -84,13 +79,13 @@ def format_mql_query(query):
     return f"db.{collection_name}.{operation}(\n{args_formatted}\n)"
 
 
-def get_max_select_results():
+def get_max_query_results():
     """Get the maximum number of results to return when re-executing queries.
 
-    Return the value from Django settings DJDT_MQL_MAX_SELECT_RESULTS if set,
+    Return the value from Django settings DJDT_MQL_MAX_QUERY_RESULTS if set,
     otherwise return the default value.
     """
-    return getattr(settings, "DJDT_MQL_MAX_SELECT_RESULTS", DEFAULT_MAX_SELECT_RESULTS)
+    return getattr(settings, "DJDT_MQL_MAX_QUERY_RESULTS", DEFAULT_MAX_QUERY_RESULTS)
 
 
 def get_mql_warning_threshold():
@@ -109,16 +104,7 @@ def parse_query_args(query_dict):
     collection_name = query_dict["mql_collection"]
     operation = query_dict["mql_operation"]
     args_json = query_dict["mql_args_json"]
-    # If args_json is None, serialization failed when the query was logged.
-    # Treat this as unreplayable to avoid re-executing a different query.
-    if args_json is None:
-        raise ValueError(
-            "Query arguments could not be serialized when logged. "
-            "This query cannot be re-executed because the original arguments are unavailable."
-        )
-    if args_json != "":
-        return collection_name, operation, json_util.loads(args_json)
-    return collection_name, operation, []
+    return collection_name, operation, json_util.loads(args_json)
 
 
 def patch_get_collection(connection):

django_mongodb_extensions/mql_panel/views.py

@@ -44,7 +44,7 @@ def mql_query(request):
     form = MQLQueryForm(verified_data)
     if form.is_valid():
         query = form.cleaned_data["query"]
-        result, headers = form.select()
+        result, headers = form.query()
 
         context = {
             "result": result,

django_mongodb_extensions/templates/mql_panel/mql.html

@@ -3,7 +3,7 @@
   {% for alias, info in databases %}
     <li>
       <strong><span class="djdt-color" data-djdt-styles="backgroundColor:rgb({{ info.rgb_color|join:', ' }})"></span> {{ alias }}</strong>
-      {{ info.time_spent|floatformat:"2" }} ms ({% blocktrans count info.num_queries as num %}{{ num }} query{% plural %}{{ num }} queries{% endblocktrans %}
+      {{ info.time_spent|floatformat:"3" }} ms ({% blocktrans count info.num_queries as num %}{{ num }} query{% plural %}{{ num }} queries{% endblocktrans %}
       {% if info.duplicate_count %}
         {% blocktrans with dupes=info.duplicate_count trimmed %}
           including <abbr title="Duplicate queries execute the same MQL operation and parameters.">{{ dupes }} duplicates</abbr>
@@ -40,7 +40,7 @@
             <button type="button" class="djToggleSwitch" data-toggle-name="sqlMain" data-toggle-id="{{ forloop.counter }}">+</button>
           </td>
           <td>
-            <div class="djDebugSql">{{ query.mql|safe }}</div>
+            <div class="djDebugSql">{{ query.mql }}</div>
             {% if query.duplicate_count %}
               <strong>
                 <span class="djdt-color" data-djdt-styles="backgroundColor:{{ query.duplicate_color }}"></span>
@@ -60,7 +60,7 @@
                 <form method="post">
                   {{ query.form.as_div }}
                   <button formaction="{% url 'djdt:mql_query' %}" class="remoteCall">Query</button>
-                  <button formaction="{% url 'djdt:mql_explain' %}" class="remoteCall">Expl</button>
+                  <button formaction="{% url 'djdt:mql_explain' %}" class="remoteCall">Explain</button>
                 </form>
               {% endif %}
             {% endif %}

django_mongodb_extensions/templates/mql_panel/mql_query.html

@@ -13,8 +13,8 @@ <h3>{% translate "Query Results" %}</h3>
       <dt>{% translate "Database" %}</dt>
       <dd>{{ alias }}</dd>
     </dl>
+    <h4>{% translate "Query Results" %}</h4>
     {% if result %}
-      <h4>{% translate "Query Results" %}</h4>
       <table>
         <thead>
           <tr>

pyproject.toml

@@ -18,6 +18,3 @@ test = [
 
 [tool.ruff.lint]
 select = ["I", "F"]
-
-[tool.hatch.build.targets.wheel]
-packages = ["django_mongodb_extensions"]

tests/mql_panel/test_panel.py

@@ -5,7 +5,6 @@
 
 from django_mongodb_extensions.mql_panel import MQLPanel
 from django_mongodb_extensions.mql_panel.forms import MQLQueryForm
-from django_mongodb_extensions.mql_panel.utils import parse_query_args
 
 rf = RequestFactory()
 
@@ -15,7 +14,7 @@ def mql_call():
     return list(User.objects.all())
 
 
-class BaseMQLTestCase(TestCase):
+class MQLPanelTests(TestCase):
     panel_id = MQLPanel.panel_id
 
     def setUp(self):
@@ -32,8 +31,6 @@ def tearDown(self):
     def get_response(self, request):
         return self._get_response(request)
 
-
-class MQLPanelTests(BaseMQLTestCase):
     def test_disabled(self):
         config = {"DISABLE_PANELS": {"django_mongodb_extensions.mql_panel.MQLPanel"}}
         self.assertIs(self.panel.enabled, True)
@@ -103,7 +100,7 @@ def test_nav_subtitle(self):
         mql_call()
         response = self.panel.process_request(self.request)
         self.panel.generate_stats(self.request, response)
-        self.assertRegex(self.panel.nav_subtitle, r"1 query in \d+\.\d+ms")
+        self.assertRegex(self.panel.nav_subtitle, r"1 query in \d+\.\d+ ms")
 
     def test_title(self):
         mql_call()
@@ -171,16 +168,3 @@ def test_handle_operation_error_format(self):
 
         # Should have one header
         self.assertEqual(headers[0], "Query Parsing Error")
-
-
-class ParseQueryArgsTests(TestCase):
-    def test_unserializable_args(self):
-        """None mql_args_json raises ValueError to prevent replaying a different query."""
-        with self.assertRaisesMessage(ValueError, "could not be serialized"):
-            parse_query_args(
-                {
-                    "mql_collection": "auth_user",
-                    "mql_operation": "aggregate",
-                    "mql_args_json": None,
-                }
-            )