Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

FromHeaderAttribute quietly ignores unsupported types #6016

Closed
kellypleahy opened this issue Mar 23, 2017 · 3 comments
Closed

FromHeaderAttribute quietly ignores unsupported types #6016

kellypleahy opened this issue Mar 23, 2017 · 3 comments

Comments

@kellypleahy
Copy link

FromHeaderAttribute (from looking at the code and spending several hours debugging) seems to only support either "string" or "compatible collections" for header binding. However, sometimes the header value is just a simple int or some other similar value (in my case it was a timeout).

While I'm fine with the fact that FromHeaderAttribute doesn't support int (in my case I wanted Nullable), it would be way nicer if it somehow told you this. I don't see any sort of logging and I'm not sure what the best way to "warn" about this problem is, so I guess I'm opening up for discussion.

Personally I would have liked an exception to be thrown for an unsupported type of parameter.

Notice I'm not mentioning what should happen if the value isn't convertible to the type of your binding target. I'd expect that this model binder should act the same as others in that case (though I don't know if there is a consistent behavior, nor do I know what it is).

The case I'm really concerned about is one where the compile time type of the object is not supported by the binder - it would be nice if it would complain ASAP on that.

Thanks.

P.S. if we can agree on a desired behavior, I'd be happy to write tests and submit a patch.

@Eilon
Copy link
Member

Eilon commented May 12, 2017

Hmm I thought there was a previous issue on this with some discussion, but I can't find it.

@rynowak / @dougbu - would it be relatively straightforward to have some sort of error for this case?

@rynowak
Copy link
Member

rynowak commented May 12, 2017

It would be easy to add. The reason why we generally don't do things in model binding is that we don't know if another binder (that you wrote) is going to handle it. But it's simple enough to just change the guidance and say register yours before ours

@Eilon
Copy link
Member

Eilon commented Jun 7, 2017

Closing as a dup of #5859.

@Eilon Eilon closed this as completed Jun 7, 2017
@Eilon Eilon added the duplicate label Jun 7, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants