Test and quick fix for #1257

[carltongibson]

Addresses #1257.

Fields defined explicitly with required=False will be in the list of exclusions from validation.

I did not address the idea for a list of non-required fields.

0: Adding (and documenting) an optional_fields list that simply acted as another sub-clause to the if field_name in exclusions... check would be simple enough.

However:

1. Do we want extra API here?
2. Would we need to do something clever if fields appeared in such a list but were also explicitly defined etc?

[tomchristie]

I did not address the idea for a list of non-required fields.

I don't think we want to include that - this look sufficient to me.

@carltongibson Go right ahead and merge this whenever you're happy with it.

[carltongibson]

Just waiting for Travis.

[carltongibson]

Given #1458, perhaps what's needed here is...

Remove from Exclusions if:

1. If it's required
   OR
2. If it's actually present in the data.

i.e. Don't exclude JUST because required=False — but only if the freedom not to provide the value is being taken advantage of.

The missing test case is required=False but data invalid data provided anyway. (I think)

[xordoquy]

I'm not sure about that one at all.
From the SO post, it looks like the initial issue was:

• the model field is marked as null=False
• the serializer is marked as optional
• no value is provided for the field

Therefore I don't see how a validation could possible pass.

[carltongibson]

The use-case is when you want/commit to providing the value yourself (in say pre_save) if it's not provided to the serialiser.

For null=False model fields we have three cases:

1. I'm required to provide it as part of the serialiser data. — "The Normal Case"
2. I exclude it from the serialiser (or mark it read-only) provide it implicitly (in say pre_save) — "The Current-User-Owns-This Case"
3. I can provide it if I wish but don't have to — "The Current Case"

The essence of #1257 is that we were only covering the first two.

#1405 tests that the serialiser doesn't require you to submit a value in The Current Case — it doesn't check that you've correctly implemented (say) pre_save to ensure the model will pass validation when you try and save it — but that's a bit out of scope — it's a programming error that it's up to you to avoid.

#1405 incorrectly removes the validation (at the serialiser level) in all cases — rather than just when there's no data. So it's only half a fix.

Making sure that the field is not excluded when there is data should be the rest. (i.e. All the tests, including that in #1459, should pass.)

I have two issues here:

1. I'm not sure of the "is there data for this field" test. It should be if self.init_data.has_attr(...) for the field. But I fear edge-cases like nested serialisers and such. And, related...
2. There's a number of boolean conditions combined in this conditional which makes it hard to see through instantly. If we put the "Don't not exclude if the field is required OR if there is data for it" last do we thereby eliminate the need to do anything more complex than the has_attr in 1?

(Sorry, it's been a long day.)

[carltongibson]

It has been a long day. I mean has_key throughout. (But the list case would need handling too.)

[xordoquy]

has_key shouldn't be used as it's not supported by Python3. We should use key in dict instead.
This seems to fix the working test case and I didn't noticed regressions on optionals serializers on my own test suite.

However, unfortunately, this isn't as easy as it seems. initial_data might be a list if the serializer has many=True in which case, depending on the current object, you may or not remove a given field from the validation :(

[xordoquy]

I updated #1459 with a more complex test case and a possible fix.

[tomchristie]

Sounds good @carltongibson, yup.

[carltongibson]

#1459 Covers the missing case — I was thinking it should be at the serialiser level rather than the model level but then I checked and saw that get_validation_exclusions is only on ModelSerializers anyway... :-)

I'm not up to speed on where things stand with nested validation but, is there an obvious implementation for a init_data_has_value_for_field(self, field). (Using self.init_data.has_attr(...) or such)

[tomchristie]

@carltongibson @xordoquy - You two seem to have this covered. Can I delegate the resolution of it onto you both? Whenever you're both satisfied feel free to go ahead and merge. (@xordoquy - feel free to ping me if you'd like to be added as a collaborator - your call)