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 <[email protected]>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 > <[email protected]>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 < >> [email protected]> 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 <[email protected]>: >>> >>> 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 < >>>> [email protected]> 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 : [email protected] >>>>> 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 : [email protected] >> Unsubscribe : https://launchpad.net/~fuel-dev >> More help : https://help.launchpad.net/ListHelp >> >> >
-- Mailing list: https://launchpad.net/~fuel-dev Post to : [email protected] Unsubscribe : https://launchpad.net/~fuel-dev More help : https://help.launchpad.net/ListHelp
