FEAT:Support 5.1-Modified JSON_Haskey_Lookup #459
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors json_HasKeyLookup in mssql/functions.py to streamline parameter initialization and SQL generation for JSON key lookups, and updates _get_check_sql for Django 5.1+ compatibility.
- Initialized
lhs_paramsandconditionslists for more consistent handling. - Simplified JSON path checks using
JSON_VALUEand unified logic across SQL Server versions. - Updated
_get_check_sqlto useself.conditionfor Django 5.1+ while preserving backwards compatibility.
Comments suppressed due to low confidence (1)
mssql/functions.py:284
- The variable
cast_sqlis assigned but never used: the subsequentconditionsuselhsinstead ofcast_sql, andcast_paramsare not applied. You should usecast_sqlin theJSON_VALUEexpression and includecast_params(or merge them withlhs_params) in the returned params.
cast_sql, cast_params = self.lhs.as_sql(compiler, connection)
mssql/functions.py
Outdated
| @@ -297,10 +296,8 @@ def json_HasKeyLookup(self, compiler, connection): | |||
| else: | |||
| # if no operators are specified | |||
| sql = conditions[0] | |||
|
|
|||
| return sql, cast_params | |||
| return sql, lhs_params | |||
There was a problem hiding this comment.
Returning only lhs_params here discards the cast_params needed for the CAST expression. Consider returning cast_params + lhs_params (or vice versa) to include all required parameters.
| return sql, lhs_params | |
| return sql, cast_params + lhs_params |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Please check this comment & resolve.
mssql/functions.py
Outdated
| logical_op = " " + self.logical_operator + " " | ||
| sql = "(" + logical_op.join(conditions) + ")" | ||
| path_escaped = path.replace("'", "''") | ||
| conditions.append("JSON_VALUE(%s, '%s') IS NOT NULL" % (lhs, path_escaped)) |
There was a problem hiding this comment.
This inlines both lhs and path_escaped into the SQL string, leaving no placeholders; passing lhs_params to the caller may introduce extraneous parameters. Either switch to placeholders with a params list or return an empty list when there are none.
| conditions.append("JSON_VALUE(%s, '%s') IS NOT NULL" % (lhs, path_escaped)) | |
| conditions.append("JSON_VALUE(%s, %s) IS NOT NULL") | |
| lhs_params.append(path_escaped) |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Consider parameterizing the SQL instead of injecting path_escaped directly—this will improve security and prevent bugs with unusual key names. Also, the code for building conditions is duplicated across multiple branches; factoring it out into a helper (e.g. _build_json_condition) could make future changes easier and less error prone. Finally, double-check that all return statements yield consistent parameter lists, not just empty ones in some cases.
e.g.
Instead of
conditions.append("JSON_VALUE(%s, '%s') IS NOT NULL" % (lhs, path_escaped))
Can change to
conditions.append("JSON_VALUE(%s, %s) IS NOT NULL")
lhs_params.extend([lhs, path])
Why: This uses SQL parameters (%s placeholders) for both the column and the path. The path is passed as a parameter, avoiding SQL injection and escaping bugs.
There was a problem hiding this comment.
Made the suggested changes
mssql/functions.py
Outdated
| logical_op = " " + self.logical_operator + " " | ||
| sql = "(" + logical_op.join(conditions) + ")" | ||
| path_escaped = path.replace("'", "''") | ||
| conditions.append("JSON_VALUE(%s, '%s') IS NOT NULL" % (lhs, path_escaped)) |
There was a problem hiding this comment.
Consider parameterizing the SQL instead of injecting path_escaped directly—this will improve security and prevent bugs with unusual key names. Also, the code for building conditions is duplicated across multiple branches; factoring it out into a helper (e.g. _build_json_condition) could make future changes easier and less error prone. Finally, double-check that all return statements yield consistent parameter lists, not just empty ones in some cases.
e.g.
Instead of
conditions.append("JSON_VALUE(%s, '%s') IS NOT NULL" % (lhs, path_escaped))
Can change to
conditions.append("JSON_VALUE(%s, %s) IS NOT NULL")
lhs_params.extend([lhs, path])
Why: This uses SQL parameters (%s placeholders) for both the column and the path. The path is passed as a parameter, avoiding SQL injection and escaping bugs.
mssql/functions.py
Outdated
| @@ -297,10 +296,8 @@ def json_HasKeyLookup(self, compiler, connection): | |||
| else: | |||
| # if no operators are specified | |||
| sql = conditions[0] | |||
|
|
|||
| return sql, cast_params | |||
| return sql, lhs_params | |||
There was a problem hiding this comment.
Please check this comment & resolve.
This pull request refactors and enhances JSON handling for SQL Server in
mssql/functions.py, focusing on improving readability, modularity, and support for logical operators in JSON conditions. The most important changes include introducing a helper function for condition building, updating thejson_HasKeyLookupmethod for better SQL generation, and improving compatibility with different SQL Server versions.Refactoring and Modularity:
_build_json_conditionshelper function to centralize and simplify the logic for combining SQL conditions with logical operators (e.g., AND/OR). This reduces code duplication and improves maintainability.Enhancements to
json_HasKeyLookup:JSON_PATH_EXISTSwith better escaping and condition building. For older SQL Server versions, fallback logic now usesJSON_VALUEwith enhanced parameter handling.These changes make the codebase cleaner, easier to extend, and more robust in handling various SQL Server versions and JSON query scenarios.