Added support to set cache timeout in milliseconds

[onetocny]

As described in #263 it is quite limiting to be able to set up cache timeout only in seconds. So that I have added new TimeSpan properties to CacheOutputAttribute to set client, server and shared timeout. Also usage of ShortTime was replaced by new implementation of IModelQuery<DateTime, CacheTime> called PreciseTime that takes TimeSpan as constructor parameters.

[filipw]

Thanks. The main problem is TimeSpan is not a compile time constant so it cannot be used in an attribute declaration.
I would probably rather see an extra property exposing milliseconds.

[onetocny]

Sorry I did not realize that. I was trying to come up with something complex that would cover most of scenarios. What do you think about string properties that would be parsed into TimeSpan internally (see https://docs.microsoft.com/en-us/dotnet/api/system.timespan.parseexact?view=net-5.0).

[filipw]

I would still prefer to just have another set of properties that allow you to set milliseconds as ints.
Otherwise there is no compile time safety anyway, as invalid strings can blow up the timespans.

[onetocny]

No problem, you are the boss here :-). There will be enough flexibility even with milliseconds. May I suggest ulong as data type for these new properties?

[filipw]

yes that would make sense. I would make it nullable too. This way if you don't set it, the existing properties kick in automatically, and if you set the new milliseconds ones, they'd take precedence over the old ones (btw it's a bit unfortunate that the existing ones are not nullable too).

We probably also need a comment on both the old properties and the new ones explaining this order of precedence.

[onetocny]

I think that nullable is not necessary. In my PR both old and new properties points to backing fields that are TimeSpan. If you do not set neither new nor old one property backing field will keep its default value that is zero. Agree that comments and docs should be extended to prevent people from confusion.

[filipw]

yes I get that but what if you set both? it should be deterministic

[onetocny]

Sorry I probably do not see your point here. How would nullable solve the undeterministic behavior when you set both secs and millis?

[filipw]

• what happens when you set [CacheOutput(ServerTimeSpan = 10, ServerTimeSpanMilliseconds = 5000)]. Is it 5 seconds or 10 seconds? does it behave the same way on all compiler versions?
• what happens when you set [CacheOutput(ServerTimeSpanMilliseconds = 5000)]. Is it 5 seconds or 0 seconds?
• what happens when you set [CacheOutput(ServerTimeSpan = 5, ServerTimeSpanMilliseconds = 0)]. Is it 5 seconds or 0 seconds?

if the new one is nullable you at least know that it was set or not set and can define behvioral rules based on that

[onetocny]

I see your point that if someone set both secs and millis it is not clear what should have precedence. But nullable does not solve it imho. Event when you make one of the properties nullable. You will still have to face the case that you have described [CacheOutput(ServerTimeSpan = 10, ServerTimeSpanMilliseconds = 5000)]. You will have two values and decide which one you would like to use. It migh be solved by one of following:

• In docs and comments specify that secs have precendence over millis or vice versa if both are non zero and implement that in code (just compare it to zero).
• In docs and comments specify that this scenario (both millis and secs are non zero) is not defined.
• Change current props to double and do not introduce new ones. Double can be multiplied by 1000 and casted to ulong which would server as timeout in millis.
• Something else?

[onetocny]

@filipw Is there anything I could do to move this PR forward?

[filipw]

The problem in what you are proposing is that you have no way of knowing if the 0 on milliseconds is intentional (set by user) or default value.

What I would like to see is as follows:

• milliseconds are nullable
• if they are null, existing property is used
• if both are set, milliseconds are used as they are given precedence

This is less ambiguous.

[filipw]

thanks!

[onetocny]

Thanks for cooperation. Glad to contribute.

[onetocny]

@filipw As you approved this PR would you please merge it and release new version of nuget?