Can I get a volunteer to help fix #286?

25 views
Skip to first unread message

Jeremy D. Miller

unread,
Apr 25, 2012, 8:37:45 PM4/25/12
to FubuMVC Develop
Guys,

I'm horribly swamped right now until this weekend.  Can I get someone to help me trace down what's wrong with issue #286?


Might be as simple as just making a new PropertyBinder that gets included by default that deals with HttpDocument files.  I'd start by looking at an old version of AspNetAggregateDictionary (no longer exists) and see what got cut when I moved things to the new structure?  Everything looks fine from a glance.  Still looks like the HttpDocumentCollectionBase is used as part of the AspNetRequestData.  I'm not seeing an obvious problem here.

Thanks,
 
Jeremy D. Miller
The Shade Tree Developer
jeremy...@yahoo.com

Chad Myers

unread,
Apr 25, 2012, 8:53:18 PM4/25/12
to fubumv...@googlegroups.com
On it.

-Chad

--
You received this message because you are subscribed to the Google Groups "FubuMVC Development Group" group.
To post to this group, send email to fubumv...@googlegroups.com.
To unsubscribe from this group, send email to fubumvc-deve...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/fubumvc-devel?hl=en.

Chad Myers

unread,
Apr 25, 2012, 9:41:11 PM4/25/12
to fubumv...@googlegroups.com
Isolated the problem:

IFubuRequest.ProblemsFor<YourModel>() has the errors (hey! the model binding Problems thing paid off!):

System.ArgumentException: Object of type 'FubuCore.Binding.BindingValue' cannot be converted to type 'System.Web.HttpFileCollectionWrapper'.

Working on the resolution now...

-Chad

Jeremy D. Miller

unread,
Apr 25, 2012, 9:45:56 PM4/25/12
to fubumv...@googlegroups.com
Pfft.  Someplace takes System.Object, and changing the signature to receiving BindingValue instead of "object" doesn't get caught be the compiler, right?  Just remember to use BindingValue.RawValue instead of passing BindingValue in as is and you should be good to go.  Hopefully I got all the other places where that was an issue, but for some reason we probably don't have a unit test on this usage of basically untestable ASP.Net goop.

FYI, the point of the change to BindingValue was to enable full traceability and diagnostics into the model binding.  Won't matter much until FubuMVC gets some diagnostic improvements, but I still think it'll be a big deal when that hits.


Thanks Chad,
 

From: Chad Myers <chad....@gmail.com>
To: fubumv...@googlegroups.com
Sent: Wed, April 25, 2012 8:41:14 PM
Subject: Re: [fubumvc] Can I get a volunteer to help fix #286?

Chad Myers

unread,
Apr 25, 2012, 9:57:04 PM4/25/12
to fubumv...@googlegroups.com
I can see the value in the BindingValue thing. Looks cool. More context is always good!

The exception is happening because ConversionPropertyBinder is taking the raw output from PassthroughConverter<> (which is a BindingValue) and trying to set it directly on my input model.

PassthroughConverter<> is involved because the AspNetPassthroughConverter sets it up during model binding.

I made a dumb "BindingValueAwarePassthroughConverter that does:

public override object Convert(IPropertyContext context)
        {
            var value = context.RawValueFromRequest;
            if (value is BindingValue) return value.RawValue;
            return value;
        }

And that worked.

Is the fix in PassthroughConverter?  I'm not up to speed on the new binding stuff, so I'm not sure where the correct fix should be.

-Chad

Chad Myers

unread,
Apr 25, 2012, 9:58:14 PM4/25/12
to fubumv...@googlegroups.com
Derp... "is the fix in PassthroughConverter" should be:   "Should the proper fix be put in PassthroughConverter, or somewhere else?"

-Chad

Jeremy D. Miller

unread,
Apr 25, 2012, 9:58:47 PM4/25/12
to fubumv...@googlegroups.com
I think you need to change passthru binder itself, yes.
 
Sent: Wed, April 25, 2012 8:57:08 PM

Chad Myers

unread,
Apr 25, 2012, 10:22:11 PM4/25/12
to fubumv...@googlegroups.com
Going through FubuCore, I see three places that are not BindingValue aware:

BasicConverterFamily
ConversionPropertyBinder
PassthroughConverter<>

I'm going to fix all three.

-Chad

Jeremy D. Miller

unread,
Apr 25, 2012, 10:32:57 PM4/25/12
to fubumv...@googlegroups.com
Hold on there partner, BasicConverterFamily / ConversionPropertyBinder are passing all the end to end tests and unit tests and *that's* well covered.  Look closer at the RawValueFromRequest property getting throw around in there and you'll see where it consumes BindingValue correctly.
 
Sent: Wed, April 25, 2012 9:22:17 PM

Chad Myers

unread,
Apr 25, 2012, 10:39:49 PM4/25/12
to fubumv...@googlegroups.com
BasicConverterFamily had one test on it, or am I missing something?

BasicConverterFamily was also doing logic basic on IPropertyBindingContext.RawRequestValue.GetType() (which will always be BindingValue, so something is amiss here).

Check this out (from BasicConverterFamily):

return context.RawValueFromRequest.GetType().CanBeCastTo(_propertyType)
                       ? context.RawValueFromRequest
                       : _strategy.Convert(context);

That boolean will always be false, meaning it will always call _strategy.Convert(context).

That can't be right, can it?

When I put some unit tests on BasicConverterFamily, it failed (unless I passed in a IConverterStrategy).

Almost all the other converter families (EnvironmentVariable, Numeric, Boolean, for example) were BindingValue aware.

Only PassthroughConverter, BasicConverterFamily, and ConversionPropertyBinder were not BindingValue aware (or were making checks on RawRequestValue.GetType() which were bogus)

-Chad

Chad Myers

unread,
Apr 25, 2012, 10:42:12 PM4/25/12
to fubumv...@googlegroups.com
I have all the tests passing and I added a few more (to PassthroughConverter and BasicConversionFamily).

I local-rippled to FubuMVC and it fixed the bug and all FubuMVC tests pass.

I'm pretty confident on this one and I was very careful to test and add more tests.

Please let me know if I'm way off base here because this is feeling pretty good.

-Chad

Jeremy D. Miller

unread,
Apr 25, 2012, 11:54:19 PM4/25/12
to fubumv...@googlegroups.com
You're saying that this code:

return context.RawValueFromRequest.GetType().CanBeCastTo(_propertyType)
                       ? context.RawValueFromRequest
                       : _strategy.Convert(context);

should be:

return context.RawValueFromRequest.Value.GetType().CanBeCastTo(_propertyType)
                       ? context.RawValueFromRequest.Value // <---- change here
                       : _strategy.Convert(context);

???

That looks right from 10000 feet.
Sent: Wed, April 25, 2012 9:42:15 PM

Chad Myers

unread,
Apr 26, 2012, 8:02:01 AM4/26/12
to fubumv...@googlegroups.com

Yes.

I think it is RawValue, but the essence is the same.

Chad

Reply all
Reply to author
Forward
0 new messages