-
Notifications
You must be signed in to change notification settings - Fork 934
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
NH-1285 - Drop/Create script with default_schema/default_catalog fix(SqlServer) #448
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
…SqlServer) NH-2288 - Test updated (fully qualified name) Typo fixed feedbacks, new test, comments previous version restored(failing tests) Refactor quoting out of Configuration. Adjust obsoletes, avoid breaking changes. Missing async regen. Refactor get schema/catalog.
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,7 +40,7 @@ public abstract partial class Dialect | |
|
||
/// <summary> Characters used for closing quoted sql identifiers </summary> | ||
public const string PossibleClosedQuoteChars = "`'\"]"; | ||
|
||
private readonly TypeNames _typeNames = new TypeNames(); | ||
private readonly TypeNames _hibernateTypeNames = new TypeNames(); | ||
private readonly IDictionary<string, string> _properties = new Dictionary<string, string>(); | ||
|
@@ -390,14 +390,14 @@ public virtual string GetAddForeignKeyConstraintString(string constraintName, st | |
res.Append(" constraint ") | ||
.Append(constraintName) | ||
.Append(" foreign key (") | ||
.Append(StringHelper.Join(StringHelper.CommaSpace, foreignKey)) | ||
.Append(string.Join(StringHelper.CommaSpace, foreignKey)) | ||
.Append(") references ") | ||
.Append(referencedTable); | ||
|
||
if (!referencesPrimaryKey) | ||
{ | ||
res.Append(" (") | ||
.Append(StringHelper.Join(StringHelper.CommaSpace, primaryKey)) | ||
.Append(string.Join(StringHelper.CommaSpace, primaryKey)) | ||
.Append(')'); | ||
} | ||
|
||
|
@@ -758,15 +758,22 @@ public virtual string GetDropForeignKeyConstraintString(string constraintName) | |
return " drop constraint " + constraintName; | ||
} | ||
|
||
|
||
/// <summary> | ||
/// The syntax that is used to check if a constraint does not exists before creating it | ||
/// </summary> | ||
/// <param name="table">The table.</param> | ||
/// <param name="name">The name.</param> | ||
/// <returns></returns> | ||
// Since v5.1 | ||
[Obsolete("Can cause issues when a custom schema is defined (https://nhibernate.jira.com/browse/NH-1285). The new overload with the defaultSchema parameter should be used instead")] | ||
public virtual string GetIfNotExistsCreateConstraint(Table table, string name) | ||
{ | ||
return ""; | ||
var catalog = table.GetQuotedCatalog(this, null); | ||
var schema = table.GetQuotedSchema(this, null); | ||
var tableName = table.GetQuotedName(this); | ||
|
||
return GetIfNotExistsCreateConstraint(catalog, schema, tableName, name); | ||
} | ||
|
||
/// <summary> | ||
|
@@ -776,9 +783,15 @@ public virtual string GetIfNotExistsCreateConstraint(Table table, string name) | |
/// <param name="table">The table.</param> | ||
/// <param name="name">The name.</param> | ||
/// <returns></returns> | ||
// Since v5.1 | ||
[Obsolete("Can cause issues when a custom schema is defined (https://nhibernate.jira.com/browse/NH-1285). The new overload with the defaultSchema parameter should be used instead")] | ||
public virtual string GetIfNotExistsCreateConstraintEnd(Table table, string name) | ||
{ | ||
return ""; | ||
var catalog = table.GetQuotedCatalog(this, null); | ||
var schema = table.GetQuotedSchema(this, null); | ||
var tableName = table.GetQuotedName(this); | ||
|
||
return GetIfNotExistsCreateConstraintEnd(catalog, schema, tableName, name); | ||
} | ||
|
||
/// <summary> | ||
|
@@ -787,9 +800,15 @@ public virtual string GetIfNotExistsCreateConstraintEnd(Table table, string name | |
/// <param name="table">The table.</param> | ||
/// <param name="name">The name.</param> | ||
/// <returns></returns> | ||
// Since v5.1 | ||
[Obsolete("Can cause issues when a custom schema is defined (https://nhibernate.jira.com/browse/NH-1285). The new overload with the defaultSchema parameter should be used instead")] | ||
public virtual string GetIfExistsDropConstraint(Table table, string name) | ||
{ | ||
return ""; | ||
var catalog = table.GetQuotedCatalog(this, null); | ||
var schema = table.GetQuotedSchema(this, null); | ||
var tableName = table.GetQuotedName(this); | ||
|
||
return GetIfExistsDropConstraint(catalog, schema, tableName, name); | ||
} | ||
|
||
/// <summary> | ||
|
@@ -799,7 +818,67 @@ public virtual string GetIfExistsDropConstraint(Table table, string name) | |
/// <param name="table">The table.</param> | ||
/// <param name="name">The name.</param> | ||
/// <returns></returns> | ||
// Since v5.1 | ||
[Obsolete("Can cause issues when a custom schema is defined (https://nhibernate.jira.com/browse/NH-1285). The new overload with the defaultSchema parameter should be used instead")] | ||
public virtual string GetIfExistsDropConstraintEnd(Table table, string name) | ||
{ | ||
var catalog = table.GetQuotedCatalog(this, null); | ||
var schema = table.GetQuotedSchema(this, null); | ||
var tableName = table.GetQuotedName(this); | ||
|
||
return GetIfExistsDropConstraintEnd(catalog, schema, tableName, name); | ||
} | ||
|
||
/// <summary> | ||
/// The syntax that is used to check if a constraint does not exists before creating it | ||
/// </summary> | ||
/// <param name="catalog">The catalog.</param> | ||
/// <param name="schema">The schema.</param> | ||
/// <param name="table">The table.</param> | ||
/// <param name="name">The name.</param> | ||
/// <returns></returns> | ||
public virtual string GetIfNotExistsCreateConstraint(string catalog, string schema, string table, string name) | ||
{ | ||
return ""; | ||
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. Here you need to call 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 realize a bit late that we have a breaking change here. So dialects overriding the obsolete method will cease working as expected, because their override will now just be ignored. It means that such dialects will return an empty string for the new So when obsoleting overridable methods, we cause a breaking change by ceasing using them. In the present case, since the new default implementation is indeed "no-op", we could try avoiding the breaking change by calling the old obsolete method instead. The old method should then, of course, be reverted to its old "no-op" behavior instead of calling the new implementation. So furthermore all its removed overrides would have to be put in place again for avoiding another breaking change. But this does not stops here. If we do that, how should we then consider overrides of the new method? These overrides would then be somewhat breaking change too, since they would cause another dialect further down the inheritance chain and overriding itself the obsolete method to cease achieving its override... So either we fully revert this PR to avoid this kind of breaking change, or we add this as possible breaking change in 5.1, or we bump version to 6.0 (and still add this as possible breaking change). An additional conclusion to this is that obsoleting overridable methods is in most cases a breaking change. (It could be done without breaking changes only if the new method call the old one and is not overridden in an exposed descendant, unless the new override also call the old method.) |
||
} | ||
|
||
/// <summary> | ||
/// The syntax that is used to close the if for a constraint exists check, used | ||
/// for dialects that requires begin/end for ifs | ||
/// </summary> | ||
/// <param name="catalog">The catalog.</param> | ||
/// <param name="schema">The schema.</param> | ||
/// <param name="table">The table.</param> | ||
/// <param name="name">The name.</param> | ||
/// <returns></returns> | ||
public virtual string GetIfNotExistsCreateConstraintEnd(string catalog, string schema, string table, string name) | ||
{ | ||
return ""; | ||
} | ||
|
||
/// <summary> | ||
/// The syntax that is used to check if a constraint exists before dropping it | ||
/// </summary> | ||
/// <param name="catalog">The catalog.</param> | ||
/// <param name="schema">The schema.</param> | ||
/// <param name="table">The table.</param> | ||
/// <param name="name">The name.</param> | ||
/// <returns></returns> | ||
public virtual string GetIfExistsDropConstraint(string catalog, string schema, string table, string name) | ||
{ | ||
return ""; | ||
} | ||
|
||
/// <summary> | ||
/// The syntax that is used to close the if for a constraint exists check, used | ||
/// for dialects that requires begin/end for ifs | ||
/// </summary> | ||
/// <param name="catalog">The catalog.</param> | ||
/// <param name="schema">The schema.</param> | ||
/// <param name="table">The table.</param> | ||
/// <param name="name">The name.</param> | ||
/// <returns></returns> | ||
public virtual string GetIfExistsDropConstraintEnd(string catalog, string schema, string table, string name) | ||
{ | ||
return ""; | ||
} | ||
|
@@ -1429,7 +1508,7 @@ IEnumerator<SqlString> IEnumerable<SqlString>.GetEnumerator() | |
|
||
public IEnumerator GetEnumerator() | ||
{ | ||
return ((IEnumerable<SqlString>)this).GetEnumerator(); | ||
return ((IEnumerable<SqlString>) this).GetEnumerator(); | ||
} | ||
|
||
public enum TokenizerState | ||
|
@@ -1641,7 +1720,7 @@ public virtual bool IsQuoted(string name) | |
return (name[0] == OpenQuote && name[name.Length - 1] == CloseQuote); | ||
} | ||
|
||
public virtual string Qualify(string catalog, string schema, string table) | ||
public virtual string Qualify(string catalog, string schema, string name) | ||
{ | ||
StringBuilder qualifiedName = new StringBuilder(); | ||
|
||
|
@@ -1653,7 +1732,7 @@ public virtual string Qualify(string catalog, string schema, string table) | |
{ | ||
qualifiedName.Append(schema).Append(StringHelper.Dot); | ||
} | ||
return qualifiedName.Append(table).ToString(); | ||
return qualifiedName.Append(name).ToString(); | ||
} | ||
|
||
/// <summary> | ||
|
@@ -1670,7 +1749,7 @@ public virtual string Qualify(string catalog, string schema, string table) | |
/// </remarks> | ||
protected virtual string Quote(string name) | ||
{ | ||
string quotedName = name.Replace(OpenQuote.ToString(), new string(OpenQuote, 2)); | ||
var quotedName = name.Replace(OpenQuote.ToString(), new string(OpenQuote, 2)); | ||
|
||
// in some dbs the Open and Close Quote are the same chars - if they are | ||
// then we don't have to escape the Close Quote char because we already | ||
|
@@ -1757,6 +1836,24 @@ public virtual string QuoteForSchemaName(string schemaName) | |
return IsQuoted(schemaName) ? schemaName : Quote(schemaName); | ||
} | ||
|
||
/// <summary> | ||
/// Quotes a name for being used as a catalogname | ||
/// </summary> | ||
/// <param name="catalogName">Name of the catalog</param> | ||
/// <returns>A Quoted name in the format of OpenQuote + catalogName + CloseQuote</returns> | ||
/// <remarks> | ||
/// <p> | ||
/// If the catalogName is already enclosed in the OpenQuote and CloseQuote then this | ||
/// method will return the catalogName that was passed in without going through any | ||
/// Quoting process. So if catalogName is passed in already Quoted make sure that | ||
/// you have escaped all of the chars according to your DataBase's specifications. | ||
/// </p> | ||
/// </remarks> | ||
public virtual string QuoteForCatalogName(string catalogName) | ||
{ | ||
return IsQuoted(catalogName) ? catalogName : Quote(catalogName); | ||
} | ||
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. This looks like a general quoting implementation. I definitely will have to check with local sources what is going on with quoting. |
||
|
||
/// <summary> | ||
/// Unquotes and unescapes an already quoted name | ||
/// </summary> | ||
|
@@ -1829,6 +1926,66 @@ public virtual string[] UnQuote(string[] quoted) | |
return unquoted; | ||
} | ||
|
||
/// <summary> | ||
/// Convert back-tilt quotes in a name for being used as an aliasname. | ||
/// </summary> | ||
/// <param name="aliasName">Name of the alias.</param> | ||
/// <returns>A name with back-tilt quotes converted if any.</returns> | ||
public virtual string ConvertQuotesForAliasName(string aliasName) | ||
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. Having X methods doing the same isn't necessary. 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. This is done for consistency with the If we consider this as not necessary, then we should obsolete the We may improve the pattern by calling the corresponding |
||
{ | ||
return StringHelper.IsBackticksEnclosed(aliasName) | ||
? Quote(StringHelper.PurgeBackticksEnclosing(aliasName)) | ||
: aliasName; | ||
} | ||
|
||
/// <summary> | ||
/// Convert back-tilt quotes in a name for being used as a columnname. | ||
/// </summary> | ||
/// <param name="columnName">Name of the column.</param> | ||
/// <returns>A name with back-tilt quotes converted if any.</returns> | ||
public virtual string ConvertQuotesForColumnName(string columnName) | ||
{ | ||
return StringHelper.IsBackticksEnclosed(columnName) | ||
? Quote(columnName) | ||
: columnName; | ||
} | ||
|
||
/// <summary> | ||
/// Convert back-tilt quotes in a name for being used as a tablename. | ||
/// </summary> | ||
/// <param name="tableName">Name of the table.</param> | ||
/// <returns>A name with back-tilt quotes converted if any.</returns> | ||
public virtual string ConvertQuotesForTableName(string tableName) | ||
{ | ||
return StringHelper.IsBackticksEnclosed(tableName) | ||
? Quote(tableName) | ||
: tableName; | ||
} | ||
|
||
/// <summary> | ||
/// Convert back-tilt quotes in a name for being used as a schemaname. | ||
/// </summary> | ||
/// <param name="schemaName">Name of the schema.</param> | ||
/// <returns>A name with back-tilt quotes converted if any.</returns> | ||
public virtual string ConvertQuotesForSchemaName(string schemaName) | ||
{ | ||
return StringHelper.IsBackticksEnclosed(schemaName) | ||
? Quote(schemaName) | ||
: schemaName; | ||
} | ||
|
||
/// <summary> | ||
/// Convert back-tilt quotes in a name for being used as a catalogname. | ||
/// </summary> | ||
/// <param name="catalogName">Name of the catalog.</param> | ||
/// <returns>A name with back-tilt quotes converted if any.</returns> | ||
public virtual string ConvertQuotesForCatalogName(string catalogName) | ||
{ | ||
return StringHelper.IsBackticksEnclosed(catalogName) | ||
? Quote(catalogName) | ||
: catalogName; | ||
} | ||
|
||
#endregion | ||
|
||
#region Union subclass support | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -554,6 +554,19 @@ public override bool DropTemporaryTableAfterUse() | |
return true; | ||
} | ||
|
||
public override string Qualify(string catalog, string schema, string name) | ||
{ | ||
if (!string.IsNullOrEmpty(catalog)) | ||
{ | ||
return string.Join(".", catalog, schema, name); | ||
} | ||
if (!string.IsNullOrEmpty(schema)) | ||
{ | ||
return string.Join(".", schema, name); | ||
} | ||
return name; | ||
} | ||
|
||
/// <summary /> | ||
/// <param name="name"></param> | ||
/// <returns></returns> | ||
|
@@ -621,25 +634,45 @@ public override long TimestampResolutionInTicks | |
} | ||
} | ||
|
||
public override string GetIfExistsDropConstraint(Table table, string name) | ||
public override string GetIfExistsDropConstraint(string catalog, string schema, string tableName, string name) | ||
{ | ||
string selectExistingObject = GetSelectExistingObject(name, table); | ||
string selectExistingObject = GetSelectExistingObject(catalog, schema, tableName, name); | ||
return string.Format(@"if exists ({0})", selectExistingObject); | ||
} | ||
|
||
public override string GetIfNotExistsCreateConstraint(string catalog, string schema, string table, string name) | ||
{ | ||
string selectExistingObject = GetSelectExistingObject(catalog, schema, table, name); | ||
return string.Format(@"if not exists ({0})", selectExistingObject); | ||
} | ||
|
||
// Since v5.1 | ||
[Obsolete("Please use overload with catalog and schema parameters")] | ||
protected virtual string GetSelectExistingObject(string name, Table table) | ||
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. Breaking change, the method must still be there but obsoleted, otherwise it will have to go into 6.0 only. |
||
{ | ||
string objName = table.GetQuotedSchemaName(this) + Quote(name); | ||
return string.Format("select 1 from sysobjects where id = OBJECT_ID(N'{0}') AND parent_obj = OBJECT_ID('{1}')", | ||
objName, table.GetQuotedName(this)); | ||
var catalog = table.GetQuotedCatalog(this, null); | ||
var schema = table.GetQuotedSchema(this, null); | ||
return GetSelectExistingObject(catalog, schema, table.GetQuotedName(), name); | ||
} | ||
|
||
public override string GetIfNotExistsCreateConstraint(Table table, string name) | ||
/// <summary> | ||
/// Returns a string containing the query to check if an object exists | ||
/// </summary> | ||
/// <param name="catalog">The catalong name</param> | ||
/// <param name="schema">The schema name</param> | ||
/// <param name="table">The table name</param> | ||
/// <param name="name">The name of the object</param> | ||
/// <returns></returns> | ||
protected virtual string GetSelectExistingObject(string catalog, string schema, string table, string name) | ||
{ | ||
string selectExistingObject = GetSelectExistingObject(name, table); | ||
return string.Format(@"if not exists ({0})", selectExistingObject); | ||
return | ||
string.Format( | ||
"select 1 from {0} where id = OBJECT_ID(N'{1}') and parent_obj = OBJECT_ID(N'{2}')", | ||
Qualify(catalog, "dbo", "sysobjects"), | ||
Qualify(catalog, schema, Quote(name)), | ||
Qualify(catalog, schema, table)); | ||
} | ||
|
||
[Serializable] | ||
protected class CountBigQueryFunction : ClassicAggregateFunction | ||
{ | ||
|
@@ -733,7 +766,7 @@ public LockHintAppender(MsSql2000Dialect dialect, IDictionary<string, LockMode> | |
// in various kinds of "FROM table1 alias1, table2 alias2". | ||
_matchRegex = new Regex(" (" + aliasesPattern + ")([, ]|$)"); | ||
_unionSubclassRegex = new Regex(@"from\s+\(((?:.|\r|\n)*)\)(?:\s+as)?\s+(?<alias>" + aliasesPattern + ")", RegexOptions.IgnoreCase | RegexOptions.Multiline); | ||
} | ||
} | ||
|
||
public SqlString AppendLockHint(SqlString sql) | ||
{ | ||
|
@@ -743,11 +776,11 @@ public SqlString AppendLockHint(SqlString sql) | |
{ | ||
if (part == Parameter.Placeholder) | ||
{ | ||
result.Add((Parameter)part); | ||
result.Add((Parameter) part); | ||
continue; | ||
} | ||
} | ||
|
||
result.Add(ProcessUnionSubclassCase((string)part) ?? _matchRegex.Replace((string)part, ReplaceMatch)); | ||
result.Add(ProcessUnionSubclassCase((string) part) ?? _matchRegex.Replace((string) part, ReplaceMatch)); | ||
} | ||
|
||
return result.ToSqlString(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,6 +41,7 @@ public class Table : IRelationalModel | |
private IKeyValue idValue; | ||
private bool isAbstract; | ||
private bool isSchemaQuoted; | ||
private bool isCatalogQuoted; | ||
private string name; | ||
private bool quoted; | ||
private string schema; | ||
|
@@ -274,7 +275,18 @@ public bool HasPrimaryKey | |
public string Catalog | ||
{ | ||
get { return catalog; } | ||
set { catalog = value; } | ||
set | ||
{ | ||
if (value != null && value[0] == '`') | ||
{ | ||
isCatalogQuoted = true; | ||
catalog = value.Substring(1, value.Length - 2); | ||
} | ||
else | ||
{ | ||
catalog = value; | ||
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. For correctness, I am tempted to changed that for all of them, by setting their flag to |
||
} | ||
} | ||
} | ||
|
||
public string Comment | ||
|
@@ -318,6 +330,11 @@ public bool IsSchemaQuoted | |
get { return isSchemaQuoted; } | ||
} | ||
|
||
public bool IsCatalogQuoted | ||
{ | ||
get { return isCatalogQuoted; } | ||
} | ||
|
||
#region IRelationalModel Members | ||
|
||
/// <summary> | ||
|
@@ -493,9 +510,9 @@ public virtual string GetQualifiedName(Dialect.Dialect dialect, string defaultCa | |
{ | ||
return "( " + subselect + " )"; | ||
} | ||
string quotedName = GetQuotedName(dialect); | ||
string usedSchema = schema == null ? defaultSchema : GetQuotedSchema(dialect); | ||
string usedCatalog = catalog ?? defaultCatalog; | ||
var quotedName = GetQuotedName(dialect); | ||
var usedSchema = GetQuotedSchema(dialect, defaultSchema); | ||
var usedCatalog = GetQuotedCatalog(dialect, defaultCatalog); | ||
return dialect.Qualify(usedCatalog, usedSchema, quotedName); | ||
} | ||
|
||
|
@@ -528,19 +545,44 @@ public string GetQuotedSchema() | |
|
||
public string GetQuotedSchema(Dialect.Dialect dialect) | ||
{ | ||
return IsSchemaQuoted ? dialect.OpenQuote + schema + dialect.CloseQuote : schema; | ||
return IsSchemaQuoted ? dialect.QuoteForSchemaName(schema) : schema; | ||
} | ||
|
||
public string GetQuotedSchema(Dialect.Dialect dialect, string defaultQuotedSchema) | ||
{ | ||
return schema == null ? defaultQuotedSchema : | ||
GetQuotedSchema(dialect); | ||
} | ||
|
||
/// <summary> returns quoted name as it is in the mapping file.</summary> | ||
public string GetQuotedCatalog() | ||
{ | ||
return IsCatalogQuoted ? "`" + catalog + "`" : catalog; | ||
} | ||
|
||
public string GetQuotedCatalog(Dialect.Dialect dialect) | ||
{ | ||
return IsCatalogQuoted ? dialect.QuoteForCatalogName(catalog) : catalog; | ||
} | ||
|
||
public string GetQuotedCatalog(Dialect.Dialect dialect, string defaultQuotedCatalog) | ||
{ | ||
return catalog == null ? defaultQuotedCatalog : | ||
GetQuotedCatalog(dialect); | ||
} | ||
|
||
/// <summary> | ||
/// Gets the schema for this table in quoted form if it is necessary. | ||
/// </summary> | ||
/// <param name="dialect"> | ||
/// The <see cref="Dialect.Dialect" /> that knows how to quote the table name. | ||
/// The <see cref="Dialect.Dialect" /> that knows how to quote the schema name. | ||
/// </param> | ||
/// <returns> | ||
/// The schema name for this table in a form that is safe to use inside | ||
/// of a SQL statement. Quoted if it needs to be, not quoted if it does not need to be. | ||
/// </returns> | ||
// Since v5.1; back-tilts are removed when storing schema: this thing is non-sens. | ||
[Obsolete("This method is no-op and has no usages")] | ||
public string GetQuotedSchemaName(Dialect.Dialect dialect) | ||
{ | ||
if (schema == null) | ||
|
@@ -727,7 +769,7 @@ public UniqueKey GetOrCreateUniqueKey(string keyName) | |
return uk; | ||
} | ||
|
||
public virtual void CreateForeignKeys() {} | ||
public virtual void CreateForeignKeys() { } | ||
|
||
public virtual ForeignKey CreateForeignKey(string keyName, IEnumerable<Column> keyColumns, string referencedEntityName) | ||
{ | ||
|
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.
Since the called method is public too, it will have to check if handling quoting is required too, for cases where it is called by some other code. So quoting here seems redundant. Same for next methods.
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.
Unless arguments are always supposed to already be with adequate quoting. It seems handled that way.