Add NodaTime.Serialization.ModelBinding

[zaccharles]

Keep in mind this will need some more work, such as tests, comments, and probably reverting some of the changes to the solution that VS2015RC made. I first did this for 2.0.0 so I was using 2015RC. This version is basically the same though; I just couldn't use nameof().

---

This is a proposal for a new package, NodaTime.Serialization.ModelBinding (maybe a different name?), which adds functionality so that Web API can bind URI parameters to Noda Time types.

To do this in a simplistic way, you need to implement an IModelBinder for each Noda Time type, and attach it to said Noda Time type in some way. Since I imagine we don't want to pollute the NodaTime project with System.Web.Http... dependencies, this should be standalone. The normal way to do that is to add a ModelBinderAttribute to each action parameter. Usually this attribute would take the Type of the IModelBinder to use. However, I didn't want users to need to do this.

In fact, I wanted using this package it to be very easy, like the Json.Net one. I got it to the point where you can just call config.ConfigureForNodelTime(DateTimeZoneProviders.Tzdb); on the HttpConfiguration in your startup code. I'm quite happy with this.

So how do we avoid all this attribute nonsense. Well, this package works by replacing the IActionValueBinder service with a NodaActionValueBinder. In case this service has already been replaced by user code, or another package, NodaActionValueBinder wraps the previously registered IActionValueBinder, and passes calls down to it for anything unrelated to Noda Time types.

The IActionValueBinder service is responsible for telling Web API which IModelBinder to use for a given parameter. At startup, it's given each parameter and asked that question. The DefaultActionValueBinder responds this by looking at the ModelBinderAttribute. The NodaActionValueBinder is slightly different. The first thing it does is identify whether the current parameter is a Noda Time type. If it is, it creates a new NodaModelBinderAttribute (which extends ModelBinderAttribute) and attaches it to the property. It then passes the request on to the IActionValueBinder it's wrapping. Now, the IActionValueBinder has a ModelBinderAttribute to look at, and we didn't need to manually add it to every parameter in code.

So why do we need NodaModelBinderAttribute? Well, by default, ModelBinderAttribute is responsible for instantiating the IModelBinder. We don't want this because we want to be able to pass the IDateTimeZoneProvider all the way down from the top.
ModelBinderAttribute first tries to resolve an instance of the IModelBinder from the configured dependency resolver. If that's not possible, it will try to instantiate a new one with Activator.CreateInstance. NodaModelBinderAttribute's job is to temporarily replace the IDependencyResolver used to try and resolve the IModelBinder.

This is where NodaDependencyResolver comes in. It's a simple wrapper for IDependencyResolver. This interface has GetService(Type) which is implemented so that if it's a Noda model binder being resolved, an instance with the correct IDateTimeZoneProvider will be returned. If it's not a Noda model binder, the call will be passed on to the wrapped IDependencyResolver.

The above could probably be avoided via the initial configuration extension method setting a static variable somewhere, and the model binders reading it as necessary. That way all model binders could have parameterless contructors and be instantiated with Activator.CreateInstance. It's simpler, but didn't seem as nice.

Lastly, the model binders are quite simple wrappers for the various IPattern<T> where available, and code ripped from the Json.Net package for other types like Interval and DateTimeZone. I'm not 100% sure I got the behavior correct for the Interval, especially the IsoInterval.

As I said at the top, this is a work in progress, though it does work currently to the extent of my manual testing. I didn't want to go too far in case it's rubbish and I'm crazy. All feedback welcome.

[jskeet]

Hi Zac - thanks very much for this. Will try to review it over the weekend, but it may take a bit longer than that.

[mattjohnsonpint]

The 1.3.x branch should be built with VS2013. 2.0 (master) uses VS 14.

[zaccharles]

@mj1856 I originally did this against 2.0 and then retrofitted it to 1.3.x. As I mentioned in the PR description, I'd fix these solution file / assembly info problems before it would be merged.

[mattjohnsonpint]

Thanks. Also, if I'm understanding correctly - the intention here is to make it easy to bind to things such as querystring parameters. If so, we should probably allow for some variability in how those parameters are formatted. For example, It's fairly common to see the short-form of ISO dates on a URI, such as 20151231 instead of 2015-12-31.

[zaccharles]

I agree that that would be nice, however, to explain: I based the current
version off the Json.Net integration which also has fixed formats. I
suppose we could consider overriding the formats during the
ConfigureForNodaTime call, but then what if someone wants to change it for
a specific parameter. In that case it would be an attribute most likely. I
imagine this is why it was avoided for Json.Net, though that's not a great
reason to avoid it here :)

[zaccharles]

I reverted the solution file and readded the new project, so solution changes are now minimal. I also corrected the version to match the other projects.

[BenJenkinson]

Since I'm about to implement something like this myself, did @jskeet get a chance to review it?

[jskeet]

@BenJenkinson: Sorry, no, I haven't had a chance yet. Any comments you have would be useful, given that you're likely to be more knowledable on the binding code than I am... looks like some more XML documentation is required and I'll need to check the impact it has on the build as well, of course... then we can port it to 2.0 when required.

[jskeet]

Okay, this is taking a really embarrassingly-long time to get to. Realistically, I'm out of my depth anyway.

@BenJenkinson and @mj1856 - are either of you able to do a reasonably-thorough review? I would definitely like to get this in, although I'm still not sure whether doing it in the 1.3.x branch rather than 2.0 is a good idea... I'd mostly considered 1.x to be "done" other than for a 1.4 compatibility release. Having said that, adding a 1.3.1 version of just this package could work too... Worth discussing separately.

[BenJenkinson]

I did try using this in my own project, but I had to make my own anyway as it inherits/implements System.Web.Http IModelBinder rather than the System.Web.Mvc IModelBinder

The (non-vNext) versions of ASP.NET MVC and ASP.NET WebAPI often have identically named classes, but have no shared inherited classes or interfaces, so we'd need an implementation & configuration for each.

[jskeet]

Oh joy. And presumably ASP.NET Core will have other classes as well? This is more complicated than I thought :(

Do we have an example of anyone else doing this that we could learn from?

[BenJenkinson]

I'm not sure what ASP.NET Core will be like, I've held off looking at it until it was more final. But I believe the idea was to reconcile MVC and WebAPI into the same stack, so there should only be one implementation required for that.

At least with MVC/WebApi the methods are usually very similar, the types are just from different namespaces, so you can't write just one class to do both.

[jskeet]

As a thought - I wonder whether it would make sense to separate this from the core Noda Time repository and even organization potentially. It feels like it could have an entirely disparate release schedule, for example - unless it requires changes to Noda Time itself.

(I should potentially have done the same for the NodaTime.Serialization.JsonNet...)

[zaccharles]

It doesn't require any changes to Noda Time itself, so it could potentially go in its own repo.

[BenJenkinson]

I think it could be made its own repo, and its own Nuget package ala NodaTime.Serialization.JsonNet, but I definitely think it should be owned by the NodaTime organisation.

It's always quite annoying if say NodaTime has been updated to 2.2, but then you're stuck waiting for some 3rd-party contributor to get around to updating the ModelBinding NuGet package, so you can't use 2.2

[jskeet]

Okay - that makes sense. I wonder whether I should start a NodaTime.Serialization repo to contain this and the Json.NET project.

We also need to consider the versioning - we could version them in parallel even if there are no new features to require revving, or we could version them separately. Will open a new issue to discuss all of this.

[jskeet]

Okay, ready to start adding stuff to a new repository - could you add the normal licence text to all the source files? That way it's obvious from the logs that you really do agree with that licence :)

[zaccharles]

Yup, sure. I'll still need to do some work breaking this out into different projects for WebAPI and MVC, but probably best to get this in there and then rework it. I'm happy to that.

[zaccharles]

Done @jskeet.

I've noticed the other projects are using project references and the lib folder rather than their own NuGet dependencies. Should I change this one to do the same? Maybe we should worry about that once its in its own repo since there will be more work anyway.

Same goes of the nuspec file, there isn't one, but that will change anyway.

[jskeet]

Go for NuGet dependencies now - that better represents the separation we're effectively introducing between the serialization stuff and Noda Time. Likewise for other dependencies such as NUnit - I've already done that in 2.0.

[zaccharles]

Ah right. That's probably why I did it, as I originally based it off master before realising you use a branch for 1.3.x. I'm still half remembering things as it's been so long.

Anyway, keep me informed. I'm got plenty of spare time and would love to contribute to make this happen :)

[TheBeardedLlama]

I've also started looking at doing this as I'm working on a new project.

Not sure what stage you're at now, but I'd be happy to assist @zaccharles with this effort if my assistance is indeed required. Otherwise, happy to be a guinea pig and reap the rewards!

I'll wedge the bits I need into my project for now and I'll feedback if I come across anything.

PS. we're using nodatime v2 and aspnet core

[lundmikkel]

What is the status of this issue? Are there any other model bindings available for NodaTime?

[jskeet]

@lundmikkel: I haven't done anything with it, I'm afraid. (Any time I do get on Noda Time is pushing towards the 2.0 release...)

[lundmikkel]

@jskeet Yeah, don't think about it. Even I would rather have you work on version 2.0 😃

[jskeet]

Assuming the code in here is still useful, it should be moved to a PR in https://github.com/nodatime/nodatime.serialization - I'm going to close this PR.

@zaccharles Is this still code you're interested in getting into a public build, or have you moved on? (Given the time frame, that would be entirely understandable...)

[zaccharles]

@jskeet It's been almost two years, but I'd still like to try and follow it through to completion. I haven't done anything in this area for a while, so I'll need to refresh.

I've just created an issue (nodatime/nodatime.serialization#3) to track progress in the new repo.