fuel-dev team mailing list archive

[Aleksey Kasatkin <akasatkin@xxxxxxxxxxxx>]

Igor,

AFAIC, validators refactoring is another task that should be done in
parallel.

In my opinion, documenting of API (parameters JSONs) should be done as a
first step to improve input cases coverage.
AFAIK, we have the same task for the orchestrator: there is no
documentation on its input yet (just some fragments and samples).


Aleksey Kasatkin

S. Software Developer | Mirantis, Inc. | http://www.mirantis.com
cell: +380938330852 | skype: alexeyk_ru


On Tue, May 20, 2014 at 3:50 PM, Igor Kalnitsky <ikalnitsky@xxxxxxxxxxxx>wrote:

> Hi guys,
>
> Indeed, our validators don't cover all input cases and it's bad. Moreover,
> our validators is too complicated to support and we have some plans to
> refactor them.
>
> Here the blueprints we have:
>
> -
> https://blueprints.launchpad.net/fuel/+spec/nailgun-validators-to-objects
> - https://blueprints.launchpad.net/fuel/+spec/nailgun-validators-testing
> - https://blueprints.launchpad.net/fuel/+spec/nailgun-validators-json
>
> So.. In order to reduce our job, I propose to implement this blueprint<https://blueprints.launchpad.net/fuel/+spec/nailgun-validators-testing>first, and then to cover input cases as much as possible.
>
> What do you think about it?
>
> - Igor
>
>
> On Tue, May 20, 2014 at 3:30 PM, Aleksey Kasatkin <akasatkin@xxxxxxxxxxxx>wrote:
>
>> Vitaly,
>> That's all about validation of input data indeed. There is no sense to
>> validate own output.
>> You had proposals about declarative description of validation logic in
>> April in Moscow. Can you provide links to materials on this topic?
>>
>>
>> Aleksey Kasatkin
>>
>> S. Software Developer | Mirantis, Inc. | http://www.mirantis.com
>> cell: +380938330852 | skype: alexeyk_ru
>>
>>
>> On Tue, May 20, 2014 at 3:10 PM, Vitaly Kramskikh <
>> vkramskikh@xxxxxxxxxxxx> wrote:
>>
>>> Aleksey,
>>>
>>> As for that particular bug, there was an UI issue, so UI sent disk names
>>> which didn't exist on a node and backend code didn't handle this case
>>> properly, so that caused 500 error. It has nothing to do with validation of
>>> user input, it is a check of existence of every model mentioned in a
>>> request and it must always be performed on backend. UI and CLI have nothing
>>> to validate, as they construct this data, and UI was constructing it
>>> improperly. We had/have cases where absence of this kind of validation
>>> leads to 500 error on every request to node handler, so nailgun becomes
>>> unusable.
>>>
>>> As for validation of user input, I think we need to describe
>>> declaratively as many validation rules as possible to minimize amounts of
>>> validation logic in backend and UI. Refactoring of networks is a good
>>> example - now we have a set of flags for every network so size of
>>> validation code reduced dramatically and we don't have hardcoded network
>>> names anywhere. The same can be done for roles:
>>> https://review.openstack.org/#/c/89601/6/nailgun/nailgun/fixtures/openstack.yaml.
>>> So UI and backend should use these configs to validate data. Also complex
>>> validation logic which cannot be described declaratively should be
>>> validated on backend only (like intersection of networks).
>>>
>>> I think CLI should not have any validation at all and just display
>>> server response in case of 4xx error.
>>>
>>>
>>> 2014-05-20 13:56 GMT+04:00 Aleksey Kasatkin <akasatkin@xxxxxxxxxxxx>:
>>>
>>> Yes, the validation of input data in Nailgun doesn't cover all possible
>>>> data integrity problems. We just send data from UI and CLI using right
>>>> format. It's true for networking data as well (e.g. networks roles are not
>>>> validated, networks can be duplicated).
>>>> AFAIC, it's very simple to get 500 error using curl. Data validation in
>>>> Nailgun should be strengthened if we want to get the same usability
>>>> for API as for UI and CLI.
>>>> It is partly implemented with introduction of JSON schemas for objects.
>>>> Additional validation logic should be added, especially for complex data
>>>> sets like disks and networking data.
>>>> We enhance data validation occasionally but API is not documented
>>>> enough (no description on parameters JSONs). We should get it covered with
>>>> documentation at first and make proper validation according to that doc
>>>> then (as it was with Networking limitations: see
>>>> https://etherpad.openstack.org/p/limitations-of-networking-configuration).
>>>>
>>>> Validation for CLI can be done similarly.
>>>>
>>>> Vitaly, please comment on this problem from UI point of view. What can
>>>> be done to formalize validation in UI and to make it systematic?
>>>>
>>>>
>>>> Aleksey Kasatkin
>>>>
>>>> S. Software Developer | Mirantis, Inc. | http://www.mirantis.com
>>>> cell: +380938330852 | skype: alexeyk_ru
>>>>
>>>>
>>>> On Mon, May 19, 2014 at 3:35 PM, Ivan Kolodyazhny <
>>>> ikolodyazhny@xxxxxxxxxxxx> wrote:
>>>>
>>>>> Hi Fuel devs!
>>>>>
>>>>> I'm looking on fix https://bugs.launchpad.net/fuel/+bug/1319306. This
>>>>> bug is fixed only for Web UI. But it should be fixed on back-end site too.
>>>>> Let's add validation for it. AFAIK, at least we've got similar errors with
>>>>> roles assignment validation earlier.
>>>>>
>>>>> I agree, that we need as much as possible validations on our Web UI.
>>>>> But back-end validation is very important too. It must be implemented
>>>>> because Web UI is only one of possible users of our API. A good REST API
>>>>> must not raises 500 errors.  4xx HTTP status codes should be used for any
>>>>> case of invalid data. It's a good practice and it makes API usage less
>>>>> risky.
>>>>>
>>>>> I propose to implement validation on every API client and API handlers:
>>>>>
>>>>>    - API handlers - all input data must be validated;
>>>>>    - Web UI - all input data should be validated as much as possible;
>>>>>    - Fuel (CLI) Client - at least, all required parameters should be
>>>>>    validated.
>>>>>
>>>>>
>>>>> Let's discuss my proposals and file bugs and blueprints if needed.
>>>>>
>>>>> --
>>>>> Regards,
>>>>> Ivan Kolodyazhny,
>>>>> Software Engineer, Mirantis, Inc.
>>>>>
>>>>> --
>>>>> Mailing list: https://launchpad.net/~fuel-dev
>>>>> Post to     : fuel-dev@xxxxxxxxxxxxxxxxxxx
>>>>> Unsubscribe : https://launchpad.net/~fuel-dev
>>>>> More help   : https://help.launchpad.net/ListHelp
>>>>>
>>>>>
>>>>
>>>
>>>
>>> --
>>> Vitaly Kramskikh,
>>> Software Engineer,
>>> Mirantis, Inc.
>>>
>>
>>
>> --
>> Mailing list: https://launchpad.net/~fuel-dev
>> Post to     : fuel-dev@xxxxxxxxxxxxxxxxxxx
>> Unsubscribe : https://launchpad.net/~fuel-dev
>> More help   : https://help.launchpad.net/ListHelp
>>
>>
>