-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Added new argument to preserve schema case DAT-10027 #2888
Conversation
DAT-10027
DAT-10027
DAT-10027
DAT-10027
@@ -188,6 +189,11 @@ public class GlobalConfiguration implements AutoloadedConfigurations { | |||
.setDefaultValue(true) | |||
.build(); | |||
|
|||
PRESERVE_SCHEMA_CASE = builder.define("preserveSchemaCase", Boolean.class) | |||
.setDescription("Should Liquibase preserve the original case of schemas") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe describe this as "Should liquibase treat schema and catalog names as case sensitive?"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it
@@ -297,7 +297,8 @@ public String correctObjectName(String objectName, Class<? extends DatabaseObjec | |||
if (objectName.contains("-") | |||
|| hasMixedCase(objectName) | |||
|| startsWithNumeric(objectName) | |||
|| isReservedWord(objectName)) { | |||
|| isReservedWord(objectName) | |||
|| Boolean.TRUE.equals(GlobalConfiguration.PRESERVE_SCHEMA_CASE.getCurrentValue())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will make it so that postgresql preserve the case on ALL objects if preserveSchemaCase is set?
I think you need to take this out of this if block and put it as a separate if block checking the objectType as the first lines of this function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I see your point. Testing didn't find any issues, but maybe they just didn't hit one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't have seen any issues in my manual testing because my non-schema objects were all lowercase for the Postgres tests. Even in the automated functional tests, we generally use the default casing for any given database platform unless we are specifically testing casing. If there is/was an issue, I can see why test would not have seen it.
Environment
Liquibase Version:
Liquibase Integration & Version: <Pick one: CLI, maven, gradle, spring boot, servlet, etc.>
Liquibase Extension(s) & Version:
Database Vendor & Version:
Operating System Type & Version:
Pull Request Type
Description
A clear and concise description of the issue being addressed. Additional guidance here.
Steps To Reproduce
List the steps to reproduce the behavior.
Actual Behavior
A clear and concise description of what happens in the software before this pull request.
Expected/Desired Behavior
A clear and concise description of what happens in the software after this pull request.
Screenshots (if appropriate)
If applicable, add screenshots to help explain your problem.
Additional Context
Add any other context about the problem here.
Fast Track PR Acceptance Checklist:
Need Help?
Come chat with us in the Liquibase Forum.