NotificationsYou must be signed in to change notification settings
Fork1.9k
Star5.4k

rewrite: Simplified MSSQLDatabase escaping#7448

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Open

mkarg wants to merge2 commits intoliquibase:master

base:master

Choose a base branch

frommkarg:simplified-mssql

Open

rewrite: Simplified MSSQLDatabase escaping#7448

mkarg wants to merge2 commits intoliquibase:masterfrommkarg:simplified-mssql

+13 −9

Conversation

Copy link

Contributor

mkarg commentedNov 30, 2025

Impact

Bug fix (non-breaking change which fixes expected existing functionality)
Enhancement/New feature (adds functionality without impacting existing logic)
Breaking change (fix or feature that would cause existing functionality to change)

Description

This PR implements the code rewrite requested by@MalloD12 in#7426 (comment).

Things to be aware of

Did not touch other existing code besides the code locations mentioned in#7426 (comment). If wanted, other code locationscould be rewritten in the same way.

Things to worry about

N/A

Additional Context

N/A

rewrite: Simplified MSSQLDatabase escaping

314eed5

mkarg requested review fromfilipelautert andwwillard7800 ascode owners

November 30, 2025 14:20

mkarg requested a deployment to external

November 30, 2025 14:20

— with

GitHub Actions Waiting

mkarg temporarily deployed to external

November 30, 2025 14:20

— with

GitHub Actions Inactive

Copy link

coderabbitaibot commentedNov 30, 2025•
edited
Loading

📝 Walkthrough

Walkthrough

The pull request refactors the MSSQL tablespace and table name escaping logic by introducing a privateisQuoted() helper method and consolidating manual quote-checking logic. TheescapeTablespaceName() method behavior changes to return bracket-escaped names without surrounding quotes. Corresponding test expectations are updated to reflect this change.

Changes

Cohort / File(s)	Summary
MSSQL Database Escaping Refactor `liquibase-standard/src/main/java/liquibase/database/core/MSSQLDatabase.java`	Added private`isQuoted()` helper method to check for quoted identifiers. Refactored`escapeTableName()` and`escapeTablespaceName()` to use`isQuoted()` and`quoteObject()` instead of manual quote-check-and-concatenate logic. Changed`escapeTablespaceName()` to return bracket-escaped names without surrounding quotes.
Test Updates `liquibase-standard/src/test/java/liquibase/database/core/MSSQLDatabaseTest.java`,`liquibase-standard/src/test/java/liquibase/sqlgenerator/core/CreateTableGeneratorTest.java`	Updated test expectations across two test files to reflect the removed surrounding quotes from bracket-escaped tablespace names in`escapeTablespaceName()` output.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Verify the behavior change: TheescapeTablespaceName() method now returns bracket-escaped names without surrounding quotes—confirm this is intentional and affects only MSSQL-specific behavior.
ValidateisQuoted() logic: Check that the new helper correctly identifies quoted identifiers (both double-quotes and bracket characters).
Cross-checkquoteObject() usage: Ensure the new calls toquoteObject() produce identical output to the replaced manual logic.
Test alignment: Confirm all test updates across both test files consistently reflect the new behavior.

Possibly related PRs

liquibase/liquibase#7426: Directly modifies MSSQLDatabase's tablespace-escaping logic and related tests with similar refactoring patterns.

Suggested reviewers

filipelautert
wwillard7800

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run`@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'rewrite: Simplified MSSQLDatabase escaping' clearly and specifically describes the main change—a refactoring/simplification of MSSQL database escaping logic.
Description check	✅ Passed	The description includes the impact type, references the related code review discussion, and explains what was done, though it lacks specific details about how the changes work and their effects.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between072336b and314eed5.

📒 Files selected for processing (3)

liquibase-standard/src/main/java/liquibase/database/core/MSSQLDatabase.java (3 hunks)
liquibase-standard/src/test/java/liquibase/database/core/MSSQLDatabaseTest.java (2 hunks)
liquibase-standard/src/test/java/liquibase/sqlgenerator/core/CreateTableGeneratorTest.java (1 hunks)

🔇 Additional comments (6)

liquibase-standard/src/test/java/liquibase/sqlgenerator/core/CreateTableGeneratorTest.java (1)
1318-1318:LGTM!
The test expectation correctly reflects the new behavior whereescapeTablespaceName returns bracket-escaped names without surrounding double quotes.
liquibase-standard/src/test/java/liquibase/database/core/MSSQLDatabaseTest.java (2)
190-191:LGTM!
Test expectations correctly updated to reflect the new behavior where tablespace names with spaces are bracket-escaped without surrounding double quotes.
254-255:LGTM!
Test expectations correctly updated for tablespace names containing both spaces and special characters.
liquibase-standard/src/main/java/liquibase/database/core/MSSQLDatabase.java (3)
312-313:LGTM! Cleaner implementation usingquoteObject.
The refactoring simplifies the escaping logic by using theisQuoted helper and the built-inquoteObject method. The behavior change (no longer wrapping bracket-escaped names in double quotes) is reflected in the updated tests.
326-327:LGTM! Consistent refactoring withescapeTableName.
The changes mirror the refactoring inescapeTableName, using the new helper and built-inquoteObject method for cleaner, more maintainable code.
749-751:Clarify the double-quote check and evaluate null safety in context.
TheisQuoted method checks for both[ (MSSQL's native quoting character) and" (literal double-quote). Since MSSQL uses brackets for escaping, the double-quote check warrants clarification—it may be intentional for pre-quoted identifiers from external sources, or it could be an oversight.
The method lacks explicit null handling, though the review notes that callers check for null before invokingisQuoted. If this is consistent across all call sites, null safety may not be necessary; however, defensive programming practices might suggest adding a null guard for robustness if this method is part of a public or frequently-used internal API.