Allow null option for properties #64

andreashug · 2016-01-04T11:11:46Z

Proposal for solving #47

The Swagger specification in version 2 doesn't support 'null' as
property value. This adds a vendor extension 'x-nullable' which
allows properties to be 'null' if set to 'true'.

msander · 2016-01-06T13:47:36Z

+1

analogue · 2016-01-09T00:43:48Z

Thanks for the PR. Haven't had time to review yet so it will probably be next week.

analogue · 2016-01-15T17:34:00Z

tests/swagger20_validator/type_validator_test.py

+@patch('jsonschema._validators.type_draft4')
+def test_skip_when_nullable_property_schema_and_value_is_None(
+        m_draft4_type_validator, minimal_swagger_spec):
+    param_schema = {'name': 'foo', 'x-nullable': True, 'type': 'string'}


I think you mean for this to be prop_schema with no name key.

You are right. I'll change that.

analogue · 2016-01-15T17:46:42Z

What is your interpretation of x-nullable when required==True? If I'm reading this correctly, when a property is nullable and required and the value passed is None, then that makes it OK. Hence, there would have to be changes to unmarshal_primitive(..) and marshal_primitive(..)

andreashug · 2016-01-26T18:57:26Z

My understanding of the spec is that required==True only means the property must exist and x-nullable==True allows null as value.

	`required==False`	`required==True`
`x-nullable==False`	`{ }->ok` `{"x": null}->fail`	`{ }->fail` `{"x": null}->fail`
`x-nullable==True`	`{ }->ok` `{"x": null}->ok`	`{ }->fail` `{"x": null}->ok`

I'm not sure if this is possible because unmarshal_object(..) re-introduces missing properties with None. What do you think?

andreashug · 2016-02-05T16:29:18Z

While searching for a good solution, I discovered that JSON Schema allows a list of types - e.g. { "schema": { "type": ["string", "null"] }
Actually I think this would be a much better way to solve my problem and it works well with jsonschema. Unfortunately it's not supported bravado-core yet. I'll try to come up with a proposal so this pull request can be postponed.

wasbazi · 2016-06-08T19:53:28Z

This would be a really nice feature to have! There is a long thread on the OpenAPI-Specification (formerly The Swagger Specification) repository discussing how they want to solve this and leaning towards the x-nullable case OAI/OpenAPI-Specification#229

Unfortunately it seems from the discussion that using arrays for type is not intended with the 2.0 spec (which seems to be indicated as feature frozen) OAI/OpenAPI-Specification#229 (comment)

Are there things I can do to help get this PR finished?

cyncyncyn · 2016-06-27T21:31:31Z

As @wasbazi says, this would be a nice feature to have!
I also offer my help in order to get this PR merged.
Thanks!

sjaensch · 2016-06-28T09:57:24Z

The CR has an outstanding issue, see #64 (comment)
The proposed solution of going with a list of types as allowed by JSON Schema will not be possible, the Swagger spec authors already clarified that Swagger 2.0 is not supposed to allow a list of types.

normalex · 2016-07-01T19:21:54Z

Can we merge this? people are eager to see it released.

The schema passed to type_validator is a prop_schema in these test cases. Yelp#64 (diff)

The Swagger specification in version 2 doesn't support 'null' as property value. This adds a vendor extension 'x-nullable' which allows properties to be 'null' if set to 'true'. See OAI/OpenAPI-Specification#229

coveralls · 2016-07-22T20:56:51Z

Coverage increased (+0.01%) to 96.989% when pulling a6c1382 on andreashug:features/allownull into 25bb775 on Yelp:master.

The schema passed to type_validator is a prop_schema in these test cases. Yelp#64 (diff)

coveralls · 2016-07-22T21:01:18Z

Coverage increased (+0.01%) to 96.989% when pulling 2a438d6 on andreashug:features/allownull into 25bb775 on Yelp:master.

The tests show that unmarshaling is not affected by the null option.

coveralls · 2016-07-22T21:11:16Z

Coverage increased (+0.01%) to 96.989% when pulling c581aab on andreashug:features/allownull into 25bb775 on Yelp:master.

andreashug · 2016-07-22T21:47:41Z

Finally found some time to continue with this.

Rebased on recent master
Fixed test according to Allow null option for properties #64 (comment)
Integration tests for unmarshaling

The good news is that unmarshaling doesn't seem to be affected (see matrix in https://github.com/andreashug/bravado-core/blob/c581aaba8924afe340f124375015afc9f5743762/tests/unmarshal/unmarshal_nullable_test.py)

The not so good news however is, I'm not sure how marshaling should behave in combination with x-nullable. Currently, marshal_object() drops keys whose value is None, which leads to a ValidationError (required property) ->

bravado-core/bravado_core/marshal.py

Lines 129 to 130 in 7d13d25

    
           if v is None: 
        
               continue

. One might think, it should keep the key if x-nullable is True. Or we could say that x-nullable is only for properties and not accounted in parameter specs.

Any opinions on that?

sjaensch · 2016-07-23T16:38:48Z

Great stuff @andreashug! I'm still travelling, I'll take a closer look at the patch on Tuesday.
Just to make sure I understand the issue you're seeing: If an endpoint expects a parameter of type object that has a x-nullable field then we get a ValidationError?

sjaensch · 2016-07-23T16:43:21Z

tests/swagger20_validator/type_validator_test.py

+        m_draft4_type_validator, minimal_swagger_spec):
+    prop_schema = {'x-nullable': False, 'type': 'string'}
+    args = (None, prop_schema['type'], None, prop_schema)
+    list(type_validator(minimal_swagger_spec, *args))


I'd use keyword arguments here as well, just like you did in the test above. It's more readable. I know it makes the test a bit longer since you have to repeat the arguments below, but I think that's fine.

This is because assert_called_once_with() expects the arguments without keywords. The existing tests in this file do it this way so I aligned with them ;-)

Reduce test complexity by splitting up in multiple tests, suggested in Yelp#64 (comment)

coveralls · 2016-07-24T08:36:33Z

Coverage increased (+0.01%) to 96.989% when pulling 0a65478 on andreashug:features/allownull into 25bb775 on Yelp:master.

andreashug · 2016-07-24T08:54:08Z

Regarding marshal_object(): The ValidationError is raised when x-allownull is True, the value is required the value is set to None. See https://dpaste.de/D0ax

sjaensch · 2016-07-27T09:54:43Z

This is great! Could you please create another paste? I didn't manage to open it in time. 😁

andreashug · 2016-07-28T07:46:57Z

Sure: http://pastebin.com/U3xeA48R (Seems like there is a bug in expiration settings on dpaste.de)

sjaensch · 2016-07-28T11:10:10Z

Thanks again @andreashug! While I think we could even ship this without fixing the edge case about a required x-nullable parameter, I think the fix is not that difficult:

diff --git a/bravado_core/marshal.py b/bravado_core/marshal.py
index 536e627..8a3771a 100644
--- a/bravado_core/marshal.py
+++ b/bravado_core/marshal.py
@@ -125,13 +125,14 @@ def marshal_object(swagger_spec, object_spec, object_value):
     result = {}
     for k, v in iteritems(object_value):

-        # Values cannot be None - skip them entirely!
-        if v is None:
-            continue
-
         prop_spec = get_spec_for_prop(
             swagger_spec, deref(object_spec), object_value, k)

+        # Values cannot be None unless x-nullable is set
+        is_nullable = prop_spec and prop_spec.get('x-nullable', False)
+        if v is None and not is_nullable:
+            continue
+

With it the test you wrote (which is in your dpaste) passes. We might want to handle the other cases (nullable primitives etc.) as well, but honestly I think it's not that important, since the use case for x-nullable are model properties in responses. You're welcome to incorporate this small change as well as the test in the branch though.

I'm going to find some time to test this with bravado and make sure it does what we want. 😊

sjaensch · 2016-07-29T09:43:49Z

An manual test using bravado to query an endpoint that returns a model with an x-nullable field worked perfectly! I'm going to merge this in and work on a second pull request that will contain my above change, a version bump and a big thank you to @andreashug in the changelog. 😊

andreashug · 2016-07-30T09:43:21Z

Yay, thanks \o/

analogue reviewed Jan 15, 2016
View reviewed changes

andreashug force-pushed the features/allownull branch from 2e60f00 to a6c1382 Compare July 22, 2016 20:53

andreashug added a commit to andreashug/bravado-core that referenced this pull request Jul 22, 2016

Fix type_validator_test according to comment in Yelp#64

c9fdbb4

The schema passed to type_validator is a prop_schema in these test cases. Yelp#64 (diff)

Allow null option for properties

2626153

The Swagger specification in version 2 doesn't support 'null' as property value. This adds a vendor extension 'x-nullable' which allows properties to be 'null' if set to 'true'. See OAI/OpenAPI-Specification#229

Fix type_validator_test according to comment in Yelp#64

a49ed30

The schema passed to type_validator is a prop_schema in these test cases. Yelp#64 (diff)

andreashug force-pushed the features/allownull branch from a6c1382 to 2a438d6 Compare July 22, 2016 20:58

Add integration tests for unmarshaling with null option

c581aab

The tests show that unmarshaling is not affected by the null option.

andreashug force-pushed the features/allownull branch from 2a438d6 to c581aab Compare July 22, 2016 21:07

sjaensch reviewed Jul 23, 2016
View reviewed changes

Refactor allownull integration tests

0a65478

Reduce test complexity by splitting up in multiple tests, suggested in Yelp#64 (comment)

sjaensch merged commit 6496b4d into Yelp:master Jul 29, 2016

andreashug deleted the features/allownull branch July 30, 2016 09:43

zroadhouse-rmn mentioned this pull request Aug 8, 2016

Add allow_null option Yelp/bravado#172

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow null option for properties #64

Allow null option for properties #64

andreashug commented Jan 4, 2016

msander commented Jan 6, 2016

analogue commented Jan 9, 2016

analogue Jan 15, 2016

andreashug Jan 26, 2016

analogue commented Jan 15, 2016

andreashug commented Jan 26, 2016

andreashug commented Feb 5, 2016

wasbazi commented Jun 8, 2016

cyncyncyn commented Jun 27, 2016

sjaensch commented Jun 28, 2016

normalex commented Jul 1, 2016

coveralls commented Jul 22, 2016 •

edited

Loading

coveralls commented Jul 22, 2016 •

edited

Loading

coveralls commented Jul 22, 2016

andreashug commented Jul 22, 2016

sjaensch commented Jul 23, 2016

sjaensch Jul 23, 2016 •

edited

Loading

andreashug Jul 23, 2016

coveralls commented Jul 24, 2016 •

edited

Loading

andreashug commented Jul 24, 2016

sjaensch commented Jul 27, 2016

andreashug commented Jul 28, 2016

sjaensch commented Jul 28, 2016

sjaensch commented Jul 29, 2016

andreashug commented Jul 30, 2016

Allow null option for properties #64

Allow null option for properties #64

Conversation

andreashug commented Jan 4, 2016

msander commented Jan 6, 2016

analogue commented Jan 9, 2016

analogue Jan 15, 2016

Choose a reason for hiding this comment

andreashug Jan 26, 2016

Choose a reason for hiding this comment

analogue commented Jan 15, 2016

andreashug commented Jan 26, 2016

andreashug commented Feb 5, 2016

wasbazi commented Jun 8, 2016

cyncyncyn commented Jun 27, 2016

sjaensch commented Jun 28, 2016

normalex commented Jul 1, 2016

coveralls commented Jul 22, 2016 • edited Loading

coveralls commented Jul 22, 2016 • edited Loading

coveralls commented Jul 22, 2016

andreashug commented Jul 22, 2016

sjaensch commented Jul 23, 2016

sjaensch Jul 23, 2016 • edited Loading

Choose a reason for hiding this comment

andreashug Jul 23, 2016

Choose a reason for hiding this comment

coveralls commented Jul 24, 2016 • edited Loading

andreashug commented Jul 24, 2016

sjaensch commented Jul 27, 2016

andreashug commented Jul 28, 2016

sjaensch commented Jul 28, 2016

sjaensch commented Jul 29, 2016

andreashug commented Jul 30, 2016

coveralls commented Jul 22, 2016 •

edited

Loading

coveralls commented Jul 22, 2016 •

edited

Loading

sjaensch Jul 23, 2016 •

edited

Loading

coveralls commented Jul 24, 2016 •

edited

Loading