New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dependency doesn't play well with required #665

Merged
merged 1 commit into from Jul 7, 2015

Conversation

Projects
None yet
3 participants
@rs
Copy link
Collaborator

rs commented Jul 2, 2015

If a field has both a dependency and a required directive, the required directive is just ignored. This PR contains a failing test to demonstrate the issue.

@nicolaiarocci

This comment has been minimized.

Copy link
Member

nicolaiarocci commented Jul 2, 2015

@rs

This comment has been minimized.

Copy link
Collaborator

rs commented Jul 2, 2015

In my case the dependency is met thru the default value of the depending field but still doesn’t complain if the required field is omitted. I can change the test to meet that case if you want.

Also I think that when you use required, you expect that whatever the other directives, you won’t end up with a missing field in the db.

@nicolaiarocci

This comment has been minimized.

Copy link
Member

nicolaiarocci commented Jul 3, 2015

Also I think that when you use required, you expect that whatever the other directives, you won’t end up with a missing field in the db.

it used to be like this. New behaviour was introduced with pyeve/cerberus/pull/61 by @hthieu1110. This test in particular shows that this is in fact the intended behaviour.

Now, I agree that this is somewhat convoluted. required being validated only when dependencies are met seems counter-intuitive. In this scenario it seems appropriate to give precedence to the required rule over dependencies.

I'd appreciate if @hthieu1110, as the original patch author, would chime in on this one.

nicolaiarocci referenced this pull request in pyeve/cerberus Jul 7, 2015

'required' is always validated regardless of any dependency.
'required' is always evaluated, independent of eventual missing
dependenies. This changes the previous behaviour whereas a required
field with dependencies would only be reported as missing if all
dependencies were met. A missing required field will always be reported.

Addresses nicolaiarocci/eve#665.

nicolaiarocci referenced this pull request in pyeve/cerberus Jul 7, 2015

'required' is always validated regardless of any dependency.
'required' is always evaluated, independent of eventual missing
dependencies. This changes the previous behaviour whereas a required
field with dependencies would only be reported as missing if all
dependencies were met. A missing required field will always be reported.

Addresses nicolaiarocci/eve#665.

@nicolaiarocci nicolaiarocci merged commit 724fe56 into pyeve:develop Jul 7, 2015

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@gmauleon

This comment has been minimized.

Copy link

gmauleon commented Sep 8, 2015

Sorry to jump in so late, just starting to use Eve. But then how could I require a field only if another field is of value X?

I got a field "mytype" with 2 possible value A and B, I want that when "mytype" is A only, then field "myfield" is required. I did that with a dependency set on the field "myfield" that need "mytype" to be A. But this patch broke that logic. Any workaround ?

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment