Re: [openstack-dev] [nova][api] Strict validation in query parameters

2017-06-16 Thread Sean Dague 
On 06/15/2017 10:01 PM, Matt Riedemann wrote:
> On 6/15/2017 8:43 PM, Alex Xu wrote:
>> We added new decorator 'query_schema' to support validate the query
>> parameters by JSON-Schema.
>>
>> It provides more strict valiadation as below:
>> * set the 'additionalProperties=False' in the schema, it means that
>> reject any invalid query parameters and return HTTPBadRequest 400 to
>> the user.
>> * use the marco function 'single_param' to declare the specific query
>> parameter only support single value. For example, the 'marker'
>> parameters for the pagination actually only one value is the valid. If
>> the user specific multiple values "marker=1=2", the validation
>> will return 400 to the user.
>>
>> Currently there is patch related to this:
>> https://review.openstack.org/#/c/459483/13/nova/api/openstack/compute/schemas/server_migrations.py
>>
>>
>> So my question is:
>> Are we all good with this strict validation in all the future
>> microversion?
>>
>> I didn't remember we explicit agreement this at somewhere, just want
>> to double check this is the direction everybody want to go.
>>
>> Thanks
>> Alex
>>
>>
>> __
>>
>> OpenStack Development Mailing List (not for usage questions)
>> Unsubscribe:
>> openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>
> 
> I think this is fine and makes sense for new microversions. The spec for
> consistent query parameter validation does talk about it a bit:
> 
> https://specs.openstack.org/openstack/nova-specs/specs/ocata/implemented/consistent-query-parameters-validation.html#proposed-change
> 
> 
> "The behaviour additionalProperties as below:
> 
> * When the value of additionalProperties is True means the extra query
> parameters are allowed. But those extra query parameters will be
> stripped out.
> * When the value of additionalProperties is False means the extra query
> aren’t allowed.
> 
> The value of additionalProperties will be True until we decide to
> restrict the parameters in the future, and it will be changed with new
> microversion."
> 
> I don't see a point in allowing someone to specify a query parameter
> multiple times if we only pick the first one from the list and use that.

Agreed. The point of doing strict validation and returning a 400 is to
help the user eliminate bugs in their program. If they specified marker
twice either they thought it did something, or they made a mistake. Both
are wrong. When we are silent on that front it means they may not be
getting the behavior they were expecting, which hurts their experience
with the API.

-Sean

-- 
Sean Dague
http://dague.net

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova][api] Strict validation in query parameters

2017-06-15 Thread Ghanshyam Mann 
On Fri, Jun 16, 2017 at 11:01 AM, Matt Riedemann  wrote:
> On 6/15/2017 8:43 PM, Alex Xu wrote:
>>
>> We added new decorator 'query_schema' to support validate the query
>> parameters by JSON-Schema.
>>
>> It provides more strict valiadation as below:
>> * set the 'additionalProperties=False' in the schema, it means that reject
>> any invalid query parameters and return HTTPBadRequest 400 to the user.
>> * use the marco function 'single_param' to declare the specific query
>> parameter only support single value. For example, the 'marker' parameters
>> for the pagination actually only one value is the valid. If the user
>> specific multiple values "marker=1=2", the validation will return 400
>> to the user.
>>
>> Currently there is patch related to this:
>>
>> https://review.openstack.org/#/c/459483/13/nova/api/openstack/compute/schemas/server_migrations.py
>>
>> So my question is:
>> Are we all good with this strict validation in all the future
>> microversion?
>>
>> I didn't remember we explicit agreement this at somewhere, just want to
>> double check this is the direction everybody want to go.
>>
>> Thanks
>> Alex
>>
>>
>> __
>> OpenStack Development Mailing List (not for usage questions)
>> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>
>
> I think this is fine and makes sense for new microversions. The spec for
> consistent query parameter validation does talk about it a bit:
>
> https://specs.openstack.org/openstack/nova-specs/specs/ocata/implemented/consistent-query-parameters-validation.html#proposed-change
>
> "The behaviour additionalProperties as below:
>
> * When the value of additionalProperties is True means the extra query
> parameters are allowed. But those extra query parameters will be stripped
> out.
> * When the value of additionalProperties is False means the extra query
> aren’t allowed.
>
> The value of additionalProperties will be True until we decide to restrict
> the parameters in the future, and it will be changed with new microversion."
>
> I don't see a point in allowing someone to specify a query parameter
> multiple times if we only pick the first one from the list and use that.
>
> There are certain query parameters that we allow multiple instances of, for
> sorting I believe. But for other things like filtering restricting to 1
> should be fine, and using additionalProperties=False should also be fine on
> new microversions. For example, if we allow additional properties, someone
> could type the parameter name incorrectly and we'd just ignore it. With
> strict validation, we'll return a 400 which should be clear to the end user
> that what they requested as invalid and they need to fix it on their end.

Yea, strict validation is always good and makes interface hard to use
wrongly. Starting strict validation on query param with microversion
is nice way but yes we cannot do that without microversion.

+1 on that direction.

>
> --
>
> Thanks,
>
> Matt
>
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova][api] Strict validation in query parameters

2017-06-15 Thread Matt Riedemann 

On 6/15/2017 8:43 PM, Alex Xu wrote:
We added new decorator 'query_schema' to support validate the query 
parameters by JSON-Schema.


It provides more strict valiadation as below:
* set the 'additionalProperties=False' in the schema, it means that 
reject any invalid query parameters and return HTTPBadRequest 400 to the 
user.
* use the marco function 'single_param' to declare the specific query 
parameter only support single value. For example, the 'marker' 
parameters for the pagination actually only one value is the valid. If 
the user specific multiple values "marker=1=2", the validation 
will return 400 to the user.


Currently there is patch related to this:
https://review.openstack.org/#/c/459483/13/nova/api/openstack/compute/schemas/server_migrations.py

So my question is:
Are we all good with this strict validation in all the future microversion?

I didn't remember we explicit agreement this at somewhere, just want to 
double check this is the direction everybody want to go.


Thanks
Alex


__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev



I think this is fine and makes sense for new microversions. The spec for 
consistent query parameter validation does talk about it a bit:


https://specs.openstack.org/openstack/nova-specs/specs/ocata/implemented/consistent-query-parameters-validation.html#proposed-change

"The behaviour additionalProperties as below:

* When the value of additionalProperties is True means the extra query 
parameters are allowed. But those extra query parameters will be 
stripped out.
* When the value of additionalProperties is False means the extra query 
aren’t allowed.


The value of additionalProperties will be True until we decide to 
restrict the parameters in the future, and it will be changed with new 
microversion."


I don't see a point in allowing someone to specify a query parameter 
multiple times if we only pick the first one from the list and use that.


There are certain query parameters that we allow multiple instances of, 
for sorting I believe. But for other things like filtering restricting 
to 1 should be fine, and using additionalProperties=False should also be 
fine on new microversions. For example, if we allow additional 
properties, someone could type the parameter name incorrectly and we'd 
just ignore it. With strict validation, we'll return a 400 which should 
be clear to the end user that what they requested as invalid and they 
need to fix it on their end.


--

Thanks,

Matt

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

[openstack-dev] [nova][api] Strict validation in query parameters

2017-06-15 Thread Alex Xu 
We added new decorator 'query_schema' to support validate the query
parameters by JSON-Schema.

It provides more strict valiadation as below:
* set the 'additionalProperties=False' in the schema, it means that reject
any invalid query parameters and return HTTPBadRequest 400 to the user.
* use the marco function 'single_param' to declare the specific query
parameter only support single value. For example, the 'marker' parameters
for the pagination actually only one value is the valid. If the user
specific multiple values "marker=1=2", the validation will return
400 to the user.

Currently there is patch related to this:
https://review.openstack.org/#/c/459483/13/nova/api/openstack/compute/schemas/server_migrations.py

So my question is:
Are we all good with this strict validation in all the future microversion?

I didn't remember we explicit agreement this at somewhere, just want to
double check this is the direction everybody want to go.

Thanks
Alex
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev