NH-1285 - Drop/Create script with default_schema/default_catalog fix(SqlServer)

[lnu]

https://nhibernate.jira.com/browse/NH-1285
related also to https://nhibernate.jira.com/browse/NH-1443 (both already closed) and #987.
In case the Schema is set using the Default_Schema property, the drop script for the generated constraint is wrong: the schema is missing(Sql Server).

[hazzik]

@lnu can you please avoid changing method signatures of Dialect class? Preferable create new virtual methods and use them, make old methods using new methods (or vice verse) and mark old methods as obsolete.

[lnu]

Ok i'll check for this

On Thu, Dec 3, 2015 at 2:08 AM, Alexander Zaytsev notifications@github.com
wrote:

@lnu https://github.com/lnu can you please avoid changing method
signatures of Dialect class? Preferable create new virtual methods and
use them, make old methods using new methods (or vice verse) and mark old
methods as obsolete.

—
Reply to this email directly or view it on GitHub
#448 (comment)
.

[lnu]

I did some changes. Is it what you were thinking about?

[hazzik]

Is it what you were thinking about?

Almost,

You forgot about this part: "make old methods using new methods (or vice verse)". The idea is that if you use not updated provider, it should still work as expected.

[hazzik]

Here you need to call GetIfExistsDropConstraintEnd(table, name), for example

[hazzik]

@lnu does it actually work for SQL2000? Shouldn't it be the same?

[lnu]

There is no schema in SqlServer 2000. It was introduced in 2005. no?
How does it work if you use a schema with sql 2000?

[hazzik]

@lnu true, it does not have a schema.

Also, the same issue exists with defaultCatalog, so, i think the code of ForeignKey and other constraints should be changed to something like this:

public override string SqlDropString(Dialect.Dialect dialect, string defaultCatalog, string defaultSchema)
{
    var tableName = Table.GetQualifiedName(dialect, defaultCatalog, defaultSchema);

    var ifExists = dialect.GetIfExistsDropConstraint(tableName, Name);
    string drop = string.Format("alter table {0} {1}", tableName, dialect.GetDropForeignKeyConstraintString(Name));
    string end = dialect.GetIfExistsDropConstraintEnd(tableName, Name);
    return ifExists + System.Environment.NewLine + drop + System.Environment.NewLine + end;
}

[lnu]

Ok...
I've checked multiple scenarios with catalog and schema and there are definitely more bugs than my initial one.
For example, the catalog is not quoted correctly if defined in mapping but works with default_catalog

[lnu]

@hazzik Does it look for you now?
Now everything is quoted correctly including the catalog (default or not). The catalog is also used when querying, which was not the case before.
ps:I rebased and squashed everything.

[hazzik]

Loud thinking:

Hi @lnu. can we move all this to the Dialect? We really need to have one method, which will generate the constraint. Reason for that, is that MS SQL does support "if exist" statement natively. Also the GetIfExistsDropConstraintEnd method looks more like a workaround than a solution.

[lnu]

It could be part of a refactoring. I think we should just close this one with the current solution.

[hazzik]

Looks good. I will review the PR shortly. Please ignore my last comment for now, as it is just a loud thinking.

[hazzik]

NH-2288 tests are failing now

[hazzik]

Is this necessary to quote everything?

[lnu]

if the catalog/schema is not quoted and a reserved keyword is used it will fail. Don't you think?

[fredericDelaporte]

It seems to me this code only handles conversion from NHibernate back-tilt quoting to dialect quoting. It will not quote an unquoted string. So if a reserved word is used without native quoting nor NHibernate quoting, it will fail. But that is not much an issue, the user just have to add quotes himself for such case. Otherwise if you want to handle that case, you need to look at the auto-quoting option, which, if enabled, does quote according to the dictionary of reserved words for the dialect.

By the way it looks strange to me to have to code that here. Some method should already supply that quote conversion somewhere else in NHibernate.

[hazzik]

It's kind-of correct behavior. On the Hibernate side they started conversion to "quotable identifier" value object for names (but did not finish, as usual)

[oskarb]

I kind of wish this had been done as a series of commits - I get the impression there are multiple issues and quoting of catalog doesn't really concern if schema is missing or not, does it? Not saying it's worth the effort to split it up now though.

I'm unsure what the repercussions of changing the signature of the overloads in the concrete dialects with regards to API stability. Any idea if that could be an issue?

Does Hibernate have the same bugs, or how have they been fixed there?

[hazzik]

There are some divergences between NHibernate and Hibernate. This feature was heavily modified to enable it for SQL Server. Another issue, that we need to refactor the feature to enable Sql Server 2016 native if exist clause.

[hazzik]

Having X methods doing the same isn't necessary.

[fredericDelaporte]

This is done for consistency with the QuoteFor series, which does exactly the same. I think the idea is to allow a dialect to override some but not all of them with a different implementation, in case a database may have different quoting rules depending of the quoted object type.

If we consider this as not necessary, then we should obsolete the QuoteFor series by the way and keep only a general purpose QuoteFor.

We may improve the pattern by calling the corresponding QuoteFor from ConvertQuotesFor, in order to allow to override only one method instead of both. (But this will cause the convert to do the additional check done in quote for, which it does not need.)

[hazzik]

Could the unnecessary whitespace changes be reverted (especially in Configuration.cs)?
Also if we want #956 then this would require more systematic approach.

[fredericDelaporte]

Could the unnecessary whitespace changes be reverted (especially in Configuration.cs)?

I have taken the time to review them all. I am favorable to let them.

[fredericDelaporte]

I realize a bit late that we have a breaking change here.
virtual string GetIfNotExistsCreateConstraint(Table table, string name) has been obsoleted in favor of this new virtual string GetIfNotExistsCreateConstraint(string catalog, string schema, string table, string name) method, which is now used instead of the old in NHibernate code base.

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 GetIfNotExistsCreateConstraint, since they do not override it. And so such dialects will no more be able to create constraints.

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...
But in the case of this PR, removing these new overrides cannot be done without undoing the PR.

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.)

[fredericDelaporte]

Possible breaking changes:

• Overridable methods have been obsoleted and their overrides will no more have effects. Overrides of their replacements must be added in the impacted deriving classes. Obsoleted overridable methods are:
  • Dialect.GetIfNotExistsCreateConstraint(Table table, string name), replaced by GetIfNotExistsCreateConstraint(string catalog, string schema, string table, string name).
  • Dialect.GetIfNotExistsCreateConstraintEnd(Table table, string name), replaced by GetIfNotExistsCreateConstraintEnd(string catalog, string schema, string table, string name).
  • Dialect.GetIfExistsDropConstraint(Table table, string name), replaced by GetIfExistsDropConstraint(string catalog, string schema, string table, string name).
  • Dialect.GetIfExistsDropConstraintEnd(Table table, string name), replaced by GetIfExistsDropConstraintEnd(string catalog, string schema, string table, string name).
  • MsSql2000Dialect.GetSelectExistingObject(string name, Table table), replaced by GetSelectExistingObject(string catalog, string schema, string table, string name).