Loquatious QueryCache constraint should be an IQueryCacheFactory constraint

[InlineAsm]

The configuration setting is used to create an instance of a factory, not of the query cache itself.

[fredericDelaporte]

In other words, you mean that the current loquacious setting for enabling the query cache is not usable, because it forces to set an invalid query cache factory by setting the query cache instead as a factory.

[InlineAsm]

In other words, you mean that the current loquacious setting for enabling the query cache is not usable, because it forces to set an invalid query cache factory by setting the query cache instead as a factory.

Yes. That's what I meant. I fixed the unit tests too,

Hang on. I accidentally included more than I should when I pushed again.

[fredericDelaporte]

Yes. That's what I meant.

Ok, then I think this also showcases a lack of test. There, the loquacious tests seems to be only unit tests, while lot of tests in NHibernate are testing the whole stack. Adding such a "whole stack" test for demonstrating this loquacious shortcoming about query cache would be better. (Here, it is likely enough to just try building the session factory from the configuration; it is probably not needed to go as far as hitting the db with some queries.)

About the fix itself, in its current form, it is a binary breaking change, which can not be merged in upcoming 5.2 version.
For merging it in a minor release, the wrong methods should be obsoleted with an appropriate message and, in this case, the flag telling to the compiler that using them is to be treated as an error. The new methods have to be added as extension methods instead of being directly added into the interface, with a comment like // 6.0 TODO: merge into the interface. (We allow binary breaking changes for major releases.)

[InlineAsm]

About the fix itself, in its current form, it is a binary breaking change, which can not be merged in upcoming 5.2 version.

I doubt if the impact would be that big. The previous version of the method(s) would not have worked so I don't think anyone is using it.

We worked around the issue by setting the configuration manually:
rawConfig.SetProperty(Environment.QueryCacheFactory, typeof(AdministrationAwareQueryCacheProviderFactory).AssemblyQualifiedName);

In the current version when you would pass a class implementing only IQueryCache NHibernate would have crashed while building the sessionfactory.

The new methods have to be added as extension methods instead of being directly added into the interface, with a comment like // 6.0 TODO: merge into the interface. (We allow binary breaking changes for major releases.)

I tried this but I don't see how an extension method would gain access to the configuration. Also changing the constraint is not enough to uniquely identify the method. You would also need to come up with a new name for the method.

I guess you could wait with merging until you reach 6.0 since a workaround is available.

[hazzik]

@fredericDelaporte I've checked our usual solution does not work - the interface method takes precedence over the extension. I think the only thing we can do is to remove constraint completely (and reintroduce it in 6.0), probably add a runtime check instead. This way it should not be a breaking change.

[fredericDelaporte]

Something like InlineAsm#1?
I think this is still at least a compile-time breaking change, but maybe not a binary breaking one. So why not.

[InlineAsm]

Thanks for the review. I looked at your changes and they look good to me.

How should I proceed? Should I squash the commits and update this PR with your changes included?

[fredericDelaporte]

The Github option "Rebase and merge" on my PR should do what is needed.

[InlineAsm]

Ok, done.

[fredericDelaporte]

Thanks for contributing this @InlineAsm.