Model binding support for fields #1613

logiclrd · 2014-07-15T19:53:14Z

This commit adds support to the ModelBinding code for binding fields, not just properties, in models. This allows the POD types for a project to look like:

public class Employee
{
  public int EmployeeID;
  public string Name;
  public string PhoneNumber;
  public decimal Salary;
}

There is a good reason to write POD types this way, rather than using implicit property implementations. The last paragraph below explains it, though it is not strictly pertinent to this pull request.

This commit includes full unit testing of this change, including all of the necessary changes to existing tests.

Implicit property implementations create backing fields that include < and > characters, in order to ensure that you cannot, in your own C# code, ever create another member with the same name. This, in turn, causes performance issues if DataContractSerializer is used on the same type. DataContractSerializer works like a traditional serializer, enumerating the private members of the type. It then tries to create XML elements named after these members. If the members contain < and > characters, an XmlException is produced. This exception can be seen by running code in a debugger set to break on all thrown exceptions, even if they're handled. The DataContractSerializer code is aware of this problem and handles the exception internally by doing additional escaping on the member name, but by then the performance hit of the exception has already been incurred. The only ways to avoid this exception are to either expand out the property definitions so that you control the name of the backing field, or to not use properties at all, and the latter solution is much more concise and has no downsides when the type really is just a POD type.

# Breaking Change

The specific way in which this change is breaking is that in the past, fields within models would simply be ignored by Nancy's model binding. Thus, if you had a mix of properties and fields, assuming that the properties would receive bound values and the fields would be ignored, starting with the release including this pull request that is no longer the case.

albertjan · 2014-07-16T08:22:24Z

src/Nancy.Tests/Unit/ModelBinding/BindingPropertyInfoFixture.cs

+
+			foreach (var propInfo in properties)
+			{
+				if (propInfo.Name.StartsWith("Int"))


Add curly braces.

albertjan · 2014-07-16T08:26:19Z

BindingPropertyInfo.cs Has tabs instead of spaces. Same goes for the tests.

albertjan · 2014-07-16T08:26:59Z

src/Nancy/ModelBinding/BindingPropertyInfo.cs

+		/// <param name="newValue">The value to assign in the specified object to this BindingPropertyInfo's property or field.</param>
+		public void SetValue(object destinationObject, object newValue)
+		{
+			if (_propertyInfo != null)


Add curly braces please 😄

khellang · 2014-07-16T09:54:13Z

Maybe you should read the contributing guidelines and read some code around the codebase to get familiar with the existing style guidelines etc. 😄

logiclrd · 2014-07-16T15:22:21Z

Wow, super fast response! :-D

I thought I'd seen some inconsistency here and there in the codebase, but you're right, I could have done a lot better at being consistent with adjacent files (though I was super careful not to mix spaces and tabs in existing files! :-) ).

I'll fix it ASAP. Sorry about that!

logiclrd · 2014-07-16T15:52:44Z

I've done a commit that makes the necessary changes to bring the files in line with the style guidelines, as well as requests on this discussion. I also applied the new method names I proposed in the thread about GetProperties' name, but I can of course change them again easily if you'd prefer different names.

logiclrd · 2014-07-16T20:59:06Z

Wait, what? double take

I swear I changed that file. Something must have changed it back before I committed.

How embarrassing :-P

logiclrd · 2014-07-16T20:59:59Z

Okay, sorry about that. Try now!

albertjan · 2014-07-17T21:15:55Z

src/Nancy/ModelBinding/BindingPropertyInfo.cs

+    /// <summary>
+    /// Represents a bindable member of a type, which can be a property or a field.
+    /// </summary>
+    public class BindingPropertyInfo


I think this should also be renamed. I don't know what exactly maybe something along the lines of GenericValueBindingInfo or GenericMemberInfo.

I think BindingMemberInfo should be OK, even though it's not used on all types of members.

Okay, for sure. Sorry, I only just saw these comments. I'll aim to have that committed within a day or two.

I think that ought to do it. :-) Let me know if any other tweaks are required.

logiclrd · 2014-07-29T07:43:40Z

Okay, 'sall fixed up and good to go. :-) The build error was caused by a test that had been commented out at the time of my last commit. It contained a reference to one of the classes this pull request changes, and it got reinstated upstream since that last commit. All sorted out.

Let me know if any further tweaks are needed. :-)

albertjan · 2014-07-29T09:04:35Z

Looks good to me

khellang · 2014-07-29T16:37:42Z

Thanks for reviewing, @albertjan ❤️

@logiclrd I'd like to ask one last thing; could you manage to squash this into the first commit? There's a lot of "oh shit" commits in here right now 😉 I'd do it myself while merging, but I'm on vacation and can't do interactive rebases with my phone 😝 After that, I think this is ready to 🚢

logiclrd · 2014-07-29T17:41:00Z

Okay, I think I got it all figured out. I've semantically merged the commit messages (e.g. it now refers directly to BindingMemberInfo and not BindingPropertyInfo), and a diff of the previous branch pointer and the current one with the squashed commit shows no differences.

You may merge at your leisure. :-)

logiclrd · 2014-07-31T17:42:51Z

Other commits upstream have produced merge conflicts. I will rebase.

albertjan · 2014-07-31T17:48:11Z

Thanks

Sent from my iPhone

On 31 jul. 2014, at 19:43, "logiclrd" notifications@github.com wrote:

Other commits upstream have produced merge conflicts. I will rebase.

—
Reply to this email directly or view it on GitHub.

logiclrd · 2014-07-31T17:56:58Z

Okay, all sorted out. A new property was added to the model in DefaultBinderFixture.cs, and my commit adds corresponding fields, so I've added a field for the new property as well and updated the affected tests to match.

logiclrd · 2014-08-15T15:53:41Z

I have discovered that a unit test has been added upstream based on the old BindingContext definition. The fix, which this branch already applies to a number of other tests, is pretty simple: The ValidModelProperties field now needs to be initialized with BindingMemberInfo.Collect<TestModel>(), instead of directly obtaining PropertyInfo objects via reflection.

Should I apply a fix in this branch again?

Many thanks :-)

horsdal · 2014-08-21T06:39:04Z

Do you mean apply the fix to the test upstream that you discover? Yes, I think it would be nice you did.

logiclrd · 2014-08-24T05:26:27Z

Build and tests pass once again. :-)

khellang · 2014-08-24T20:42:46Z

Woah. It seems you forgot to edit the commit message when you rebased. It would be great to have a nice, short, descriptive commit message before we merge this 😄 (nitpicking here 😉)

logiclrd · 2014-08-25T02:26:00Z

...can't tell if you're being serious or not. :-)

It's true that the commit message is unchange from the previous configuration of the branch (I deleted the message details from the follow-up commit in the squash), but only because the details of the follow-up commit are already covered by existing text in the commit message. :-)

khellang · 2014-08-25T08:01:41Z

I'll let you decide whether you think this is a "nice, short, descriptive commit message" 😉

To me, it looks like a wall of text 😜

logiclrd · 2014-08-25T14:59:44Z

Hmm, okay :-) Well, the first line is nice, short and describes the entire change to a T, but I can condense the remainder of the text by removing all the actual details and leaving only the conceptual change. I am in the habit of writing commit messages like you see there so that they might be searchable in an index of commit messages, but if it's not appropriate for this project then I will change it. :-)

logiclrd · 2014-08-25T18:13:15Z

How does this look? :-)

logiclrd · 2014-08-25T18:52:01Z

Er, okay, not sure why that build failed. I only edited the commit message, I swear! :-P

Looking at the details now...

logiclrd · 2014-08-25T19:16:20Z

Okay, the first error I see in the build log is complaining that string has no method ShouldContain, which is added by ShouldExtensions.cs in the same project. Could it be that the build script is trying to use a .NET 2.0 instead of .NET 4.0 compiler? Not sure why that would have changed since the previous build attempt... Also, the log does explicitly show it calling /usr/bin/dmcs.

On my machine, in Visual Studio 2012, the project builds without issues.

khellang · 2014-08-25T20:10:59Z

src/Nancy/ModelBinding/BindingContext.cs

@@ -37,7 +37,7 @@ public class BindingContext
        /// <summary>
        /// DestinationType properties that are not black listed
        /// </summary>
-        public IEnumerable<PropertyInfo> ValidModelProperties { get; set; }
+        public IEnumerable<BindingMemberInfo> ValidModelProperties { get; set; }


Just for completeness, should this be called ValidModelMembers since it's not only properties anymore?

Can be done. :-)

How about ValidModelBindingMembers instead, since it's an IEnumerable<BindingMemberInfo>, not an IEnumerable<MemberInfo>?

Doesn't matter to me, though it sounds like smurf naming... It's all in a binding context, I'm not sure we need to prefix everything with Binding* 😉

Replaced references to PropertyInfo in the binding code with a new class BindingMemberInfo, which can refer to either PropertyInfo or FieldInfo objects. Turned the reflection code into a utility method Collect() on the new class. Updated existing unit tests and added new ones.

logiclrd · 2014-08-26T15:38:13Z

Build passed :-) Did you see the previous one failing? I'm curious as to why, if you know. I came back today to find it magically transmuted to passing. :-)

khellang · 2014-08-26T20:23:12Z

I restarted the build. It's just randomly failing. Just some Mono issues or something.

logiclrd · 2014-08-26T20:33:11Z

Ah, cool. :-)

logiclrd · 2014-09-22T15:10:30Z

Are there any remaining issues to resolve in this pull request? If not, is it on a timeline to be merged in?

I've just verified that it still merges cleanly with all tests passing. :-)

Model binding support for fields

khellang · 2014-09-23T07:22:42Z

Thanks @logiclrd! ❤️

logiclrd · 2014-09-23T15:48:25Z

Not at all, thank you! :-D

logiclrd · 2014-09-23T16:05:08Z

I have updated the documentation of model binding to include a section on fields vs. properties. It indicates that Nancy supports both, gives an example of two classes that will both bind in the same manner, and includes a discussion of why one would choose one implementation style over the other. :-)

logiclrd · 2014-09-23T16:06:43Z

In case anyone follows a link to this pull request from the "Breaking Changes" section of the documentation, the specific way in which this change is breaking is that in the past, fields within models would simply be ignored by Nancy's model binding. Thus, if you had a mix of properties and fields, assuming that the properties would receive bound values and the fields would be ignored, starting with the release including this pull request that is no longer the case.

ClemPi · 2014-11-18T10:04:47Z

This patch is breaking https://github.com/NancyFx/Nancy.Serialization.JsonNet. Can someone have a look ? Should I create a new issue on either project ?

albertjan · 2014-11-18T10:29:28Z

@ClemPi It'd be a good idea to open up a new issue and add a description on how you can reproduce the error you're seeing. A stacktrace'd be nice too.

ClemPi · 2014-11-18T11:09:12Z

Thanks, done here : NancyFx/Nancy.Serialization.JsonNet#15.

albertjan reviewed Jul 16, 2014
View reviewed changes

albertjan reviewed Jul 17, 2014
View reviewed changes

logiclrd force-pushed the bind-fields branch from 315866b to 0ab06a1 Compare August 24, 2014 05:18

logiclrd force-pushed the bind-fields branch from 0ab06a1 to 547337e Compare August 25, 2014 18:12

khellang reviewed Aug 25, 2014
View reviewed changes

logiclrd force-pushed the bind-fields branch from 547337e to 353598d Compare August 26, 2014 15:11

khellang added Breaking Change Update Documentation labels Sep 23, 2014

khellang added this to the 1.0 Alpha milestone Sep 23, 2014

khellang added a commit that referenced this pull request Sep 23, 2014

Merge pull request #1613 from logiclrd/bind-fields

d8ca755

Model binding support for fields

khellang merged commit d8ca755 into NancyFx:master Sep 23, 2014

ClemPi mentioned this pull request Nov 18, 2014

Model binding support for fields support in Nancy is breaking compilation NancyFx/Nancy.Serialization.JsonNet#15

Closed

khellang removed the Update Documentation label Jan 26, 2015

grumpydev modified the milestones: 1.0 Alpha, 1.0 Feb 4, 2015

Model binding support for fields #1613

Model binding support for fields #1613

Conversation

logiclrd commented Jul 15, 2014

Choose a reason for hiding this comment

albertjan commented Jul 16, 2014

Choose a reason for hiding this comment

khellang commented Jul 16, 2014

logiclrd commented Jul 16, 2014

logiclrd commented Jul 16, 2014

logiclrd commented Jul 16, 2014

logiclrd commented Jul 16, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

logiclrd commented Jul 29, 2014

albertjan commented Jul 29, 2014

khellang commented Jul 29, 2014

logiclrd commented Jul 29, 2014

logiclrd commented Jul 31, 2014

albertjan commented Jul 31, 2014

logiclrd commented Jul 31, 2014

logiclrd commented Aug 15, 2014

horsdal commented Aug 21, 2014

logiclrd commented Aug 24, 2014

khellang commented Aug 24, 2014

logiclrd commented Aug 25, 2014

khellang commented Aug 25, 2014

logiclrd commented Aug 25, 2014

logiclrd commented Aug 25, 2014

logiclrd commented Aug 25, 2014

logiclrd commented Aug 25, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

logiclrd commented Aug 26, 2014

khellang commented Aug 26, 2014

logiclrd commented Aug 26, 2014

logiclrd commented Sep 22, 2014

khellang commented Sep 23, 2014

logiclrd commented Sep 23, 2014

logiclrd commented Sep 23, 2014

logiclrd commented Sep 23, 2014

ClemPi commented Nov 18, 2014

albertjan commented Nov 18, 2014

ClemPi commented Nov 18, 2014