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

ModelState errors are wrong when using attributes like [FromHeader] #1681

Closed
rynowak opened this issue Dec 8, 2014 · 2 comments
Closed

ModelState errors are wrong when using attributes like [FromHeader] #1681

rynowak opened this issue Dec 8, 2014 · 2 comments
Assignees
Labels
Milestone

Comments

@rynowak
Copy link
Member

rynowak commented Dec 8, 2014

In this case the model state error has the wrong key when no value is supplied:

public void Create([FromHeader] string transactionId)
{
}

The error should have key transactionId, but it's transactionId.transactionId

@yishaigalatzer yishaigalatzer added this to the 6.0.0-beta2 milestone Dec 8, 2014
@rynowak
Copy link
Member Author

rynowak commented Dec 9, 2014

Analysis suggests this is an undocumented issue from WSR webapi (which this code is based upon).
I made the same change in the WSR codebase, and no unit tests broke as a result.

See

var modelStateKey = ModelBindingHelper.CreatePropertyModelName(ModelStateKey,

ModelStateKey will already be set to transactionId so this code will double up the name.

We encounter this issue because our 'greedy' model binders return true to halt the binding process. This is interpreted as a successful bind, and so validation runs. This has the effect of giving attributes like [FromHeader] 'required' behavior. This will be tracked by a separate issue.

rynowak added a commit that referenced this issue Dec 9, 2014
…omHeader]

The issue here is that a model state error is added with the model name
'doubled'. This is on a fairly obscure code path and the code dates back
to the original WSR git checkin of webapi. There are no wsr tests that
verify this behavior.

The cause here is that our 'greedy' model binders (like
FromHeaderModelBinder) return 'true' whether or not they successfully
found a model, because they don't want other model binders to run.

This also has an effect on the validation system. That means that
validators will run and attempt to validate the model (which may be null).
That's that rare case where we get to this code path.
rynowak added a commit that referenced this issue Dec 9, 2014
…omHeader]

The issue here is that a model state error is added with the model name
'doubled'. This is on a fairly obscure code path and the code dates back
to the original WSR git checkin of webapi. There are no wsr tests that
verify this behavior.

The cause here is that our 'greedy' model binders (like
FromHeaderModelBinder) return 'true' whether or not they successfully
found a model, because they don't want other model binders to run.

This also has an effect on the validation system. That means that
validators will run and attempt to validate the model (which may be null).
That's that rare case where we get to this code path.
rynowak added a commit that referenced this issue Dec 9, 2014
…omHeader]

The issue here is that a model state error is added with the model name
'doubled'. This is on a fairly obscure code path and the code dates back
to the original WSR git checkin of webapi. There are no wsr tests that
verify this behavior.

The cause here is that our 'greedy' model binders (like
FromHeaderModelBinder) return 'true' whether or not they successfully
found a model, because they don't want other model binders to run.

This also has an effect on the validation system. That means that
validators will run and attempt to validate the model (which may be null).
That's that rare case where we get to this code path.
rynowak added a commit that referenced this issue Dec 10, 2014
…omHeader]

The issue here is that a model state error is added with the model name
'doubled'. This is on a fairly obscure code path and the code dates back
to the original WSR git checkin of webapi. There are no wsr tests that
verify this behavior.

The cause here is that our 'greedy' model binders (like
FromHeaderModelBinder) return 'true' whether or not they successfully
found a model, because they don't want other model binders to run.

This also has an effect on the validation system. That means that
validators will run and attempt to validate the model (which may be null).
That's that rare case where we get to this code path.
rynowak added a commit that referenced this issue Dec 10, 2014
…omHeader]

The issue here is that a model state error is added with the model name
'doubled'. This is on a fairly obscure code path and the code dates back
to the original WSR git checkin of webapi. There are no wsr tests that
verify this behavior.

The cause here is that our 'greedy' model binders (like
FromHeaderModelBinder) return 'true' whether or not they successfully
found a model, because they don't want other model binders to run.

This also has an effect on the validation system. That means that
validators will run and attempt to validate the model (which may be null).
That's that rare case where we get to this code path.
@rynowak
Copy link
Member Author

rynowak commented Dec 10, 2014

Fixed in release 1631ca1

@rynowak rynowak closed this as completed Dec 10, 2014
rynowak added a commit that referenced this issue Dec 10, 2014
…omHeader]

The issue here is that a model state error is added with the model name
'doubled'. This is on a fairly obscure code path and the code dates back
to the original WSR git checkin of webapi. There are no wsr tests that
verify this behavior.

The cause here is that our 'greedy' model binders (like
FromHeaderModelBinder) return 'true' whether or not they successfully
found a model, because they don't want other model binders to run.

This also has an effect on the validation system. That means that
validators will run and attempt to validate the model (which may be null).
That's that rare case where we get to this code path.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants