Support CacheMode in QueryBatch

[fredericDelaporte]

And add a test for ReadOnly

[bahusoid]

As this code is used more than once and it's not really one-liner I would wrap it in IDisposable class. Something like:

class SessionContext : IDisposable
{
	private readonly ISession _session;
	private readonly CacheMode? _cacheModeToReturn;

	public SessionContext(ISession session, CacheMode? cacheMode)
	{
		if (cacheMode.HasValue && cacheMode != session.CacheMode)
		{
			_cacheModeToReturn = session.CacheMode;
			_session.CacheMode = cacheMode.Value;
		}
	}

	public void Dispose()
	{
		if(_cacheModeToReturn.HasValue)
			_session.CacheMode = _cacheModeToReturn.Value;
	}
}

So usage in code would be more concise:

using(new SessionContext(Session, _cacheMode)
...

I think this class can also be made private in QueryParameters and exposed only via method. Something like:

using(queryParams.FillSessionContext(session))

So it can be extended easily with filling some other context fields if added. But not sure if it's really usefull.

[hazzik]

Agree.

[fredericDelaporte]

I thought about it but considered not really worth it for so few cases. I will add it.

[bahusoid]

IMHO it's better to end with "," where allowed.
So it's less change noise when new initialization property is added. And sometimes it even allows to skip merge conflicts.

[fredericDelaporte]

It does not look to me as an usual practice in NHibernate current code base, and while I understand the rationals of it, I still dislike it a bit. So I do not tend to follow this practice.

[bahusoid]

session == null check looks unnecessary in internal method. Unless it's a live scenario I think it's actually better to simply allow it to throw NRE on session.CacheMode access.

[fredericDelaporte]

If it has to change I would instead throw an ArgumentNullException. I do not want to code NRE cases. Anyway that part will go away once merged into ISessionImplementor.

[bahusoid]

Ah. Ok. I've missed the fact that you are actually planning to include this method in interface.

[bahusoid]

But then.. this session == null check only makes things worse. If there is such case in code - it will be silently ignored now. But when merged to interface - NRE will happen.

[fredericDelaporte]

That is true, but it is done also on BeginContext and BeginProcess right above.

[bahusoid]

And is it really required to go in interface? It seems to me that it can live as internal static extension. It doesn't depend on any specific implementation. So why to clutter interface with something that can be implemented on top of existing members?

[fredericDelaporte]

It is a possibility.

[bahusoid]

Did you mean "unexpected query cache miss"? "query hit" is a bit unclear error message considering that in previous check above it's also used but for different condition.

[fredericDelaporte]

I generally use the label for identifying which assert has failed. That is the assert on query hit. I do not think this label has to describe what is the failure. That would be already interpreting it, and sometimes it could be off the actual cause.

[bahusoid]

lgtm
The only thing: if you agree that we don't need to expose SwitchCacheMode in ISessionImplementor and keep it as internal extension we should somehow make this intention obvious. Maybe move it to separate partial SessionImplementorExtensions class that's not commented as "6.0 TODO: Convert to interface methods"