- Notifications
You must be signed in to change notification settings - Fork691
Make Oracle integration tests run and fix Oracle single statement execution code#2126
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
Uh oh!
There was an error while loading.Please reload this page.
Changes fromall commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -16,33 +16,85 @@ | ||
| // | ||
| #endregion | ||
| usingSystem; | ||
| usingSystem.Collections.Generic; | ||
| usingJetBrains.Annotations; | ||
| namespaceFluentMigrator.Runner.Generators.Oracle | ||
| { | ||
| publicclassOracleQuoter:OracleQuoterQuotedIdentifier | ||
| { | ||
| /// <summary> | ||
| /// https://docs.oracle.com/cd/A97630_01/appdev.920/a42525/apb.htm | ||
| /// </summary> | ||
| privatestaticreadonlyHashSet<string>_keywords=newHashSet<string>( | ||
| new[] | ||
| { | ||
| "access","else","modify","start","add","exclusive","noaudit","select", | ||
| "all","exists","nocompress","session","alter","file","not","set", | ||
| "and","float","notfound","share","any","for","nowait","size", | ||
| "arraylen","from","null","smallint","as","grant","number","sqlbuf", | ||
| "asc","group","of","successful","audit","having","offline","synonym", | ||
| "between","identified","on","sysdate","by","immediate","online","table", | ||
| "char","in","option","then","check","increment","or","to", | ||
| "cluster","index","order","trigger","column","initial","pctfree","uid", | ||
| "comment","insert","prior","union","compress","integer","privileges","unique", | ||
| "connect","intersect","public","update","create","into","raw","user", | ||
| "current","is","rename","validate","date","level","resource","values", | ||
| "decimal","like","revoke","varchar","default","lock","row","varchar2", | ||
| "delete","long","rowid","view","desc","maxextents","rowlabel","whenever", | ||
| "distinct","minus","rownum","where","drop","mode","rows","with" | ||
| }, | ||
| StringComparer.OrdinalIgnoreCase); | ||
| [ContractAnnotation("name:null => false")] | ||
| protectedoverrideboolShouldQuote(stringname) | ||
| { | ||
| if(string.IsNullOrEmpty(name)) | ||
| { | ||
| returnfalse; | ||
| } | ||
| if(_keywords.Contains(name)) | ||
Collaborator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Why not just always quote identifiers? CollaboratorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Because it would be too breaking. Quoted identifiers in Oracle become case sensitive 🙃 Collaborator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Got it. I would put that in a separate PR though with a separate issue, right? Because currently we're already in case sensitive behavior, which I can see as non-desirable. We would need to list that in the release notes as a breaking change to move to case sensitive by default. We may also want a way to easily allow forcing case sensitive identifiers to maintain backward compatibility, right? CollaboratorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Why not, but without it the integration tests on Oracle won't work, or I ignore them for now ? Collaborator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Ok, can you create a separate Issue and I will put it in the 7.2 milestone. I think GitHub will just generate release notes correctly. CollaboratorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. | ||
| { | ||
| returntrue; | ||
| } | ||
| // Otherwise, quote only when it's not a valid Oracle identifier | ||
| varfirst=name[0]; | ||
| if(!IsLetter(first)) | ||
| { | ||
| returntrue; | ||
| } | ||
| varlen=name.Length; | ||
| for(vari=1;i<len;i++) | ||
| { | ||
| varc=name[i]; | ||
| if(!IsIdentifierChar(c)) | ||
| { | ||
| returntrue; | ||
| } | ||
| } | ||
| returnfalse; | ||
| } | ||
| privatestaticboolIsLetter(charc) | ||
| { | ||
| returncis>='A' and<='Z' or>='a' and<='z'; | ||
| } | ||
| privatestaticboolIsIdentifierChar(charc) | ||
| { | ||
| returnIsLetter(c)||IsDigit(c)||c=='_'||c=='$'||c=='#'; | ||
| } | ||
| privatestaticboolIsDigit(charc) | ||
| { | ||
| returncis>='0' and<='9'; | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -58,6 +58,6 @@ public Oracle12CManagedProcessor( | ||
| /// <inheritdoc /> | ||
| public override IList<string> DatabaseTypeAliases { get; } = new List<string>() | ||
| { "Oracle-12c-Managed", "Oracle 12c Managed", "Oracle Managed", "Oracle" }; | ||
Collaborator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. 😰 | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -17,10 +17,10 @@ | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Data; | ||
| using System.Text; | ||
| using FluentMigrator.Expressions; | ||
| using FluentMigrator.Runner.BatchParser; | ||
| using FluentMigrator.Runner.Generators; | ||
| using FluentMigrator.Runner.Generators.Oracle; | ||
| using FluentMigrator.Runner.Helpers; | ||
| @@ -195,15 +195,41 @@ public override bool SequenceExists(string schemaName, string sequenceName) | ||
| public override bool DefaultValueExists(string schemaName, string tableName, string columnName, | ||
| object defaultValue) | ||
| { | ||
| if (tableName == null) | ||
| { | ||
| throw new ArgumentNullException(nameof(tableName)); | ||
| } | ||
| if (columnName == null) | ||
| { | ||
| throw new ArgumentNullException(nameof(columnName)); | ||
| } | ||
| if (columnName.Length == 0 || tableName.Length == 0) | ||
| { | ||
| return false; | ||
| } | ||
| if (string.IsNullOrEmpty(schemaName)) | ||
| { | ||
| return Exists( | ||
| "SELECT 1 FROM USER_TAB_COLUMNS WHERE upper(TABLE_NAME) = '{0}' AND upper(COLUMN_NAME) = '{1}' AND DATA_DEFAULT IS NOT NULL", | ||
| FormatHelper.FormatSqlEscape(tableName.ToUpper()), | ||
| FormatHelper.FormatSqlEscape(columnName.ToUpper())); | ||
| } | ||
| return Exists( | ||
| "SELECT 1 FROM ALL_TAB_COLUMNS WHERE upper(OWNER) = '{0}' AND upper(TABLE_NAME) = '{1}' AND upper(COLUMN_NAME) = '{2}' AND DATA_DEFAULT IS NOT NULL", | ||
| schemaName.ToUpper(), FormatHelper.FormatSqlEscape(tableName.ToUpper()), | ||
| FormatHelper.FormatSqlEscape(columnName.ToUpper())); | ||
| } | ||
| public override void Execute([StructuredMessageTemplate]string template, params object[] args) | ||
| { | ||
| Process(string.Format(template, args)); | ||
| } | ||
| public override bool Exists([StructuredMessageTemplate]string template, params object[] args) | ||
| { | ||
| if (template == null) | ||
| { | ||
| @@ -235,7 +261,7 @@ public override DataSet ReadTableData(string schemaName, string tableName) | ||
| return Read("SELECT * FROM {0}.{1}", Quoter.QuoteSchemaName(schemaName), Quoter.QuoteTableName(tableName)); | ||
| } | ||
| public override DataSet Read([StructuredMessageTemplate]string template, params object[] args) | ||
| { | ||
| if (template == null) | ||
| { | ||
| @@ -265,6 +291,110 @@ public override void Process(PerformDBOperationExpression expression) | ||
| expression.Operation?.Invoke(Connection, Transaction); | ||
| } | ||
| /// <summary> | ||
| /// Splits a SQL script into individual statements, taking into account Oracle-specific syntax rules. | ||
| /// <remarks>We could use <see cref="SqlBatchParser"/> but it does not handle multiple lines strings.</remarks> | ||
| /// </summary> | ||
| private static List<string> SplitOracleSqlStatements(string sqlScript) | ||
CollaboratorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. I just noticed (again) the existence of SemicolonSearcher, SqlBatchParser etc, I should use it, what do you think ? CollaboratorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. I tried to use it (with the same implementation as SnowflakeBatchParser) but it seems that it matches semicolons inside "ranges", in my case : strings 🫠 Collaborator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. I am not a big fan of the batch parser. sql is a very complicated dialect CollaboratorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Oh I think I understood ... My string spans multiple lines (I'm in the long string tests for Oracle) CollaboratorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more.
Yeah, neither am I, but it still a lot better than the Regex previously used Collaborator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. The simplest regex would be if the line ends in a CollaboratorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. how to handle strings spaning multiple lines, containing a semicolon ? I'm sure the batch parser could be solid, but it needs to handle "ranges" spanning multiple lines, which it does not ATM. CollaboratorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. I finally gave up on using the SqlBatchParser, its handling of ranges line-by-line seems broken Collaborator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. my regex hack makes it so long as the last non whitespace character on a line is a | ||
| { | ||
| var statements = new List<string>(); | ||
| var currentStatement = new StringBuilder(); | ||
| var inString = false; | ||
| var inIdentifier = false; | ||
| var inSingleLineComment = false; | ||
| var inMultiLineComment = false; | ||
| var prevChar = '\0'; | ||
| foreach (var c in sqlScript) | ||
| { | ||
| if (inSingleLineComment) | ||
| { | ||
| currentStatement.Append(c); | ||
| if (c == '\n') | ||
| { | ||
| inSingleLineComment = false; | ||
| } | ||
| continue; | ||
| } | ||
| if (inMultiLineComment) | ||
| { | ||
| currentStatement.Append(c); | ||
| if (prevChar == '*' && c == '/') | ||
| { | ||
| inMultiLineComment = false; | ||
| } | ||
| prevChar = c; | ||
| continue; | ||
| } | ||
| if (inString) | ||
| { | ||
| currentStatement.Append(c); | ||
| if (c == '\'' && prevChar != '\\') | ||
| { | ||
| inString = false; | ||
| } | ||
| prevChar = c; | ||
| continue; | ||
| } | ||
| if (inIdentifier) | ||
| { | ||
| currentStatement.Append(c); | ||
| if (c == '"') | ||
| { | ||
| inIdentifier = false; | ||
| } | ||
| prevChar = c; | ||
| continue; | ||
| } | ||
| switch (c) | ||
| { | ||
| // Check for comment start | ||
| case '-' when prevChar == '-': | ||
| inSingleLineComment = true; | ||
| currentStatement.Append(c); | ||
| continue; | ||
| case '*' when prevChar == '/': | ||
| inMultiLineComment = true; | ||
| currentStatement.Append(c); | ||
| continue; | ||
| // Check for string start | ||
| case '\'': | ||
| inString = true; | ||
| currentStatement.Append(c); | ||
| prevChar = c; | ||
| continue; | ||
| // Check for inIdentifier start | ||
| case '"': | ||
| inIdentifier = true; | ||
| currentStatement.Append(c); | ||
| prevChar = c; | ||
| continue; | ||
| // Check for statement terminator | ||
| case ';': | ||
| statements.Add(currentStatement.ToString().TrimEnd(';').Trim()); | ||
| currentStatement.Clear(); | ||
| prevChar = '\0'; | ||
| continue; | ||
| default: | ||
| currentStatement.Append(c); | ||
| prevChar = c; | ||
| break; | ||
| } | ||
| } | ||
| // Add any remaining content | ||
| if (currentStatement.Length > 0) | ||
| { | ||
| statements.Add(currentStatement.ToString()); | ||
| } | ||
| return statements; | ||
| } | ||
| protected override void Process(string sql) | ||
| { | ||
| Logger.LogSql(sql); | ||
| @@ -276,9 +406,7 @@ protected override void Process(string sql) | ||
| EnsureConnectionIsOpen(); | ||
| var batches = SplitOracleSqlStatements(sql); | ||
| foreach (var batch in batches) | ||
| { | ||
Uh oh!
There was an error while loading.Please reload this page.